-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove no_parens when using capture with arity #14090
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
Conversation
| {_, []} when not Sequential -> | ||
| invalid_capture(Meta, Expr, E); | ||
| {EExpr, []} -> | ||
| Meta2 = lists:keydelete(no_parens, 1, Meta), |
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.
This is not strictly needed for the fix, but otherwise we're expanding to something like {:fn, [{:no_parens, true}], [{:->, [{:no_parens, true}], ... which feels weird and make it harder to test.
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.
I think ideally we want to compile this to:
Function.capture(expr, name, arity)
So something we get something like this back:
iex> &(if true, do: NaiveDateTime, else: DateTime).utc_now/0
&NaiveDateTime.utc_now/0
Note we already do this for variables:
iex> var = NaiveDateTime
iex> &var.utc_now/0
&NaiveDateTime.utc_now/0
So we probably need to generalize this branch of the code a bit more (i.e. it should apply whenever the left side of the . is an expression)?
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.
Nah, ignore me, there is clearly a semantic difference here. Your version executes foo() in &foo().bar/0 when the function is invoked. Mine would execute it always.
I think this is actually a bug. We should not have accepted this format but we clearly do. My suggestion is to deprecate this feature in main.
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.
To be clear beyond doubt:
&expr.fun/arity
Only works if:
expris a variable or an atom at compile-time- fun is an atom at compile-time
- arity is an integer at compile-time
If those conditions are not met and it is valid, we should deprecate it.
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.
Sounds good, I hesitated to propose something in this vein but was unsure if there would be other legit cases.
As you said assigning to a variable or using fn seems better either way.
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.
Ship it and please back port it.
I will open up a separate issue for the deprecation, feel free to tackle it!
Thanks, will do! |
Close #14089
To be backported