ASTScope: Redo 'selfDC' computation #33989

slavapestov · 2020-09-18T06:55:01Z

This was motivated by getting self in capture lists working without parse-time name lookup.

…Expr We'll need this to get the right 'selfDC' when name lookup finds a 'self' declaration in a capture list, eg class C { func bar() {} func foo() { _ = { [self] in bar() } } }

davidungar

I love this direction. Didn't look deeply enough to assure correctness, but very nice to shift the complexity as this is doing. It wouldn't be a review from me without a request to break up a long function, where I think clarity could be improved.

davidungar · 2020-09-18T16:34:27Z

lib/AST/Expr.cpp

+  auto *expr = ::new(mem) CaptureListExpr(captureList, closureBody);
+
+  for (auto capture : captureList)
+    capture.Var->setParentCaptureList(expr);
+
+  return expr;


I would prefer putting the for loop before the CaptureListExpr constructor to compute the nullable CaptureListExpr, then passing it in to the constructor. That way the instance variable could be a const and it would be a bit clearer when reading the code that it is immutable.

I'm not sure I understand. The for loop references the newly-constructed CaptureListExpr instance.

Sorry, my mistake. Thanks for pointing it out so gently.

I guess the real issue is not knowing where the var is captured when the var is created. Would be great if the var could be immutable somehow.

davidungar · 2020-09-18T16:36:00Z

include/swift/AST/Decl.h

