Issue with computing closure captures in diagnostic function for implicit self use

(Frederick Kellison-Linn) #1

I'm poking around to start implementing a fix for SR-10218, but have run into an issue that makes me feel like I'm doing something wrong. At the time when diagnoseImplicitSelfUseInClosure is called, it looks like the closure has not actually had its CaptureInfo computed yet. The DeclContext * provided as a parameter is const, so the traversal of the parent DeclContexts yields only const AbstractClosureExpr *s, which can't be provided to TypeChecker::computeCaptures().

Is there a better place to call computeCaptures() that will ensure we have the correct capture info by the time we're checking for implicit self use, or a different way that I should be approaching this issue? Any advice is much appreciated!

(Jordan Rose) #2

Oops, I didn't think that part through. That said, though, I don't think you need the proper CaptureInfo, since you only care what's written explicitly in the CaptureListExpr. It doesn't look like there's a way to get from the ClosureExpr, which is a little inconvenient, but maybe we can pass it down too?

(Frederick Kellison-Linn) #3

Hmm... where would be the best place to pass it down from? diagnoseImplicitSelfUseInClosure is ultimately called from TypeChecker::typeCheckExpressionImpl, and we find the ClosureExprs by moving back up through DeclContext::getParent() until we reach a local context.

(Jordan Rose) #4

Heh, I saw this a little after you posted it and then couldn't come up with an answer. Some ideas, none of which I love:

  • Add a (nullable) captureList pointer to ClosureExpr. (Easy, makes AST node a little bigger but maybe no one cares?)
  • Figure out how implicit self works and make it do name lookup to find the appropriate self declaration. (We know name lookup can find captures because capture lists can introduce new names with the [y = x + 1] syntax.)
  • Build some kind of "parent map" like the one in Expr::getParentMap. (Expensive for something we're only going to use once, though.)
  • Propagate this knowledge through typeCheckExpression from typeCheckClosureBody (and into typeCheckClosureBody somehow).

I'm leaning towards the first option, but maybe there's a reason why we haven't done it before. @Slava_Pestov, @Joe_Groff, @Douglas_Gregor, do you know?

(Frederick Kellison-Linn) #5

Darn, was hoping there was a simpler API I was missing that would let this fall into place. I’ll wait for feedback from the others regarding the best path forward before I proceed!

(Frederick Kellison-Linn) #6

For reference, the current architecture (where the capture list is a parent of the closure expression) is discussed here, and the PR that introduced that change is linked for further context.

(Jordan Rose) #7

Huh. I wasn't questioning the nesting in this case, just suggesting a back-edge in the AST graph. That seems like a smaller change.

1 Like
(Slava Pestov) #8

I'm not opposed to the first one in practice. Perhaps we could simply merge CaptureListExpr with ClosureExpr.

However another, potentially simpler approach you could take would be to move the implicit self diagnosis to happen after captures have been computed.

1 Like
(Jordan Rose) #9

The implicit self diagnosis happens when type-checking individual expressions; capture diagnostics necessarily happen after that. We'd have to walk the AST again. I guess that's not the end of the world, but it's another thing that tips me towards just having a back-edge (or doing the merge).

(Frederick Kellison-Linn) #10

Would the merge be meaningfully distinct from reverting Chris’s commit in the linked thread? Maybe I’m misunderstanding but it seems like the lay of the land before that was that ClosureExpr and CaptureListExpr were merged.

(Jordan Rose) #11

I doubt it would revert cleanly this many years later. But as John said in that thread:

Well that certainly doesn't seem like a good enough reason to complicate the expression.

That is, splitting them up made it easier to fix a bug at the time, but like any decision it's a tradeoff. Merging them back together would be work, but I wouldn't assume that we'd immediately run into the same bug and decide it was a mistake. It might be more than you want to take on, though.

(Frederick Kellison-Linn) #12

Of course, just wanted to make sure I was understanding correctly that a “merge” of the two would entail essentially returning to that structure at a high level. I’ll explore that direction and try to see how much effort it would entail.

(Frederick Kellison-Linn) #13

So once the capture information makes it to diagnoseImplicitSelfUseInClosure, we have to decide which conditions will allow self to be used implicitly. IMO it should only be valid when the capture list contains self as a bare identifier, disallowing captures like self = x and x = self from enabling implicit self in the closures. A couple questions about how best to implement this:

  1. Do you think a capture of self = self should enable implicit self in the closure body?
  2. How should this check be implemented? If the answer to the above question is "no," it seems like the only option is to add a field to CaptureListEntry (something like IsSimpleCapture) which we set at parse time, since after parsing we lose the distinction between [x] and [x = x]. If we don't care about that distinction, then the other easy option is check that the CaptureListEntry.Init consists of a single pattern, which is a single DeclRefExpr for which getDecl().isSelfParameter() == true, and also that CaptureListEntry.Var->getName() is the same as the name of the DeclRefExpr (could also compare to "self" directly, I'm not sure what the idiom is here.

If there's other good options I'm not seeing I'd love to know.

(Nobody1707) #14

I think it would be odd if [self = self] didn't enable implicit self.

(Jordan Rose) #15

I agree that self = self isn't technically a problem, and also probably not worth optimizing for. Your #2 strategy sounds pretty straightforward.

(I can think of one way to recover the distinction between [x] and [x = x]: if the PatternBindingDecl's EqualLoc is implicit but the NameLoc is explicit, or if the VarDecl's location is the same as the Initializer's location. But that's pretty subtle, and doesn't buy us anything in practice.)

1 Like
(Frederick Kellison-Linn) #16

As it happens, it looks like we will have to mess with the name lookup regardless since otherwise the self capture gets marked as unused. It looks like it would be somewhat cumbersome to integrate this into UnqualifiedLookup due to the nature of how the lookup results are handled.

I'm wondering if it seems to hacky to check for this case (that is, we are in a DeclContext which is a ClosureExpr and the base decl is self) explicitly in resolveDeclRefExpr, and modify the base decl and base decl context if we find an explicitly captured self somewhere up the context chain to the DeclContext for the actual self param.

(Slava Pestov) #17

Instead you ensure that the VarDecl we synthesize for the self capture in a capture points back to the original self declaration somehow.

Do vars in a capture list have a PatternBindingDecl?

(Frederick Kellison-Linn) #18

Oh, yeah, the we can find the proper self decl at parse time, and it goes in the capture list as a pair of VarDecl and pattern binding decl. But the actual implict-self member reference isn’t resolved until type checking, and when UnqualifiedLookup doesn’t find the actual name until we’re in reaches lookupNamesIntroducedByMemberFunction and uses a ResultFinderForTypeContext. Given how the result finder is set up, it seems like it would be difficult to recover the proper dynamicContext and staticContext when we may be nested inside arbitrarily many contexts while in lookupNamesIntroducedByClosure. I might be misunderstanding how the member function lookup works, though, I just glanced it over. Will give it another look tomorrow.

(Frederick Kellison-Linn) #19

@Slava_Pestov reread your comment today and something clicked—wasn't thinking clearly last night. Thank you!

I have an implementation working which has UnqualifiedLookup::lookForNamesIntroducedByClosure look through the PatternBindingDecl to get the proper context to supply to ResultFinderForTypeContext. With the lookup happening this way, we don't actually need to make any changes to diagnoseImplicitSelfUseInClosure since the VarDecl for the self capture has isSelfParameter() == false, so isImplicitSelfUse doesn't catch it (will probably rename that function to isImplicitSelfParamUse or something for correctness).

Thank you both (@jrose) for all your help! Will clean things up and put up the implementation in the next couple days.