Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't mutate lhs while destructuring #40737

Merged
merged 1 commit into from
May 28, 2021
Merged

Conversation

simeonschaub
Copy link
Member

@JeffBezanson was a bit sceptical about this in #40574, but I think this
is worth doing since it seems strictly more useful than the current
behavior and we already go out of our way in cases like x, y = y, x to
only assign to the lhs after the rhs has been evaluated, so I think
handling x[2], x[1] = x in a similar way would only be consisten. As a
general rule that assignment will always happen after destructuring does
not seem much less obvious than the current behavior to me. This might
technically be slightly breaking, but I would be surprised if people
relied on the current behavior here. Nevertheless, it would probably
good to run PkgEval if we decide to go with this change.

closes #40574

@simeonschaub simeonschaub added compiler:lowering Syntax lowering (compiler front end, 2nd stage) needs news A NEWS entry is required for this change triage This should be discussed on a triage call needs pkgeval Tests for all registered packages should be run with this change labels May 6, 2021
@JeffBezanson
Copy link
Member

Triage thinks this is the right thing to do.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label May 6, 2021
@simeonschaub simeonschaub force-pushed the sds/destructure_aliasing branch from 4d07e2f to bcccabf Compare May 6, 2021 21:14
simeonschaub added a commit that referenced this pull request May 6, 2021
Discovered while working on #40737. Currently, this throws an
unintuitive error:
```julia
julia> a, f()... = 1, 2, 3
ERROR: syntax: ssavalue with no def
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1
```

Might as well fix this properly - Why not allow function definitions to
slurp a little from time to time? ;)
@simeonschaub
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@simeonschaub simeonschaub changed the title RFC: don't mutate lhs while destructuring don't mutate lhs while destructuring May 7, 2021
@simeonschaub simeonschaub removed needs news A NEWS entry is required for this change needs pkgeval Tests for all registered packages should be run with this change labels May 7, 2021
@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@simeonschaub
Copy link
Member Author

@nanosoldier runtests(["AbstractMCMC", "Circuitscape", "DiffEqUncertainty", "EliminateGraphs", "Expect", "ExtensibleUnions", "Graph500", "KrigingEstimators", "Manifolds", "MixedModelsExtras", "MixedModelsSim", "NumericalAlgorithms", "PLCTag", "ProgressMeterLogging", "Reactive", "SpatialJackknife"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@StefanKarpinski
Copy link
Member

StefanKarpinski commented May 7, 2021

This does seem strictly better. I'm pretty sure we can sweep any breakage under the rug of undefined behavior.

@StefanKarpinski StefanKarpinski added the minor change Marginal behavior change acceptable for a minor release label May 7, 2021
@simeonschaub simeonschaub requested a review from JeffBezanson May 19, 2021 14:41
@simeonschaub simeonschaub added this to the 1.7 milestone May 26, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label May 27, 2021
@simeonschaub simeonschaub force-pushed the sds/destructure_aliasing branch from dbe795d to 560dcc7 Compare May 27, 2021 22:06
@simeonschaub simeonschaub reopened this May 27, 2021
Jeff was a bit sceptical about this in #40574, but I think this
is worth doing since it seems strictly more useful than the current
behavior and we already go out of our way in cases like `x, y = y, x` to
only assign to the lhs after the rhs has been evaluated, so I think
handling `x[2], x[1] = x` in a similar way would only be consistent. As
a general rule that assignment will always happen after destructuring
does not seem much less obvious than the current behavior to me. This
might technically be slightly breaking, but I would be surprised if
people relied on the current behavior here. Nevertheless, it would
probably be good to run PkgEval if we decide to go with this change.

closes #40574
@simeonschaub simeonschaub force-pushed the sds/destructure_aliasing branch from 560dcc7 to 030bf53 Compare May 27, 2021 22:09
@simeonschaub simeonschaub merged commit d692b89 into master May 28, 2021
@simeonschaub simeonschaub deleted the sds/destructure_aliasing branch May 28, 2021 07:35
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label May 29, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
Jeff was a bit sceptical about this in JuliaLang#40574, but I think this
is worth doing since it seems strictly more useful than the current
behavior and we already go out of our way in cases like `x, y = y, x` to
only assign to the lhs after the rhs has been evaluated, so I think
handling `x[2], x[1] = x` in a similar way would only be consistent. As
a general rule that assignment will always happen after destructuring
does not seem much less obvious than the current behavior to me. This
might technically be slightly breaking, but I would be surprised if
people relied on the current behavior here. Nevertheless, it would
probably be good to run PkgEval if we decide to go with this change.

closes JuliaLang#40574
simeonschaub added a commit that referenced this pull request Jul 2, 2021
Discovered while working on #40737. Currently, this throws an
unintuitive error:
```julia
julia> a, f()... = 1, 2, 3
ERROR: syntax: ssavalue with no def
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1
```

Might as well fix this properly - Why not allow function definitions to
slurp a little from time to time? ;)
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Jeff was a bit sceptical about this in JuliaLang#40574, but I think this
is worth doing since it seems strictly more useful than the current
behavior and we already go out of our way in cases like `x, y = y, x` to
only assign to the lhs after the rhs has been evaluated, so I think
handling `x[2], x[1] = x` in a similar way would only be consistent. As
a general rule that assignment will always happen after destructuring
does not seem much less obvious than the current behavior to me. This
might technically be slightly breaking, but I would be surprised if
people relied on the current behavior here. Nevertheless, it would
probably be good to run PkgEval if we decide to go with this change.

closes JuliaLang#40574
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Discovered while working on JuliaLang#40737. Currently, this throws an
unintuitive error:
```julia
julia> a, f()... = 1, 2, 3
ERROR: syntax: ssavalue with no def
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1
```

Might as well fix this properly - Why not allow function definitions to
slurp a little from time to time? ;)
KristofferC pushed a commit that referenced this pull request Jul 7, 2021
Discovered while working on #40737. Currently, this throws an
unintuitive error:
```julia
julia> a, f()... = 1, 2, 3
ERROR: syntax: ssavalue with no def
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1
```

Might as well fix this properly - Why not allow function definitions to
slurp a little from time to time? ;)

(cherry picked from commit 5c49a0d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluation order when using mutating operations with destructuring assignment
5 participants