[IDE] Teach type checker about conditional conformance extensions. #14554

huonw · 2018-02-12T14:38:17Z

Before conditional conformances, the archetypes in conformance
extensions (i.e. extension Foo: SomeProtocol) were equivalent to those
in the type decl, with the same protocol bounds and so on. The code for
printing "synthesized" members relied on this fact. This commit teaches
that code to deal with archetypes in the conditional conformance
extension when required.

Fixes rdar://problem/36553066 and SR-6930.

No point in getting rid of the sugar so early.

huonw · 2018-02-12T14:40:04Z

@DougGregor thoughts on the safety of this for the swift-*-branches? It's kinda huge (but, fortunately, 300 lines of the net 400 increase are from the expected output of a test case), and fiddly. :(

huonw · 2018-02-12T14:40:13Z

@swift-ci please smoke test

DougGregor

This is looking great. It's definitely fine for the Swift 5.0 branch. And although it's quite a bit of code churn... the changes are fairly clear and you've made the code fairly defensive, so I think on balance it's safe for the 4.1 branch. Thank you!

DougGregor · 2018-02-12T17:23:40Z

lib/IDE/IDETypeChecking.cpp

-    if (!Ext->isConstrainedExtension()) {
+    auto conformanceIsConditional =
+        Conf && !Conf->getConditionalRequirements().empty();
+    if (!Ext->isConstrainedExtension() && !conformanceIsConditional) {


It seems to me that one cannot have a conditional conformance without also having a constrained extension?

It's confusing (took me 10 minutes to remember why I had to have this... will add a comment), but they're referring to two different extensions:

protocol Foo {} extension Foo { ... } // Ext struct Bar<T> {} extension Bar : Foo where T: Foo {} // Conf

Oh, of course! Ext is the protocol extension we're considering, and Conf is referring to a nominal type's conditional conformance to the corresponding protocol.

DougGregor · 2018-02-12T17:25:45Z

lib/IDE/IDETypeChecking.cpp

+
+        // FIXME: Could do something here
+        if (Kind == RequirementKind::Layout)
+          continue;


I know this isn't your new code, but it'd be so nice to at least check for class constraints here. I guess we'll wait for the isApplicable rewrite.

DougGregor · 2018-02-12T17:27:38Z

lib/IDE/IDETypeChecking.cpp

+          First = First.subst(subMap);
+          Second = Second.subst(subMap);
+
+          if (!First || !Second) {


Random: should we move this safety code out of the if? This particular code path is known to be a bit crashy, and maybe that would help somewhat...

I think that can probably become Req.subst(subMap) too, but I was keeping everything the same to reduce risk as much as possible, since this patch is large enough. (And similarly for the class constraints.)

DougGregor · 2018-02-12T17:30:20Z

lib/IDE/ModuleInterfacePrinting.cpp

-                  Printer << "\n";
-            }
-          });
+              // For top-level decls, only constraint extensions are to print;


If you're revising stuff, could you clean up the comment grammar here?

DougGregor · 2018-02-12T17:44:55Z

tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp

