fix(compiler): mutable lifted object may be modified inflight #6258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for opening this pull request! 🎉
|
|
Console preview environment is available at https://wing-console-pr-6258.fly.dev 🚀 Last Updated (UTC) 2024-04-17 20:38 |
BenchmarksComparison to Baseline ⬜⬜⬜⬜⬜⬜⬜⬜⬜🟥⬜⬜⬜
⬜ Within 1.5 standard deviations Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI. Results
Last Updated (UTC) 2024-04-17 20:43 |
yoav-steinberg
changed the title
Signed-off-by: monada-bot[bot] <[email protected]>
monadabot
added
the
⚠️ pr/review-mutation
yoav-steinberg
added
🚧 pr/do-not-merge
…ng/wing into yoav/unmut_lifted_objects
yoav-steinberg
removed
the
🚧 pr/do-not-merge
Signed-off-by: monada-bot[bot] <[email protected]>
monadabot
added
the
⚠️ pr/review-mutation
yoav-steinberg
removed
the
⚠️ pr/review-mutation
eladb
reviewed
Apr 17, 2024
monadabot
added
the
⚠️ pr/review-mutation
yoav-steinberg
removed
the
⚠️ pr/review-mutation
|
Thanks for contributing, @yoav-steinberg! This PR will now be added to the merge queue, or immediately merged if |
|
Congrats! 🚀 This was released in Wing 0.70.26. |
mergify bot
pushed a commit
that referenced
this pull request
Apr 17, 2024
Reverts #6258 broke some winglibs due to strange behavior trying to call `copyMut` on immutable maps during inflight. See below <img width="363" alt="image" src="https://github.com/winglang/wing/assets/45375125/89f09ba9-de35-4a01-b039-9330a2c3a586">
5 tasks
mergify bot
pushed a commit
that referenced
this pull request
Apr 24, 2024
…tempt (#6303) Fixes #3069 (2nd attempt, instead of #6258) The type checker will now modify the type of preflight expressions that are being used inflight to a non-mutable version of the same type: MutArray -> Array, MutJson -> Json... Note that since we change the type of the expression we don't get descriptive error telling us that we're trying to mutate a lifted mutable object, instead we just get the same error we'd expect if the lifted object was non-mutable to begin with. This isn't ideal but it makes the fix simpler. Let me know what you think... To get this working we needed to do the following changes to how we evaluate the phases of reference expressions: * The phase of phase independent properties inflight will be inflight. This makes sure inflight calls like `preflight_array.copyMut()` will be we evaluated inflight so the side effect of turning the result to immutable won't happen. This is a major change to lifting because inflight calls like `preflight_array.at(0)` will also be executed inflight (no matter what args are passed to the call). * There's a special exception to the above, struct fields: we need to keep them phase independent and not turn them to inflight when accessing them inflight. This is required so we can lift qualify methods called on struct field when those fields are preflight objects. This needs to be addressed in the future, because somehow struct fields being "phase independent" doesn't seem correct. * Index access (`[i]`) phase calculation was wrong and didn't take into account the phase of the index. When the index phase is inflight the expression must be inflight too. When the index phase is independent and the instance is preflight we want to lift the entire expression and not only the instance so it can be correctly lift-qualified. * All this means we don't need the (admittedly beautiful) logic of only turning phase independent method calls to inflight calls when they are passed some inflight state (instance or arg). Now we always treat them as inflight. This cleaned up some code. ## Checklist - [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted) - [x] Description explains motivation and solution - [x] Tests added (always) - [x] Docs updated (only required for features) - [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #3069
The type checker will now modify the type of preflight expressions that are being used inflight to a non-mutable version of the same type:
MutArray->Array,MutJson->Json...Note that since we change the type of the expression we don't get descriptive error telling us that we're trying to mutate a lifted mutable object, instead we just get the same error we'd expect if the lifted object was non-mutable to begin with. This isn't ideal but it makes the fix simpler. Let me know what you think...
Checklist
pr/e2e-fulllabel if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.