[CodeCompletion] Avoid type checking all top-level decls in the primary file #27981

rintaro · 2019-10-31T00:23:59Z

The first commit is purely bug fixes. They are revealed if the second commit is enabled. However they can be reproduced if the declaration is in the other files in the current module. (Added test/IDE/complete_multifile.swift regression test case).

Default memberwise initializer for struct was not suggested
Dynamic member lookup didn't work
init(rawValue:) was not suggested for RawRepresentable types (rdar://problem/56391233)
'$name' was not suggested for @propertyWrappered value

The second commit actually stops eager typechecking decls in the primary file.

Use performParseAndResolveImportsOnly() to invoke the frontend
Do bindExtensions() in ide::typeCheckContextUntil()
Typecheck preceding TopLevelCodeDecls only if the compleiton is in
a TopLevelCodeDecl
Other related tweaks

rdar://problem/56636747

rintaro · 2019-10-31T00:38:17Z

lib/IDE/CodeCompletion.cpp

+    auto Access = VD->getFormalAccess();
+    // Use the greater access between the protocol requirement and the witness.
+    // In case of:
+    //
+    //   public protocol P { func foo() }
+    //   public class B { func foo() {} }
+    //   public class C: B, P {
+    //     <complete>
+    //   }
+    //
+    // 'VD' is 'B.foo()' which is implicitly 'internal'. But as the overriding
+    // declaration, the user needs to write both 'public' and 'override':
+    //
+    //   public class C: B {
+    //     public override func foo() {}
+    //   }
+    if (Access < AccessLevel::Public &&
+        !isa<ProtocolDecl>(VD->getDeclContext())) {
+      for (auto Conformance : CurrentNominal->getAllConformances()) {
+        auto Proto = Conformance->getProtocol();
+        for (auto Member : Proto->getMembers()) {
+          auto Requirement = dyn_cast<ValueDecl>(Member);
+          if (!Requirement || isa<AssociatedTypeDecl>(Requirement))
+            continue;
+
+          auto Witness = Conformance->getWitnessDecl(Requirement);
+          if (Witness == VD)
+            Access = std::max(Access, Requirement->getFormalAccess());
+        }
+      }
+    }


Previously B.foo() is overwriteAccess(AccessLevel::Public)ed from diagnoseWitnessFixAccessLevel(). Since the typechecking is now in DiagnosticSuppression, diagnoseWitnessFixAccessLevel() is not called so we need to manually check it.

rintaro · 2019-10-31T00:39:43Z

lib/IDE/CodeCompletion.cpp

+      return;
+
+    // A 'class' member with an initial value cannot be overriden either.
+    if (D->isStatic() && isa<VarDecl>(D) && cast<VarDecl>(D)->hasInitialValue())


HasInitialValueAttr is injected in the typechecking.

rintaro · 2019-10-31T00:42:00Z

lib/IDE/CodeCompletion.cpp

@@ -5096,6 +5135,9 @@ void CodeCompletionCallbacksImpl::doneParsing() {
      CurDeclContext,
      CurDeclContext->getASTContext().SourceMgr.getCodeCompletionLoc());

+  // Add keywords even if type checking fails completely.
+  addKeywords(CompletionContext.getResultSink(), MaybeFuncBody);


Moved after typeCheckContextUntil() because addKeywords() may call DeclContext::getSelfProtocolDecl() which can only be called after bindExtensions().

lib/Sema/TypeCheckAttr.cpp

rintaro · 2019-10-31T00:44:49Z

@swift-ci Please smoke test

rintaro · 2019-10-31T06:19:05Z

@swift-ci Please smoke test

rintaro · 2019-10-31T13:49:13Z

@swift-ci Please test Linux

swift-ci · 2019-10-31T13:51:35Z

Build failed
Swift Test Linux Platform
Git Sha - 883cbd4005925c9adad35506e888c20bc6dc4ec3

rintaro · 2019-10-31T16:30:32Z

@swift-ci Please test Linux

rintaro · 2019-10-31T21:30:21Z

@swift-ci Please smoke test

benlangmuir · 2019-10-31T22:19:23Z

lib/Sema/LookupVisibleDecls.cpp

@@ -483,6 +477,58 @@ static void
  lookupTypeMembers(BaseTy, PT, Consumer, CurrDC, LS, Reason);
 }

