Fixes for expression type checking with parse-time name lookup disabled #34088

slavapestov · 2020-09-25T20:46:07Z

When parse-time lookup is enabled, the parser resolves DeclRefExprs that refer to local bindings. Any remaining UnresolvedDeclRefExprs were handled by preCheckExpression(). When parse-time lookup is disabled, preCheckExpression() is now responsible for resolving all UnresolvedDeclRefExprs itself.

This PR fixes two problems with that:

In a handful of places, the constraint solver would walk not-yet or partially type checked function and closure bodies, expecting to find DeclRefExprs for references to locals. This is perhaps a layering violation and better solutions would include either doing preCheckExpression() more eagerly, or finding a way to ensure these walks only happen after type checking has completed. The fix I went with was a suggestion by @DougGregor, which is to just perform the unqualified lookup right there when we see an UnresolvedDeclRefExpr. To help with this, I added a new ASTScope::lookupSingleLocalDecl() entry point that skips lookups into types and at the top level, and does not perform any shadowing checks which might trigger further type checking.
When resolving a reference to a ParamDecl, preCheckExpression() would trigger computation of ImplInfoRequest by computing the access kind. This would cache the wrong value and fail later if the parameter was later inferred as an inout by the constraint solver. Previously this problem was masked by parse-time lookup resolving all references to ParamDecls, so we never actually hit this case. My fix is to move the access kind computation to CSApply, since we always have full information there, and nothing in CSGen needs to know the access kind.

davidungar

Looks good, except for that one very long function which I bet isn't all your doing. I wouldn't object if you just landed this, but I'd rather that thing were chopped up first.

davidungar · 2020-09-25T20:50:34Z

include/swift/AST/Decl.h

@@ -6770,6 +6763,37 @@ enum class CtorInitializerKind {
  Factory
 };