+  /// var decl.
+  void setParentCaptureList(CaptureListExpr *expr) {
+    assert(expr != nullptr);
+    Parent = expr;


Can Parent be a const or do VarDecls move from family to family?

davidungar · 2020-09-18T16:36:34Z

include/swift/AST/Decl.h

    /// Encodes whether this is a 'let' binding.
    Introducer : 1,

-    /// Whether this declaration was an element of a capture list.
-    IsCaptureList : 1,
-
    /// Whether this declaration captures the 'self' param under the same name.


Nice clean up.

davidungar · 2020-09-18T16:37:41Z

lib/AST/ASTScopeLookup.cpp

-#pragma mark computeSelfDC
-
-NullablePtr<DeclContext>
-GenericTypeOrExtensionWhereOrBodyPortion::computeSelfDC(
-    ArrayRef<const ASTScopeImpl *> history) {
-  ASTScopeAssert(history.size() != 0, "includes current scope");
-  size_t i = history.size() - 1; // skip last entry (this scope)
-  while (i != 0) {
-    Optional<NullablePtr<DeclContext>> maybeSelfDC =
-        history[--i]->computeSelfDCForParent();
-    if (maybeSelfDC) {
-      // If we've found a selfDC, we'll definitely be returning something.
-      // However, we may have captured 'self' somewhere down the tree, so we
-      // can't return outright without checking the nested scopes.
-      NullablePtr<DeclContext> nestedCapturedSelfDC =
-          checkNestedScopesForSelfCapture(history, i);
-      return nestedCapturedSelfDC ? nestedCapturedSelfDC : *maybeSelfDC;
-    }
-  }
-  return nullptr;
-}
-
-#pragma mark checkNestedScopesForSelfCapture
-
-NullablePtr<DeclContext>
-GenericTypeOrExtensionWhereOrBodyPortion::checkNestedScopesForSelfCapture(
-    ArrayRef<const ASTScopeImpl *> history, size_t start) {
-  NullablePtr<DeclContext> innerCapturedSelfDC;
-  // Start with the next scope down the tree.
-  size_t j = start;
-
-  // Note: even though having this loop nested inside the while loop from
-  // GenericTypeOrExtensionWhereOrBodyPortion::computeSelfDC may appear to
-  // result in quadratic blowup, complexity actually remains linear with respect
-  // to the size of history. This relies on the fact that
-  // GenericTypeOrExtensionScope::computeSelfDCForParent returns a null pointer,
-  // which will cause this method to bail out of the search early. Thus, this
-  // method is called once per type body in the lookup history, and will not
-  // end up re-checking the bodies of nested types that have already been
-  // covered by earlier calls, so the total impact of this method across all
-  // calls in a single lookup is O(n).
-  while (j != 0) {
-      auto *entry = history[--j];
-    Optional<NullablePtr<DeclContext>> selfDCForParent =
-      entry->computeSelfDCForParent();
-
-    // If we encounter a scope that should cause us to forget the self
-    // context (such as a nested type), bail out and use whatever the
-    // the last inner captured context was.
-    if (selfDCForParent && (*selfDCForParent).isNull())
-      break;
-
-    // Otherwise, if we have a captured self context for this scope, then
-    // remember it since it is now the innermost scope we have encountered.
-    NullablePtr<DeclContext> capturedSelfDC = entry->capturedSelfDC();
-    if (!capturedSelfDC.isNull())
-      innerCapturedSelfDC = entry->capturedSelfDC();
-
-    // Continue searching in the next scope down.
-  }
-  return innerCapturedSelfDC;
-}
-


davidungar · 2020-09-18T16:37:54Z

lib/AST/ASTScopeLookup.cpp

-#pragma mark computeSelfDCForParent
-
-// If the lookup depends on implicit self, selfDC is its context.
-// (Names in extensions never depend on self.)
-// Lookup can propagate it up from, say a method to the enclosing type body.
-
-// By default, propagate the selfDC up to a NomExt decl, body,
-// or where clause
-Optional<NullablePtr<DeclContext>>
-ASTScopeImpl::computeSelfDCForParent() const {
-  return None;
-}
-
-// Forget the "self" declaration:
-Optional<NullablePtr<DeclContext>>
-GenericTypeOrExtensionScope::computeSelfDCForParent() const {
-  return NullablePtr<DeclContext>();
-}
-
-Optional<NullablePtr<DeclContext>>
-PatternEntryInitializerScope::computeSelfDCForParent() const {
-  // Pattern binding initializers are only interesting insofar as they
-  // affect lookup in an enclosing nominal type or extension thereof.
-  if (auto *ic = getPatternEntry().getInitContext()) {
-    if (auto *bindingInit = dyn_cast<PatternBindingInitializer>(ic)) {
-      // Lazy variable initializer contexts have a 'self' parameter for
-      // instance member lookup.
-      if (bindingInit->getImplicitSelfDecl()) {
-        return NullablePtr<DeclContext>(bindingInit);
-      }
-    }
-  }
-  return None;
-}
-
-Optional<NullablePtr<DeclContext>>
-MethodBodyScope::computeSelfDCForParent() const {
-  return NullablePtr<DeclContext>(decl);
-}
-
-#pragma mark capturedSelfDC
-
-// Closures may explicitly capture the self param, in which case the lookup
-// should use the closure as the context for implicit self lookups.
-
-// By default, there is no such context to return.
-NullablePtr<DeclContext> ASTScopeImpl::capturedSelfDC() const {
-  return NullablePtr<DeclContext>();
-}
-
-// Closures track this information explicitly.
-NullablePtr<DeclContext> ClosureParametersScope::capturedSelfDC() const {
-  if (closureExpr->capturesSelfEnablingImplictSelf())
-    return NullablePtr<DeclContext>(closureExpr);
-  return NullablePtr<DeclContext>();
-}
-


lib/AST/UnqualifiedLookup.cpp

davidungar · 2020-09-18T16:46:48Z

lib/AST/UnqualifiedLookup.cpp

+
+  // We're looking for members of a type.
+  //
+  // If we started the looking from inside a scope where a 'self' parameter
+  // is visible, instance members are returned with the 'self' parameter's
+  // DeclContext as the base, which is how the expression checker knows to
+  // convert the unqualified reference into a self member access.
  auto resultFinder = UnqualifiedLookupFactory::ResultFinderForTypeContext(
-      &factory, selfDC ? selfDC.get() : scopeDC, scopeDC);
+      &factory, candidateSelfDC ? candidateSelfDC : scopeDC, scopeDC);
  const bool isCascadingUse =
      calculateIsCascadingUse(factory.getInitialIsCascadingUse());
  factory.findResultsAndSaveUnavailables(scopeDC, std::move(resultFinder),
                                         isCascadingUse, factory.baseNLOptions);
  factory.recordCompletionOfAScope();
+
+  // We're done looking inside a nominal type declaration. It is possible
+  // that this nominal type is nested inside of another type, in which case
+  // we will visit the outer type next. Make sure to clear out the known
+  // 'self' parameeter context, since any members of the outer type are
+  // not accessed via the innermost 'self' parameter.
+  candidateSelfDC = nullptr;


The new comments are great!

…ut parse-time lookup The DeclRefExpr here was resolved by parse-time lookup. Without it, it's an UnresolvedDeclRefExpr.

slavapestov · 2020-09-18T17:36:37Z

@swift-ci Please smoke test

