-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fix associativity inconsistencies of "->", "", "where" and ":" #59815
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
base: master
Are you sure you want to change the base?
Fix associativity inconsistencies of "->", "", "where" and ":" #59815
Conversation
This is one of the PRs where you start with a simple finding and then you go down the rabbit hole: - The relationship between operator precedence and associativity does not seem to be documented anywhere. So document it in - `operator_associativity`'s docstring - `operator_precedence`'s docstring - documentation of mathematical operations - link to each other in the docstrings - the associativity is not defined, so document it in `operator_associativity`'s extended help. - test for (nearly) all operators that the used associativity in the parser matches what `Base.operator_associativity` returns - find that we have indeed not 1, but 4 deviations - assume that the parser is correct and change `Base.operator_associativity` to fix the inconsistencies Please focus the review especially on these points: - check whether `Base.operator_associativity` was wrong or whether this really needs to be fixed withing `JuliaSyntax.jl`. - check whether the assumed relationship between precedence and associativity is correct. - check whether the definition of associativity is correct Probably for the future: - This PR should probably be backported. - Currently we only test each operator with itself for correct associativity. In general all operators in the same precedence level should be tested with each other (in both orders). As a heuristic they could all be tested with the first and last operator of the same precedence level of a certain associativity. - Check whether this logic should really be part of `Base` itself. It feels like poor encapsulation to do so. If this does not lead to bootstrapping problems, I think the logic should be moved to `JuliaSyntax.jl`. `Base` should only have forwarding functions `operator_associativity` and `operator_precedence` to the correspondingly named `JuliaSyntax.jl` functions.
base/show.jl
Outdated
| Associativity | Parsing behavior | | ||
|:-------------:|:-----------------------------------------:| | ||
| `:left` | `(x ⊙ y) ⊡ z == x ⊙ y ⊡ z != x ⊙ (y ⊡ z)` | |
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.
| `:left` | `(x ⊙ y) ⊡ z == x ⊙ y ⊡ z != x ⊙ (y ⊡ z)` | | |
| `:left` | `(x ⊙ y) ⊡ z == x ⊙ y ⊡ z` | |
this !=
is not actually guaranteed. what if all three expressions are equal?
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.
Thanks for starting the review.
I am not sure whether I got your point. Do you have an example of "all three expressions are equal"?
I concluded the definition I documented in this PR from the current behavior of JuliaSyntax.jl
. There, you always seem to have
julia> :⊗ |> operator_associativity
:left
julia> Meta.parse("2 ⊗ 3 ⊗ 4")
:((2 ⊗ 3) ⊗ 4)
i.e. for associativity :left
always the left operator is in parentheses and never the right one.
This is also what is tested with the tests added with this PR, where none of the tested operators (currently?) fails this condition.
Note, that this table only describes the parsing behavior. It does not mean that the result of the expressions will be different.
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 find this way of writing it confusing. It seems sufficient to me to just show either (x + y) + z
or x + (y + z)
.
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 just mean the guarantee for :left
is that a + b + c == (a + b) + c
. it is not guaranteed one way or the other that it will also equal a + (b + c)
, but the docstring makes it seem as if it is explicitly guaranteeing a !=
value.
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 find this way of writing it confusing. It seems sufficient to me to just show either
(x + y) + z
orx + (y + z)
.
Thanks, I boiled it down to the short form as suggested by you.
Do you also suggest to not use exotic/generic operators such as ⊙
and ⊡
and/or to only use a single operator (twice) instead of two different operators?
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 just mean the guarantee for
:left
is thata + b + c == (a + b) + c
. it is not guaranteed one way or the other that it will also equala + (b + c)
, but the docstring makes it seem as if it is explicitly guaranteeing a!=
value.
I hope I have addressed this by implementing Jeff's suggestion.
But to hopefully reduce the confusion even more: I am pretty sure, that it is guaranteed as a parsing behavior. It is obviously not guaranteed as the result of the expression.
Try it out yourself:
Meta.parse("a & b & c") == :((a & b) & c) == :(a & b & c) != :(a & (b & c))
And this is true although obviously
(a & b) & c == a & b & c == a & (b & c)
is also true.
what is the distinction between "same precedence but parsing depends on associativity" and just "different precedence" ? if associativity helps define parsing precedence |
Currently we have "parsing precedence" which consists of two levels:
As |
This is one of the PRs where you start with a simple finding and then you go down the rabbit hole:
operator_associativity
's docstringoperator_precedence
's docstringoperator_associativity
's extended help.Base.operator_associativity
returnsBase.operator_associativity
to fix the inconsistenciesPlease focus the review especially on these points:
Base.operator_associativity
was wrong or whether this really needs to be fixed withingJuliaSyntax.jl
.Probably for the future:
Base
itself. It feels like poor encapsulation to do so. If this does not lead to bootstrapping problems, I think the logic should be moved toJuliaSyntax.jl
.Base
should only have forwarding functionsoperator_associativity
andoperator_precedence
to the correspondingly namedJuliaSyntax.jl
functions.