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.

Sign up for GitHub

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

Jump to bottom

don't mutate lhs while destructuring #40737

Merged
merged 1 commit into from
May 28, 2021
Merged

don't mutate lhs while destructuring #40737

merged 1 commit into from
May 28, 2021

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 added a commit that referenced this pull request May 6, 2021
@simeonschaub
fix slurping into function definition
c1bc3c9 
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 simeonschaub mentioned this pull request May 6, 2021
Merged
@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 •
edited
Loading

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 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 reopened this May 27, 2021
@simeonschaub
don't mutate lhs while destructuring
030bf53 
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 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
@simeonschaub
don't mutate lhs while destructuring (JuliaLang#40737)
b00020f 
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
@simeonschaub
fix slurping into function definition (#40738)
5c49a0d 
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
@simeonschaub @johanmon
don't mutate lhs while destructuring (JuliaLang#40737)
fdffd63 
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
@simeonschaub @johanmon
fix slurping into function definition (JuliaLang#40738)
675f93f 
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
@simeonschaub @KristofferC
fix slurping into function definition (#40738)
8ebbd94 
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
Reviewers

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees

@JeffBezanson JeffBezanson

Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

Evaluation order when using mutating operations with destructuring assignment
5 participants
@simeonschaub @JeffBezanson @nanosoldier @StefanKarpinski @vtjnash