This centralizes some invariants around the 'self' parameter. While all ConstructorDecls and DestructorDecls have a 'self', even if they're invalid because they're not nested inside a type, we don't want to consider this as the base 'self' for lookups. Eg, consider this invalid code: class C { func f() { init() { x } } } The base for the lookup should be f.self, not f.init.self.

…fiedLookup Instead of having ASTScope compute 'selfDC', the lookup consumer can compute it on its own, by looking for bindings named 'self'.

davidungar

The selfDC computation is better, but maybe in a future PR it could be even better.

davidungar · 2020-09-18T19:11:01Z

lib/AST/UnqualifiedLookup.cpp

+    // references. This is used by lookInMembers().
+    if (auto *var = dyn_cast<VarDecl>(value)) {
+      if (var->getName() == factory.Ctx.Id_self) {
+        maybeUpdateSelfDC(var);


Now that you've refactored--thanks!--I can see more clearly what is going on with candidateSelfDC. Is it a fragile piece of mutable state? Could we pass a piece of immutable state through the lookup recursion instead? It seems to be a bit odd that a consume method has a side effect. What do you think?

I was actually going to try to refactor the lookup to use a loop that walks the parents instead of recursing. This will allow us to implement the one-level lookup we need to do re-declaration checking for locals defined in the same scope. Maybe then the selfDC computation can go there and be totally explicit, instead of being part of consume().

slavapestov · 2020-09-18T23:02:36Z

swiftlang/llvm-project#1820
@swift-ci Please smoke test

slavapestov · 2020-09-18T23:02:58Z

@swift-ci Please test source compatibility

slavapestov force-pushed the astscope-self-dc-cleanup branch from bcc7eb1 to a00b5cd Compare September 18, 2020 06:58

slavapestov added 2 commits September 18, 2020 02:59

AST: Capture list bindings now point back to their parent CaptureList…

d7f4b1a

…Expr We'll need this to get the right 'selfDC' when name lookup finds a 'self' declaration in a capture list, eg class C { func bar() {} func foo() { _ = { [self] in bar() } } }

ASTScope: Remove unused shouldCreateAccessorScope() method

38883ce

slavapestov force-pushed the astscope-self-dc-cleanup branch 2 times, most recently from 6da6639 to 1a73364 Compare September 18, 2020 07:18

slavapestov mentioned this pull request Sep 18, 2020

[Swift] Update for VarDecl constructor change swiftlang/llvm-project#1820

Merged

slavapestov requested a review from davidungar September 18, 2020 07:25

davidungar approved these changes Sep 18, 2020

View reviewed changes

AST: Generalize CaptureListEntry::isSimpleSelfCapture() to work witho…

9c4c959

…ut parse-time lookup The DeclRefExpr here was resolved by parse-time lookup. Without it, it's an UnresolvedDeclRefExpr.

slavapestov force-pushed the astscope-self-dc-cleanup branch from 1a73364 to e115b2f Compare September 18, 2020 17:35

slavapestov added 2 commits September 18, 2020 15:05

ASTScope: Compute 'selfDC' directly in ASTScopeDeclConsumerForUnquali…

ed17c33

…fiedLookup Instead of having ASTScope compute 'selfDC', the lookup consumer can compute it on its own, by looking for bindings named 'self'.

slavapestov force-pushed the astscope-self-dc-cleanup branch from e115b2f to f4083ba Compare September 18, 2020 19:06

davidungar approved these changes Sep 18, 2020

View reviewed changes

slavapestov force-pushed the astscope-self-dc-cleanup branch from f4083ba to 0712d0b Compare September 18, 2020 19:48

slavapestov added 2 commits September 18, 2020 16:11

ASTScope: Remove old implementation of 'selfDC' computation

7cb809b

AST: Remove VarDecl::hasNonPatternBindingInit()

d4cc35a

slavapestov force-pushed the astscope-self-dc-cleanup branch from 0712d0b to d4cc35a Compare September 18, 2020 20:11

slavapestov merged commit 1e0a9ca into swiftlang:master Sep 19, 2020

ASTScope: Redo 'selfDC' computation #33989

ASTScope: Redo 'selfDC' computation #33989

Uh oh!

Conversation

slavapestov commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Sep 18, 2020

Uh oh!

davidungar 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!

slavapestov commented Sep 18, 2020

Uh oh!

slavapestov commented Sep 18, 2020

Uh oh!

Uh oh!

slavapestov commented Sep 18, 2020 •

edited

Loading