ASTScope: Simplify representation of closures #33981
Conversation
Let's use a ClosureParametersScope for all closures, even those without an 'in' keyword. This eliminates the need for the ClosureBodyScope and WholeClosureScope. Also, let's move the lookup of capture list bindings from CaptureParametersScope to CaptureListScope. This eliminates the need for CaptureParametersScope to store a reference to the capture list, which allows us to remove the AbstractClosureScope base class entirely.
slavapestov
force-pushed
the
simplify-astscope-closure-representation
branch
from
45f6cd2 to
3801d16
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
Just from reading the PR description, it sounds like maybe it would be good to split this up into 2 PRs, one for each paragraph of the description. |
| if (auto *closure = dyn_cast<ClosureExpr>(E)) { | ||
| scopeCreator | ||
| .ifUniqueConstructExpandAndInsert<ClosureParametersScope>( | ||
| parent, closure); | ||
| return {false, E}; |
There was a problem hiding this comment.
Before, the code separated out finding the closures from constructing the scopes for them. But here, they are combined. Now, the class name on line 390 isn't quite right. But more to the point, I don't think it serves clarity to combine the enumeration (i.e. finding) with the handling. I would prefer the forEachClosureIn form. When I look at lines 386-390 in the old version, I can see what's going on for each closure without reading through all the code in ClosureFinder.
There was a problem hiding this comment.
I’d have to pass in two lambdas, one to handle capture lists and one to handle closure expressions. It didn’t seem worth it at that point.
There was a problem hiding this comment.
My quick read didn't catch that. Thanks! So that class would have to be named BuildScopesForContainedClosuresAndCaptureLists. And that name still has a bit of a bad smell to my nose. Maybe passing in two lambdas would be better.
|
Default argument values can’t see other parameters, and generic parameters would get their own scope that begins at the generic parameter list and not after the “in”. |
Generic constraints can see prior generic parameters, at least in some cases, so maybe someday default argument values will be able to see prior arguments. (but maybe not) I WRT generic parameters, in the old scheme, the generic param scope matrushka could have gone in the "whole closure" scope (assuming closures could be generic). I guess what I'm thinking is that I wish closures were more like functions. But I can't say that my hallucinations are enough to object too much to this simplification. |
|
Let’s have the scope tree reflect the language we have, not the language we think we want or might have in the future :-) |
Yes, there's value in minimalism. I also find value in symmetry. Not always obvious to me which to favor. |
Let's use a ClosureParametersScope for all closures, even those
without an 'in' keyword. This eliminates the need for the
ClosureBodyScope and WholeClosureScope.
Also, let's move the lookup of capture list bindings from
CaptureParametersScope to CaptureListScope. This eliminates the
need for CaptureParametersScope to store a reference to the
capture list, which allows us to remove the AbstractClosureScope
base class entirely.