ASTScope: Redo 'selfDC' computation #33989
Conversation
slavapestov
force-pushed
the
astscope-self-dc-cleanup
branch
from
bcc7eb1 to
a00b5cd
Compare
…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() }
}
}
slavapestov
force-pushed
the
astscope-self-dc-cleanup
branch
2 times, most recently
from
6da6639 to
1a73364
Compare
davidungar
left a comment
davidungar
left a comment
There was a problem hiding this comment.
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.
lib/AST/Expr.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure I understand. The for loop references the newly-constructed CaptureListExpr instance.
There was a problem hiding this comment.
Sorry, my mistake. Thanks for pointing it out so gently.
There was a problem hiding this comment.
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.
include/swift/AST/Decl.h
Outdated
There was a problem hiding this comment.
Can Parent be a const or do VarDecls move from family to family?
include/swift/AST/Decl.h
Outdated
lib/AST/ASTScopeLookup.cpp
Outdated
lib/AST/ASTScopeLookup.cpp
Outdated
lib/AST/UnqualifiedLookup.cpp
Outdated
There was a problem hiding this comment.
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
force-pushed
the
astscope-self-dc-cleanup
branch
from
1a73364 to
e115b2f
Compare
|
@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'.
slavapestov
force-pushed
the
astscope-self-dc-cleanup
branch
from
e115b2f to
f4083ba
Compare
davidungar
left a comment
davidungar
left a comment
There was a problem hiding this comment.
The selfDC computation is better, but maybe in a future PR it could be even better.
lib/AST/UnqualifiedLookup.cpp
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
force-pushed
the
astscope-self-dc-cleanup
branch
from
f4083ba to
0712d0b
Compare
slavapestov
force-pushed
the
astscope-self-dc-cleanup
branch
from
0712d0b to
d4cc35a
Compare
|
swiftlang/llvm-project#1820 |
|
@swift-ci Please test source compatibility |
2 participants
This was motivated by getting
selfin capture lists working without parse-time name lookup.