+/// Specifies the kind of initialization call performed within the body
+/// of the constructor, e.g., self.init or super.init.
+enum class BodyInitKind {


Yes, good choice to use the enum class vs a straight enum.

davidungar · 2020-09-25T20:51:30Z

include/swift/AST/Decl.h

@@ -6770,6 +6763,37 @@ enum class CtorInitializerKind {
  Factory
 };

+/// Specifies the kind of initialization call performed within the body
+/// of the constructor, e.g., self.init or super.init.


Totally optional thought: It could nice to add a sentence like: "Needed to ... when ..."

davidungar · 2020-09-25T20:52:16Z

include/swift/AST/Decl.h

@@ -6818,38 +6842,11 @@ class ConstructorDecl : public AbstractFunctionDecl {

  ParamDecl **getImplicitSelfDeclStorage() { return &SelfDecl; }

-  /// Specifies the kind of initialization call performed within the body
-  /// of the constructor, e.g., self.init or super.init.
-  enum class BodyInitKind {


Oh, saw the insertion of this before the deletion. Didn't realize it wasn't a pure addition.

davidungar · 2020-09-25T20:55:36Z

lib/AST/UnqualifiedLookup.cpp

-#endif // NDEBUG
+namespace {
+
+class ASTScopeDeclConsumerForLocalLookup


I like the use of an otology here.

davidungar · 2020-09-25T20:57:33Z

lib/Sema/TypeCheckDecl.cpp

+    FindReferenceToInitializer(const ConstructorDecl *decl,
+                               ASTContext &ctx)
+        : Decl(decl), ctx(ctx) { }
+
+    bool walkToDeclPre(class Decl *D) override {
+      // Don't walk into further nominal decls.
+      return !isa<NominalTypeDecl>(D);
+    }
+
+    std::pair<bool, Expr*> walkToExprPre(Expr *E) override {
+      // Don't walk into closures.
+      if (isa<ClosureExpr>(E))
+        return { false, E };
+
+      // Look for calls of a constructor on self or super.
+      auto apply = dyn_cast<ApplyExpr>(E);
+      if (!apply)
+        return { true, E };
+
+      auto Callee = apply->getSemanticFn();
+
+      Expr *arg;
+
+      if (isa<OtherConstructorDeclRefExpr>(Callee)) {
+        arg = apply->getArg();
+      } else if (auto *CRE = dyn_cast<ConstructorRefCallExpr>(Callee)) {
+        arg = CRE->getArg();
+      } else if (auto *dotExpr = dyn_cast<UnresolvedDotExpr>(Callee)) {
+        if (dotExpr->getName().getBaseName() != DeclBaseName::createConstructor())
+          return { true, E };
+
+        arg = dotExpr->getBase();
+      } else {
+        // Not a constructor call.
+        return { true, E };
+      }
+
+      // Look for a base of 'self' or 'super'.
+      arg = arg->getSemanticsProvidingExpr();
+
+      BodyInitKind myKind;
+      if (arg->isSuperExpr())
+        myKind = BodyInitKind::Chained;
+      else if (arg->isSelfExprOf(Decl, /*sameBase*/true))
+        myKind = BodyInitKind::Delegating;
+      else if (auto *declRef = dyn_cast<UnresolvedDeclRefExpr>(arg)) {
+        // FIXME: We can see UnresolvedDeclRefExprs here because we have
+        // not yet run preCheckExpression() on the entire function body
+        // yet.
+        //
+        // We could consider pre-checking more eagerly.
+        auto name = declRef->getName();
+        auto loc = declRef->getLoc();
+        if (name.isSimpleName(ctx.Id_self)) {
+          auto *otherSelfDecl =
+            ASTScope::lookupSingleLocalDecl(Decl->getParentSourceFile(),
+                                            name.getFullName(), loc);
+          if (otherSelfDecl == Decl->getImplicitSelfDecl())
+            myKind = BodyInitKind::Delegating;
+        }
+      }
+      else {
+        // We're constructing something else.
+        return { true, E };
+      }
+
+      if (Kind == BodyInitKind::None) {
+        Kind = myKind;
+
+        InitExpr = apply;
+        return { true, E };
+      }
+
+      // If the kind changed, complain.
+      if (Kind != myKind) {
+        // The kind changed. Complain.
+        ctx.Diags.diagnose(E->getLoc(), diag::init_delegates_and_chains);
+        ctx.Diags.diagnose(InitExpr->getLoc(), diag::init_delegation_or_chain,
+                           Kind == BodyInitKind::Chained);
+      }
+
+      return { true, E };
+    }
+  };
+
+  auto &ctx = decl->getASTContext();
+  FindReferenceToInitializer finder(decl, ctx);
+  decl->getBody()->walk(finder);
+
+  // get the kind out of the finder.
+  auto Kind = finder.Kind;
+
+  auto *NTD = decl->getDeclContext()->getSelfNominalTypeDecl();
+
+  // Protocol extension and enum initializers are always delegating.
+  if (Kind == BodyInitKind::None) {
+    if (isa<ProtocolDecl>(NTD) || isa<EnumDecl>(NTD)) {
+      Kind = BodyInitKind::Delegating;
+    }
+  }


Really need a shorthand for "This-function-is-too-long-for-David-to-understand". I couldn't even select it all.

As you noted I just cut and paste the body of getDelegatingOrChainedInitKind() into BodyInitKindRequest::evaluate() here. Do you mind if I leave this as-is?

slavapestov · 2020-09-25T21:04:58Z

@swift-ci Please smoke test

xedin · 2020-09-25T21:15:49Z

lib/Sema/CSGen.cpp

+          // yet.
+          //
+          // We could consider pre-checking more eagerly.
+          if (auto *declRef = dyn_cast<UnresolvedDeclRefExpr>(expr)) {


Does this mean that this is a body of a multi-statement closure because preCheckExpression should walk into single-statement ones and resolve their refs? I think we can move closure->walk(collectVarRefs); under shouldTypeCheckInEnclosingExpression(closure) to avoid having to handle not-yet-unresolved references, because currently solver handles only single-statement closures...

Unfortunately test/ConstraintSystem/function_builders.swift ends up with a multi-statement closure here that has not been pre-checked, and if I skip it, the solver fails with unbound free variables.

I wonder if pre-check should walk to any closure body then, since it's impossible to say whether it's going to be used as a function builder body?

My concern is that since preCheckExpression() uses TypeChecker::lookupUnqualified(), it can end up calling getInterfaceType() on the closure’s parameters for example, so in general it’s not valid to pre-check the body before having applied a solution to the expression containing the multi-statement closure. Avoiding the possibility of getInterfaceType() calls on local parameters with inferred type is also why I implemented lookupSingleLocalDecl() separately.

Hm, could you elaborate on why getInterfaceType() on a parameter is a problem? Since preCheckExpression currently walks into single-statement closures it could find nested closures which wouldn't have any information in the AST from their parents either...

I might be misunderstanding but it's concerning because this problem with getInterfaceType() on parameter sounds like it would affect any kind of closure...

Today, parameter references are resolved earlier in the parser. The unqualified lookup in preCheckExpression() only finds type members and globals, and forward references of local functions.

In general the type checker’s lookup also won’t call getInterfaceType() unless it finds multiple decls and has to do shadowing checks, but that’s not guaranteed.

So what does this mean for single-statement closures which don't have their type resolved yet but share parameter names with some other declarations from outer context?

var x: Int = 0 let _: (Int) -> Void = { x in _ = { x, y in x + y }(x, 42) }

here either parameter x would be resolved until solution is applied to the AST.

It feels like what we really want to do here is instead of mutating AST generate constraints for UnresolvedDeclRefExpr directly and that way delay lookup until solver just like we do for members today.

… request This method had a messy contract: - Setting the diags parameter to nullptr inhibited caching - The initExpr out parameter could only used if no result had yet been cached Let's instead use the request evaluator here.

This is used in a few places that used to expect parsed but not yet type-checked code to contain DeclRefExprs that reference local bindings. Instead, we can call lookupSingleLocalDecl() with an UnresolvedDeclRefExpr instead.

…ailure

… pass When parse-time lookup is disabled, we have to resolve UnresolvedDeclRefExprs inside the closure body, since there's no guarantee that preCheckExpression() has run yet.

It's too early to do that here, because we may be building a reference to a ParamDecl that is not yet known to be inout, because the constraint solver has not run yet. Instead, always compute the access semantics in CSApply.

slavapestov · 2020-09-25T22:05:37Z

@swift-ci Please smoke test

slavapestov · 2020-09-25T23:27:30Z

@swift-ci Please test source compatibility

slavapestov requested review from xedin, hborla and davidungar September 25, 2020 20:47

davidungar reviewed Sep 25, 2020

View reviewed changes

xedin reviewed Sep 25, 2020

View reviewed changes

slavapestov added 6 commits September 25, 2020 17:59

ASTScope: Add a new lookupSingleLocalDecl() entry point

6b98f8f

This is used in a few places that used to expect parsed but not yet type-checked code to contain DeclRefExprs that reference local bindings. Instead, we can call lookupSingleLocalDecl() with an UnresolvedDeclRefExpr instead.

Sema: Use ASTScope::lookupSingleLocalDecl() in MissingOptionalUnwrapF…

dcafd58

…ailure

Sema: Use ASTScope::lookupSingleLocalDecl() in CSGen's CollectVarRefs…

8ec878b

… pass When parse-time lookup is disabled, we have to resolve UnresolvedDeclRefExprs inside the closure body, since there's no guarantee that preCheckExpression() has run yet.

Sema: Use ASTScope::lookupSingleLocalDecl() in BodyInitKindRequest

ddc0cbd

slavapestov force-pushed the constraint-solver-fixes-for-astscope branch from c6dc676 to 6a82f24 Compare September 25, 2020 22:02

slavapestov merged commit f1c0a49 into swiftlang:main Sep 26, 2020

Fixes for expression type checking with parse-time name lookup disabled #34088

Fixes for expression type checking with parse-time name lookup disabled #34088

Uh oh!

Conversation

slavapestov commented Sep 25, 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!

slavapestov commented Sep 25, 2020

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!

slavapestov commented Sep 25, 2020

Uh oh!

slavapestov commented Sep 25, 2020

Uh oh!

Uh oh!

slavapestov commented Sep 25, 2020 •

edited

Loading