+/// Trigger synthesizing implicit member declarations to make them "visible".
+static void synthesizeMemberDeclsForLookup(NominalTypeDecl *NTD,


How is this triggered during normal type-checking? This seems kind of brittle to live in lookup-visible-decls, although admittedly this whole file is basically like that...

If the type is declared in the primary files, they are synthesized in TypeChecker::typeCheckDecl()(DeclChecker) or in typeCheckFunctionsAndExternalDecls().

But if it's in the non-primary files, AFAIU, they are lazily synthesized on demand in DeclContext::lookupQualified()
https://github.com/apple/swift/blob/bd971507bae5a12a2b40365e795799037ec59fcc/lib/AST/NameLookup.cpp#L1573-L1579
https://github.com/apple/swift/blob/bd971507bae5a12a2b40365e795799037ec59fcc/lib/Sema/TypeChecker.h#L1018-L1024

I can move this synthesizeMemberDeclsForLookup() function into TypeChecker::, but I don't think it's worth...

rintaro · 2019-11-05T18:40:51Z

Resolved conflicts.
@swift-ci Please smoke test

slavapestov · 2019-11-05T23:06:42Z

lib/IDE/CodeCompletion.cpp

+    if (D->isFinal())
+      return;
+
+    // A 'class' member with an initial value cannot be overriden either.


I'm not sure I understand. Can you give an example? Normally stored properties can be overridden with computed properties as long as they're not final.

I believe this meant something like:

class C { class var foo = 1 }

But that's compile error, so we don't care much how they are treated in override completion. So maybe this check is not needed.
But here, I'm just changing D->getAttrs().hasAttribute<HasInitialValueAttr>() to isa<VarDecl>(D) && cast<VarDecl>(D)->hasInitialValue()

My point was that the presence of an initial value expression does not have any bearing on whether the declaration can be overridden or not. The comment above is incorrect.

I think the only relevant check here is whether the property is final (static is equivalent to class final).

Right, I just removed this check.

slavapestov · 2019-11-05T23:08:18Z

lib/IDE/CodeCompletion.cpp

+
+          auto Witness = Conformance->getWitnessDecl(Requirement);
+          if (Witness == VD)
+            Access = std::max(Access, Requirement->getFormalAccess());


The access of the requirement is the same as the access of the protocol, so I believe this entire loop and the search for the witness of 'VD' can go away.

You mean like this?

if (Access < AccessLevel::Public && !isa<ProtocolDecl>(VD->getDeclContext())) { for (auto Conformance : CurrentNominal->getAllConformances()) { auto Proto = Conformance->getProtocol(); Access = std::max(Access, Proto->getFormalAccess()); } }

This may cause unnecessary public when the type conforms to two or more protocols. Like

public protocol P1 { } protocol P2 { func bar() } public class B { func bar() {} } public class C: B, P1, P2 { <complete> }

In this case when overriding bar(), C.bar() is not needed to be public.

slavapestov · 2019-11-05T23:09:08Z

lib/IDE/ExprContextAnalysis.cpp

+  // protocols in the AST.
+  (void) DC->getASTContext().getStdlibModule();
+
+  bindExtensions(*DC->getParentSourceFile());


Maybe the getStdlibModule() and bindExtensions() calls should be done as part of performParseAndNameBindingOnly()?

Made bindExtensions() a part of performParseAndNameBindingOnly().

It turned out that getStdlibModule() is meaningless. Since ASTContext::getStdlibModule(bool loadIfAbsent = false) is called with default loadIfAbsent = false, it doesn't do anything. So I just removed it from performTypechecking()

slavapestov · 2019-11-05T23:10:27Z

lib/Sema/LookupVisibleDecls.cpp

+        Conformance->getRootConformance());
+    if (!NormalConformance)
+      continue;
+    for (auto Member : Proto->getMembers()) {


You can replace this loop with calls to Conformance->forEachTypeWitness() and forEachValueWitness(), passing in useResolver=true.

slavapestov · 2019-11-05T23:10:58Z

lib/Sema/LookupVisibleDecls.cpp

+    for (auto Member : IDC->getMembers()) {
+      if (auto var = dyn_cast<VarDecl>(Member)) {
+        if (var->hasAttachedPropertyWrapper()) {
+          auto sourceFile = var->getDeclContext()->getParentSourceFile();


You can check if the IDC itself is a member of a source file outside the loop.

Thanks, done!

slavapestov · 2019-11-05T23:12:10Z

lib/Sema/LookupVisibleDecls.cpp

+                                           const DeclContext *DC) {
+  // Synthesize the memberwise initializer for structs or default initializer
+  // for classes.
+  if (!NTD->hasInterfaceType())


It's possible that an interface type has been set, but implicit constructors were not added (and vice versa). You can unconditinally call addImplicitConstructors() here; it's idempotent if called multiple times.

IIRC, that causes circular dependency error. I will try that again.

Here's the cyclic dependency error when enabling typo correction. How can we fix this?

$ cat test.swift struct Broken { var b = True } $ swift -frontend -typecheck -typo-correction-limit 10 -debug-cycles test.swift test.swift:1:8: error: circular reference struct Broken { ^ test.swift:2:3: note: through reference here var b = True ^ test.swift:2:7: note: through reference here var b = True ^ test.swift:2:7: note: through reference here var b = True ^ ===CYCLE DETECTED=== `--SynthesizeMemberwiseInitRequest(test.(file)[email protected]:1:8) `--ParseMembersRequest(test.(file)[email protected]:1:8) -> {(unknown decl), test.(file)[email protected]:2:7} `--StorageImplInfoRequest(test.(file)[email protected]:2:7) | `--AttachedPropertyWrappersRequest(test.(file)[email protected]:2:7) `--StorageImplInfoRequest(test.(file)[email protected]:2:7) | `--AttachedPropertyWrappersRequest(test.(file)[email protected]:2:7) `--AccessLevelRequest(test.(file)[email protected]:2:7) `--AccessLevelRequest(test.(file)[email protected]:2:7) `--InterfaceTypeRequest(test.(file)[email protected]:2:7) `--NamingPatternRequest(test.(file)[email protected]:2:7) `--PatternBindingEntryRequest((unknown decl), 0) `--AttachedPropertyWrappersRequest(test.(file)[email protected]:2:7) ... `--ParseMembersRequest(test.(file)[email protected]:1:8) -> {(unknown decl), test.(file)[email protected]:2:7} `--GenericParamListRequest(test.(file)[email protected]:1:8) `--GenericSignatureRequest(test.(file)[email protected]:1:8) | `--GenericParamListRequest(test.(file)[email protected]:1:8) `--GenericParamListRequest(test.(file)[email protected]:1:8) `--InterfaceTypeRequest(test.(file)[email protected]:1:8) | `--GenericParamListRequest(test.(file)[email protected]:1:8) | `--GenericParamListRequest(test.(file)[email protected]:1:8) `--HasMemberwiseInitRequest(test.(file)[email protected]:1:8) -> 1 | `--InterfaceTypeRequest(test.(file)[email protected]:1:8) | | `--GenericParamListRequest(test.(file)[email protected]:1:8) | | `--GenericParamListRequest(test.(file)[email protected]:1:8) | `--HasUserDefinedDesignatedInitRequest(test.(file)[email protected]:1:8) -> 0 | | `--ParseMembersRequest(test.(file)[email protected]:1:8) -> {(unknown decl), test.(file)[email protected]:2:7} | `--ParseMembersRequest(test.(file)[email protected]:1:8) -> {(unknown decl), test.(file)[email protected]:2:7} | `--StorageImplInfoRequest(test.(file)[email protected]:2:7) | | `--AttachedPropertyWrappersRequest(test.(file)[email protected]:2:7) | `--StorageImplInfoRequest(test.(file)[email protected]:2:7) | | `--AttachedPropertyWrappersRequest(test.(file)[email protected]:2:7) | `--AccessLevelRequest(test.(file)[email protected]:2:7) `--SynthesizeMemberwiseInitRequest(test.(file)[email protected]:1:8) (cyclic dependency)

I see. It might be better then to check to explicitly check if an SynthesizeMemberwiseInitRequest is active, instead of using hasInterfaceType(), ie,

VD->getASTContext().evaluator.hasActiveRequest( SynthesizeMemberwiseInitRequest{...})

slavapestov · 2019-11-05T23:14:00Z

lib/Sema/LookupVisibleDecls.cpp

+          resolver->resolveTypeWitness(NormalConformance, ATD);
+      } else if (auto *VD = dyn_cast<ValueDecl>(Member)) {
+        if (!NormalConformance->hasWitness(VD))
+          resolver->resolveWitness(NormalConformance, VD);


Can we do this all this inside lookupDeclsFromProtocolsBeingConformedTo() instead?

I don't think so because this has to be done before lookupTypeMembers() which is called before lookupDeclsFromProtocolsBeingConformedTo().

slavapestov · 2019-11-05T23:14:29Z

lib/Sema/LookupVisibleDecls.cpp

+  };
+  synthesizePropertyWrappers(NTD);
+  for (auto ED : NTD->getExtensions())
+    synthesizePropertyWrappers(ED);


I might be cleaner to visit the extension's members in doGlobalExtensionLookup()

rintaro · 2019-11-06T21:08:19Z

@swift-ci Please smoke test

DougGregor

I don't have much to add to Slava and Ben's feedback, but I love the form of this. It's great to see us taking advantage of more laziness in the type checker.

slavapestov · 2019-11-12T03:20:14Z

lib/IDE/CodeCompletion.cpp

+        Conformance->getRootConformance()->forEachValueWitness(
+            [&](ValueDecl *req, Witness witness) {
+              if (witness.getDecl() == VD)
+                Access = std::max(Access, req->getFormalAccess());


I still think it's better to write Conformance->getProtocol()->getFormalAccess() here.

slavapestov · 2019-11-12T03:21:34Z

lib/IDE/CodeCompletion.cpp

+    if (D->isFinal())
+      return;
+
+    // A 'class' member with an initial value cannot be overriden either.


My point was that the presence of an initial value expression does not have any bearing on whether the declaration can be overridden or not. The comment above is incorrect.

I think the only relevant check here is whether the property is final (static is equivalent to class final).

slavapestov · 2019-11-12T03:23:23Z

lib/Sema/LookupVisibleDecls.cpp

+                                           const DeclContext *DC) {
+  // Synthesize the memberwise initializer for structs or default initializer
+  // for classes.
+  if (!NTD->hasInterfaceType())


I see. It might be better then to check to explicitly check if an SynthesizeMemberwiseInitRequest is active, instead of using hasInterfaceType(), ie,

VD->getASTContext().evaluator.hasActiveRequest( SynthesizeMemberwiseInitRequest{...})

- Default memberwise initializer for struct was not suggested - Dynamic member lookup didn't work - `init(rawValue:)` was not suggested for `RawRepresentable` types - '$name' was not suggested for `@propertyWrapper`ed value rdar://problem/56391233

… file - Use `performParseAndResolveImportsOnly()` to invoke the frontend - Do `bindExtensions()` in `ide::typeCheckContextUntil()` - Typecheck preceding `TopLevelCodeDecl`s only if the compleiton is in a `TopLevelCodeDecl` - Other related tweaks rdar://problem/56636747

rintaro · 2019-11-12T07:39:27Z

@swift-ci Please smoke test

In override completion, we didn't use to emit 'class var' with the initial expression. However, having initial expression does not have any bearing on whether the declaration is overridable or not.

rintaro · 2019-11-12T08:41:27Z

@swift-ci Please smoke test

rintaro force-pushed the ide-completion-rdar56636747 branch from 4ffb312 to 883cbd4 Compare October 31, 2019 00:30

rintaro commented Oct 31, 2019

View reviewed changes

lib/Sema/TypeCheckAttr.cpp Outdated Show resolved Hide resolved

rintaro requested review from slavapestov, benlangmuir and DougGregor October 31, 2019 00:45

rintaro force-pushed the ide-completion-rdar56636747 branch 2 times, most recently from f96a0cd to 9f9298e Compare October 31, 2019 06:14

rintaro force-pushed the ide-completion-rdar56636747 branch from 9f9298e to 0d6e0c8 Compare October 31, 2019 21:30

benlangmuir reviewed Oct 31, 2019

View reviewed changes

rintaro mentioned this pull request Nov 1, 2019

[CodeCompleiton] Avoid walking the whole module to find the delayed func #28014

Merged

rintaro force-pushed the ide-completion-rdar56636747 branch from 0d6e0c8 to fc2a4d6 Compare November 5, 2019 18:40

slavapestov reviewed Nov 5, 2019

View reviewed changes

rintaro force-pushed the ide-completion-rdar56636747 branch 2 times, most recently from 024b1f6 to 113ea63 Compare November 6, 2019 21:03

rintaro requested a review from slavapestov November 6, 2019 22:30

DougGregor reviewed Nov 7, 2019

View reviewed changes

slavapestov approved these changes Nov 12, 2019

View reviewed changes

rintaro added 2 commits November 12, 2019 12:57

rintaro added 4 commits November 12, 2019 12:57

Factor out synthesizePropertyWrapperStorageWrapperProperties()

32e68c7

Utilize forEachTypeWitness()/forEachValueWitness()

f6a76f5

Do 'bindExtensions()' as a part of 'performParseAndNameBindingOnly()'

d51d844

Workaround cyclic dependency issue in memberwise initializer synthesis

044790d

rintaro force-pushed the ide-completion-rdar56636747 branch from 113ea63 to 5c2b6a5 Compare November 12, 2019 07:38

[CodeCompletion] Remove unsound check in override completion

97b16ce

In override completion, we didn't use to emit 'class var' with the initial expression. However, having initial expression does not have any bearing on whether the declaration is overridable or not.

rintaro force-pushed the ide-completion-rdar56636747 branch from 5c2b6a5 to 97b16ce Compare November 12, 2019 08:40

rintaro merged commit 71c382e into swiftlang:master Nov 12, 2019

rintaro deleted the ide-completion-rdar56636747 branch November 12, 2019 12:43

rintaro mentioned this pull request Nov 13, 2019

[CodeCompletion] Avoid unnecessary performTypeChecking() in REPL #28228

Closed

[CodeCompletion] Avoid type checking all top-level decls in the primary file #27981

[CodeCompletion] Avoid type checking all top-level decls in the primary file #27981

Uh oh!

Conversation

rintaro commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro commented Oct 31, 2019

Uh oh!

rintaro commented Oct 31, 2019

Uh oh!

rintaro commented Oct 31, 2019

Uh oh!

swift-ci commented Oct 31, 2019

Uh oh!

rintaro commented Oct 31, 2019

Uh oh!

rintaro commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented Oct 31, 2019 •

edited

Loading

rintaro Oct 31, 2019 •

edited

Loading

rintaro Oct 31, 2019 •

edited

Loading

rintaro Nov 1, 2019 •

edited

Loading

rintaro Nov 5, 2019 •

edited

Loading