+  NominalTypeDecl *SynthesizedTargetNTD = nullptr;
+  if (SynthesizedTarget) {
+    SynthesizedTargetNTD = SynthesizedTarget.dyn_cast<NominalTypeDecl *>();
+    if (!SynthesizedTargetNTD)


I'd appreciate some curly braces for the 3-line body here.

DougGregor · 2018-02-12T17:46:22Z

tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp

                          DocInfoConsumer &Consumer) {
  if (!D || isa<ParamDecl>(D))
    return;
  if (const auto *ED = dyn_cast<ExtensionDecl>(D)) {
    if (SynthesizedTarget) {
-      passExtends((const ValueDecl*)SynthesizedTarget, Consumer);
+      auto Extends = SynthesizedTarget.dyn_cast<NominalTypeDecl *>();
+      if (!Extends)


You have this same "figure out the nominal type declaration to which SynthesizedTarget refers" pattern in two places; can you pull it out into a function somewhere?

Oh, yeah, bit embarrassing that I missed that. I've made TypeOrExtensionDecl a wrapper struct with some convenience methods like getBaseNominal.

Although, it is in a somewhat clunky place: PrintOptions.h. Any thoughts for where something like that should go? Decl.h seems wrong to me.

I suspect that TypeOrExtensionDecl could be more widely used, if we were to search around for pointer unions similar to the one you're replacing. It's fine to put it in Decl.h

FWIW, I couldn't find any other NominalTypeDecl, ExtensionDecl pointer unions.

Ugh, moving it to Decl.h is rather ugly: PrintOptions.h doesn't/can't include that file, so everything has to be indirected...

DougGregor · 2018-02-12T17:47:06Z

tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp

                                    Optional<BracketOptions> Bracket) override {
    // When we start print a synthesized extension, record the target's USR.
    llvm::SmallString<64> Buf;
    llvm::raw_svector_ostream OS(Buf);
-    if (!SwiftLangSupport::printUSR(Target, OS)) {
+    auto TargetNTD = Target.dyn_cast<NominalTypeDecl *>();
+    if (!TargetNTD)


Another copy of this pattern

DougGregor · 2018-02-12T17:47:40Z

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

-  if (TargetToAnalyzerMap.count(Target) == 0) {
+  NominalTypeDecl *TargetNTD = Target.dyn_cast<NominalTypeDecl *>();
+  if (!TargetNTD)
+    TargetNTD = Target.get<ExtensionDecl *>()


Another one :)

huonw · 2018-02-13T00:31:02Z

@swift-ci please test

huonw · 2018-02-13T00:42:39Z

@swift-ci please test

huonw · 2018-02-13T01:12:38Z

@swift-ci please test

huonw · 2018-02-13T01:29:16Z

@swift-ci test.

huonw · 2018-02-13T02:59:41Z

@swift-ci test.

huonw · 2018-02-13T05:07:36Z

@swift-ci test

swift-ci · 2018-02-13T05:10:03Z

Build failed
Swift Test OS X Platform
Git Sha - da7b9d6fe59b31911aa85963d7e5a77b67efad77

swift-ci · 2018-02-13T05:27:31Z

Build failed
Swift Test Linux Platform
Git Sha - da7b9d6fe59b31911aa85963d7e5a77b67efad77

Before conditional conformances, the archetypes in conformance extensions (i.e. extension Foo: SomeProtocol) were equivalent to those in the type decl, with the same protocol bounds and so on. The code for printing "synthesized" members relied on this fact. This commit teaches that code to deal with archetypes in the conditional conformance extension when required. Fixes rdar://problem/36553066 and SR-6930.

huonw · 2018-02-13T06:39:29Z

@swift-ci please smoke test and merge

huonw · 2018-02-13T06:53:37Z

@swift-ci please smoke test and merge

huonw · 2018-02-13T07:52:16Z

@swift-ci please smoke test and merge

huonw · 2018-02-13T11:48:49Z

@swift-ci please smoke test and merge

huonw · 2018-02-13T11:56:20Z

@swift-ci please smoke test

[AST] Store sugared requirements in NormalProtocolConformances.

715ca9d

No point in getting rid of the sugar so early.

DougGregor approved these changes Feb 12, 2018

View reviewed changes

huonw force-pushed the doc-conditional-conformances branch from b4cfd93 to da7b9d6 Compare February 13, 2018 00:30

huonw force-pushed the doc-conditional-conformances branch from da7b9d6 to cb60dbe Compare February 13, 2018 06:37

huonw merged commit 56cf077 into swiftlang:master Feb 13, 2018

huonw deleted the doc-conditional-conformances branch February 13, 2018 20:21

huonw mentioned this pull request Feb 14, 2018

[Sema] Disallow conditional conformances on objective-c generics. #14628

Merged

huonw mannequin mentioned this pull request Feb 13, 2018

[SR-6930] Documentation should not refer to the τ_0_0.Element which is a compilation time artifact. #49478

Closed

[IDE] Teach type checker about conditional conformance extensions. #14554

[IDE] Teach type checker about conditional conformance extensions. #14554

Uh oh!

Conversation

huonw commented Feb 12, 2018

Uh oh!

huonw commented Feb 12, 2018

Uh oh!

huonw commented Feb 12, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

swift-ci commented Feb 13, 2018

Uh oh!

swift-ci commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

huonw commented Feb 13, 2018

Uh oh!

Uh oh!