-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
implement property destructuring #39285
Conversation
(define (expand-tuple-destruct lhss x) | ||
(define (sides-match? l r) | ||
;; l and r either have equal lengths, or r has a trailing ... | ||
(cond ((null? l) (null? r)) | ||
((vararg? (car l)) #t) | ||
((null? r) #f) | ||
((vararg? (car r)) (null? (cdr r))) | ||
(else (sides-match? (cdr l) (cdr r))))) | ||
(if (and (pair? x) (pair? lhss) (eq? (car x) 'tuple) (not (any assignment? (cdr x))) | ||
(not (has-parameters? (cdr x))) | ||
(sides-match? lhss (cdr x))) | ||
;; (a, b, ...) = (x, y, ...) | ||
(expand-forms | ||
(tuple-to-assignments lhss x)) | ||
;; (a, b, ...) = other | ||
(begin | ||
;; like memq, but if last element of lhss is (... sym), | ||
;; check against sym instead | ||
(define (in-lhs? x lhss) | ||
(if (null? lhss) | ||
#f | ||
(let ((l (car lhss))) | ||
(cond ((and (pair? l) (eq? (car l) '|...|)) | ||
(if (null? (cdr lhss)) | ||
(eq? (cadr l) x) | ||
(error (string "invalid \"...\" on non-final assignment location \"" | ||
(cadr l) "\"")))) | ||
((eq? l x) #t) | ||
(else (in-lhs? x (cdr lhss))))))) | ||
;; in-lhs? also checks for invalid syntax, so always call it first | ||
(let* ((xx (if (or (and (not (in-lhs? x lhss)) (symbol? x)) | ||
(ssavalue? x)) | ||
x (make-ssavalue))) | ||
(ini (if (eq? x xx) '() (list (sink-assignment xx (expand-forms x))))) | ||
(n (length lhss)) | ||
;; skip last assignment if it is an all-underscore vararg | ||
(n (if (> n 0) | ||
(let ((l (last lhss))) | ||
(if (and (vararg? l) (underscore-symbol? (cadr l))) | ||
(- n 1) | ||
n)) | ||
n)) | ||
(st (gensy))) | ||
`(block | ||
,@(if (> n 0) `((local ,st)) '()) | ||
,@ini | ||
,@(map (lambda (i lhs) | ||
(expand-forms | ||
(if (vararg? lhs) | ||
`(= ,(cadr lhs) (call (top rest) ,xx ,@(if (eq? i 0) '() `(,st)))) | ||
(lower-tuple-assignment | ||
(if (= i (- n 1)) | ||
(list lhs) | ||
(list lhs st)) | ||
`(call (top indexed_iterate) | ||
,xx ,(+ i 1) ,@(if (eq? i 0) '() `(,st))))))) | ||
(iota n) | ||
lhss) | ||
(unnecessary ,xx)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NFC here, the level of indenting was just getting a bit much, so I moved this into a separate function.
Awesome! Thanks for this. Would you consider adding named tuple/struct destructuring in function arguments to this PR? That was the original ask in #28579. |
If you have a look at the tests, you can see that this already just works™️. Lowering works recursively, so we basically get this for free. |
Nice! Yeah I was actually just looking at the tests now and noticed that. Thanks! Do you know if the following syntax would work? struct A
x
y
end
foo((; x, y)::A) = x + y In other words, I want to be able to both dispatch on |
Sure, that works just fine: julia> struct A
x
y
end
julia> foo((; x, y)::A) = x + y
foo (generic function with 1 method)
julia> foo(A(1, 2))
3 |
Ha, ok that example doesn't quite prove that it works fine. I need to see a method error if you call |
Here you go 😄 julia> foo((x=1, y=2))
ERROR: MethodError: no method matching foo(::NamedTuple{(:x, :y), Tuple{Int64, Int64}})
Closest candidates are:
foo(::A) at REPL[2]:1
Stacktrace:
[1] top-level scope
@ REPL[5]:1 If you don't want to build it yourself, you could also download the build from https://s3.amazonaws.com/julialangnightlies/assert_pretesting/linux/x64/1.7/julia-6aede6071e-linux64.tar.gz to try this out (assuming you are on Linux). |
Nice! Seems elegant enough to me. |
Yes, this is a nice, elegant feature! |
Agreed, I have wanted this! Though calling |
One use case where I wanted something like this before was for destructuring DataFrames, which don't define |
Triage is ok with this, but we note that it would be nice to add |
This currently only allows the form `(; a, b) = x` and lowers this to calls to `getproperty`. We could think about whether we want to allow specifying default values for properties as well, or even some kind of property renaming in the lhs. We could even allow slurping unused properties, but all of that sounds more difficult to work out in detail and potentially controversial, so I left this as an error for now. fixes #28579
6aede60
to
d2e2a9b
Compare
Buildbot failure is the usual "hard kill repl test". |
* implement property destructuring This currently only allows the form `(; a, b) = x` and lowers this to calls to `getproperty`. We could think about whether we want to allow specifying default values for properties as well, or even some kind of property renaming in the lhs. We could even allow slurping unused properties, but all of that sounds more difficult to work out in detail and potentially controversial, so I left this as an error for now. fixes JuliaLang#28579 * add NEWS entry
* implement property destructuring This currently only allows the form `(; a, b) = x` and lowers this to calls to `getproperty`. We could think about whether we want to allow specifying default values for properties as well, or even some kind of property renaming in the lhs. We could even allow slurping unused properties, but all of that sounds more difficult to work out in detail and potentially controversial, so I left this as an error for now. fixes JuliaLang#28579 * add NEWS entry
Sorry to barge in here with this, but after looking at the v1.7 announcement, I have a question. |
No, not really. Of course, if there is self-contained functionality that would make sense to have in Base, that can be discussed separately. |
Alright thanks ! |
I can't find this documented in the manual anywhere? Seems like an oversight in this PR? |
Docs where added in #41189 |
Hi. splat destructuring is not currently supported
|
This currently only allows the form
(; a, b) = x
and lowers this to calls togetproperty
. We could think about whether we want to allow specifying default values for properties as well, or even some kind of property renaming in the lhs. We could even allow slurping unused properties, but all of that sounds more difficult to work out in detail and potentially controversial, so I left this as an error for now.fixes #28579