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

allow slurping in lhs of assignment #37410

Merged
merged 4 commits into from
Oct 26, 2020

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented Sep 5, 2020

This is currently branched off #37268. I will rebase once that is merged, but for now, only the last commit is relevant. This implements @StefanKarpinski's comment in #2626 (comment). I am not sure, slurp_rest is the best name and API for this, perhaps it would make more sense to extend iterate_and_index to return a third function for slurping, since slurp_rest currently relies on what iterate does, which could lead to gotchas if users overload iterate_and_index.
fixes #2626

@JeffBezanson
Copy link
Member

JeffBezanson commented Sep 5, 2020

Cool! My current thinking is that if we do this, it should use Base.rest (or tail?), which can be overloaded appropriately --- arrays can return arrays (or views?), if you had a linked list (:rofl:) it could return the list tail, tuples can return tuples, and infinite or lazy iterators can return Rest iterators. I don't think this is necessarily inconsistent with varargs, since arguments are always implicitly a tuple

@simeonschaub
Copy link
Member Author

Ok, that can certainly be discussed, I was just under the impression that Stefan's comment concluded the discussion in #2626. I personally would still favor just always returning tuples, because I think it's going to be difficult to find a rule for which container types should slurp to which type for the rest and it's simpler to remember that it's just always a tuple. For example, what should a..., = 1 do? a, b... = 1? I see that it's not necessarily inconsistent with varargs, but to me, it still feels a bit weird for such a language feature to change meaning based on the input type instead of returning a tuple.

@JeffBezanson
Copy link
Member

It's not changing meaning, it's using a different implementation of that meaning. Multiple data structures support the concept of rest, and each one implements it by returning a smaller version of the same structure. Using only tuples prevents e.g. infinite iterators from working, when they could work. Lots of syntax features like this lower to overloadable functions.

@simeonschaub
Copy link
Member Author

I mostly want this just for tuples, so what to do for other iterators is not a hill I'm willing to die on. What do you reckon would be the best way forward here? Should this just be triaged, or would it make sense to reopen the discussion in #2626, so that people that participated there can chime in?

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Sep 8, 2020
@simeonschaub simeonschaub mentioned this pull request Sep 29, 2020
@JeffBezanson
Copy link
Member

Ok, let's have this call Base.rest(itr, state), and it can be overloaded to be eager for e.g. arrays and tuples.

@simeonschaub
Copy link
Member Author

simeonschaub commented Oct 1, 2020

So I'm thinking as the default return types of Base.rest:

  • Tuple -> Tuple
  • AbstractArray -> Vector
  • everything else -> Iterators.rest

Does that make sense, or is there anything else that would make sense to special-case? Perhaps UnitRange?

@simeonschaub
Copy link
Member Author

Ah, this doesn't work with just passing the iteration state to Base.rest, because with #37268, iterate_and_index(x)[1], won't tell us anything about the current iteration state for things like tuples or vectors, since it just returns the object itself and indexes with the index generated by lowering. We either need to change how iterate_and_index works for those or need to pass the index generated by lowering to Base.rest as well.

base/tuple.jl Outdated Show resolved Hide resolved
base/tuple.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

So the interesting thing is that this feature is slightly incompatible with #37268, since that change means we don't always have an iteration state available. To get elements, you don't need the iteration state, but for ... you do. Hence the need for _rest. But it probably doesn't matter much since you only have to implement _rest if you implemented indexed_iterate before (right?), and nobody was supposed to do that :)

@JeffBezanson
Copy link
Member

I think we are not going to do the indexed_iterate lowering change for now, so this can be rebased on master and we can merge it.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Oct 21, 2020
@simeonschaub
Copy link
Member Author

So we decided to make Base.rest part of the public API, but not export it, right? Should probably have a docstring and some separate tests then. Also still needs news. The actual implementation should pretty much be done from my side now.

@@ -2399,3 +2399,11 @@ function hash(A::AbstractArray, h::UInt)

return h
end

# The semantics of `collect` are weird. Better to write our own
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about collect(T, Iterators.rest(a, state))? Then for the 1-argument case Vector{T}(a). Better to avoid repeated push! since it's a bit slow.

Copy link
Member Author

@simeonschaub simeonschaub Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then for the 1-argument case Vector{T}(a).

That won't work for multi-dimensional arrays, unfortunately:

julia> Vector{Int}([1 2; 3 4])
ERROR: MethodError: no method matching Array{Int64,1}(::Array{Int64,2})
Closest candidates are:
  Array{Int64,1}(::AbstractArray{S,N}) where {T, N, S} at array.jl:562
  Array{Int64,1}() where T at boot.jl:425
  Array{Int64,1}(::UndefInitializer, ::Int64) where T at boot.jl:406
  ...
Stacktrace:
 [1] top-level scope at REPL[25]:1

collect also doesn't always return a Vector and I think it would be weird if Base.rest(::A) and Base.rest(::A, st) returned different types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note that:

julia> Base.IteratorSize(Iterators.rest([1 2; 3 4], 1))
Base.SizeUnknown()

so collect will end up just using push! anyways.

base/tuple.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Oct 23, 2020
@JeffBezanson
Copy link
Member

Looking good.

@JeffBezanson JeffBezanson self-assigned this Oct 23, 2020
Co-authored-by: Jeff Bezanson <[email protected]>
@simeonschaub simeonschaub changed the title RFC: allow slurping in lhs of assignment allow slurping in lhs of assignment Oct 24, 2020
@JeffBezanson JeffBezanson merged commit 6edf6d9 into JuliaLang:master Oct 26, 2020
@StefanKarpinski
Copy link
Member

OMG, I did not think this was happening!

fredrikekre added a commit that referenced this pull request Oct 26, 2020
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Oct 26, 2020
@tlienart tlienart mentioned this pull request Nov 2, 2020
ViralBShah added a commit that referenced this pull request Nov 10, 2020
There's also a bunch of issue links that are missing (#37410, #37247, #37540, #37973, #37461, #37753) but it seems there's a script that generates the links so I'm assuming that will be fixed automatically.

Co-authored-by: Viral B. Shah <[email protected]>
achuchmala pushed a commit to achuchmala/julia that referenced this pull request Nov 11, 2020
There's also a bunch of issue links that are missing (JuliaLang#37410, JuliaLang#37247, JuliaLang#37540, JuliaLang#37973, JuliaLang#37461, JuliaLang#37753) but it seems there's a script that generates the links so I'm assuming that will be fixed automatically.

Co-authored-by: Viral B. Shah <[email protected]>
@simeonschaub simeonschaub added compiler:lowering Syntax lowering (compiler front end, 2nd stage) and removed needs docs Documentation for this change is required labels Nov 11, 2020
simonbyrne added a commit that referenced this pull request Apr 22, 2021
* Document assignment destructuring
* Add reference to slurping assignment (#37410)
* mention swapping variables and other kinds of LHS

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Simeon Schaub <[email protected]>
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* Document assignment destructuring
* Add reference to slurping assignment (JuliaLang#37410)
* mention swapping variables and other kinds of LHS

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Simeon Schaub <[email protected]>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* Document assignment destructuring
* Add reference to slurping assignment (JuliaLang#37410)
* mention swapping variables and other kinds of LHS

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Simeon Schaub <[email protected]>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
* Document assignment destructuring
* Add reference to slurping assignment (JuliaLang#37410)
* mention swapping variables and other kinds of LHS

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Simeon Schaub <[email protected]>
@simeonschaub simeonschaub deleted the slurp_assignment branch November 4, 2021 15:30
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a, b... = [1,2,3]
3 participants