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

implement property destructuring #39285

Merged
merged 2 commits into from
Jan 23, 2021
Merged

Conversation

simeonschaub
Copy link
Member

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

@simeonschaub simeonschaub added compiler:lowering Syntax lowering (compiler front end, 2nd stage) needs news A NEWS entry is required for this change labels Jan 16, 2021
Comment on lines +1953 to +2011
(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))))))
Copy link
Member Author

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.

@simeonschaub simeonschaub added the status:triage This should be discussed on a triage call label Jan 16, 2021
@CameronBieganek
Copy link
Contributor

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.

@simeonschaub
Copy link
Member Author

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.

@CameronBieganek
Copy link
Contributor

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 A and unpack the fields of A.

@simeonschaub
Copy link
Member Author

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

@CameronBieganek
Copy link
Contributor

Ha, ok that example doesn't quite prove that it works fine. I need to see a method error if you call foo((x=1, y=2)). 😉

@simeonschaub
Copy link
Member Author

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).

@JeffBezanson
Copy link
Sponsor Member

Nice! Seems elegant enough to me.

@StefanKarpinski
Copy link
Sponsor Member

Yes, this is a nice, elegant feature!

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 19, 2021

Agreed, I have wanted this! Though calling getindex rather than getproperty feels to me like it would mesh with more existing types and usages?

@simeonschaub
Copy link
Member Author

Though calling getindex rather than getproperty feels to me like it would mesh with more existing types and usages?

One use case where I wanted something like this before was for destructuring DataFrames, which don't define getindex(x, ::Symbol). Besides a Dict with symbols as keys, I can't really think of anything that defines getindex(x, ::Symbol), but not getproperty. The nice thing about getproperty is also that it works with any user-defined type by default.

@JeffBezanson
Copy link
Sponsor Member

Triage is ok with this, but we note that it would be nice to add getproperty methods for Pairs of a NamedTuple, and really for that matter all AbstractDicts.

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
@simeonschaub
Copy link
Member Author

Buildbot failure is the usual "hard kill repl test".

@simeonschaub simeonschaub merged commit d9e2632 into master Jan 23, 2021
@simeonschaub simeonschaub deleted the sds/property_destructuring branch January 23, 2021 09:55
@simeonschaub simeonschaub removed needs news A NEWS entry is required for this change status:triage This should be discussed on a triage call labels Jan 23, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* 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
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* 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
@BambOoxX
Copy link

Sorry to barge in here with this, but after looking at the v1.7 announcement, I have a question.
I currently use Parameters.jl for this kind of things, with the @unpack macro, and I was wondering if the plan is to sort of integrate the functionalities of this package in Base, or if it is just sort of a coincidence or else ? Thanks.

@simeonschaub
Copy link
Member Author

No, not really. Of course, if there is self-contained functionality that would make sense to have in Base, that can be discussed separately.

@BambOoxX
Copy link

Alright thanks !

@stevengj
Copy link
Member

stevengj commented Feb 9, 2022

I can't find this documented in the manual anywhere?

Seems like an oversight in this PR?

@fredrikekre
Copy link
Member

Docs where added in #41189

@o314
Copy link
Contributor

o314 commented Jul 24, 2022

Hi. splat destructuring is not currently supported

julia> f((; head, arg)) = (; head, arg)
f (generic function with 1 method)

julia> f((; head, args...)) = (; head, args...)
ERROR: syntax: malformed expression

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.

NamedTuple unpacking (and also struct unpacking)
9 participants