Consistently treat let properties as immutable within the type checker
#73609
Conversation
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
| /// Expressions that are in the left-hand side of an assignment | ||
| /// expression. This is used to help distinguish assignments from | ||
| /// any other potential use that might need lvalues. | ||
| llvm::SmallPtrSet<Expr *, 2> TargetOfAssignExprs; |
There was a problem hiding this comment.
I think this needs to be controlled by the solver scope because result builder generate assignments as part of the transform which means that if not controlled this can grow pretty large.
There was a problem hiding this comment.
Since the constraint system records parent chains maybe this could be replaced with a walk up the parent chain until it reaches assignment expression?
There was a problem hiding this comment.
I missed the fact that we record parent chains; I'd much prefer that to having to keep solver state!
There was a problem hiding this comment.
I did this and it's so much better, thank you!
| }); | ||
| }, | ||
| llvm::function_ref<bool(Expr *)> isAssignTarget = [](Expr *) { | ||
| return true; |
There was a problem hiding this comment.
It's the default that keeps existing code working the same way as it has before. I could remove the default and force all callers to choose.
There was a problem hiding this comment.
Maybe this is going to be a non-issue if you go with walk approach since it could just be a method on a constraint system?
There was a problem hiding this comment.
I've cleaned this up and removed the default, thanks for the nudge.
lib/Sema/CSGen.cpp
Outdated
| // Find all of the entries on the left-hand side of an assignment | ||
| // expression. | ||
| if (auto assign = dyn_cast<AssignExpr>(expr)) { | ||
| recordAssignmentTargets(assign->getDest()); |
There was a problem hiding this comment.
This looks similar to what happens with argument labels, should probably be moved into ConstraintGenerator::visitAssignExpr itself.
There was a problem hiding this comment.
This code is gone now, since I'm using parent pointers
lib/Sema/CSGen.cpp
Outdated
| @@ -4307,6 +4313,49 @@ namespace { | |||
| return Action::Continue(expr); | |||
| } | |||
|
|
|||
| /// Walk the destination of an assignment expression, recording | |||
| /// each of the | |||
lib/Sema/ConstraintSystem.cpp
Outdated
| AbstractStorageDecl *storage, Type baseType, | ||
| DeclContext *useDC, | ||
| llvm::function_ref<bool(Expr *)> isAssignTarget = [](Expr *) { | ||
| return true; |
There was a problem hiding this comment.
Similar to getUnopenedTypeOfReference I'm not sure if that's always correct to default to true, what if argument is omitted by mistake?
There was a problem hiding this comment.
I've removed the defaults, thanks
lib/Sema/ConstraintSystem.cpp
Outdated
| if (!memberLocator) | ||
| break; | ||
|
|
||
| auto *anchor = getAsExpr(memberLocator->getAnchor()); |
There was a problem hiding this comment.
Should this me simplifyToAnchor instead?
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
…value This operation determines whether a particular storage declaration, when accessed from a particular location, is mutable or not. It has a particular semantic that `let` declarations, when accessed from an initializer, are considered mutable even though they can only be assigned. There is similar logic for init accessors. Tease apart "truly mutable" from "initializable because we're in an initializer", introducing AbstractStorageDecl::mutability() to represent all three states. isSettable() remains available as a thin shim over mutability() and all clients are unchanged thus far, making this a no-op refactoring.
Stored `let` properties of a struct, class, or actor permit 'inout' modification within the constructor body after they have been initialized. Tentatively remove this rule, only allowing such `let` properties to be initialized (assigned to) and not treated as `inout`. Fixes rdar://127258363.
DougGregor
force-pushed
the
instance-let-not-inout-in-initializer
branch
from
0786510 to
39f4e38
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
DougGregor
changed the title
let properties as immutable within the type checker
…nces as lvalues As we do with references to initializable local lets, teach the type checker to produce ASTs where member references to initializable instance property lets (e.g., within an initializer) treat the `let` as an lvalue and then immediately load. This modeling addresses a source compatibility issue uncovered in swift-syntax, where code that *technically* has a use-before-definition on a `let` property in an initializer wasn't diagnosed as such because the loaded value wasn't actually used for anything.
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
|
@swift-ci please clean smoke test macOS |
|
@swift-ci please clean test source compatibility |
|
@swift-ci please test source compatibility |
There are a number of places where references
letproperties are treated as lvalues by the type checker, and we rely on Definite Initialization to detect and diagnoseinoutuses of these properties. This includes:letinstance properties of a type within an initializer.letproperties that aren't initialized at point of declaration.The result of modeling these as lvalues in the type checker is that we could end up selecting a
mutatingfunction for aletbase, which would then get diagnosed in Definite Initialization. If there was an alternative non-mutatingoverload that is otherwise not preferred, it will essentially get ignored.Rework our handling of
letproperties so that they are treated as rvalues uniformly except when they are being assigned to. This moves the inout/mutating checking up into the type checker, allowing overload resolution to choose appropriately.Fixes rdar://127258363, a source compatibility issue with
AsyncIteratorProtocol.next()that uncovered this issue.