-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add infix operator precedence rules #5273
Conversation
My one concern is with I think it would hurt readability if my carefully crafted Or perhaps we can change the rules so it parses with the current precedence but still parenthesizes when printing just to avoid confusion? TBH I think if I were designing just for me my preference would be to just throw a parse error |
I’m trying to figure out if this is more intuitive from the user’s perspective than just giving un-precedenced operators the lowest precedence (one lower than the lowest precedenced op). E.g., if I write the expression But with “lowest” precedence rules, To get both to parse to I feel like I have to think pretty hard about the asymmetric version of this, rather than “other operators have precedence -1”. With lowest precedence, we still get
Should this be more like “The precedence of the operator in the subexpression is at least as high (if it’s on the left) or higher (if it’s on the right) than the parent expression.”? Granted, my phrasing is awkward, but in the current description, it implies that
I agree with this decision. Logically combining a lot of comparisons is a common pattern, and I think people expect this precedence. |
|
Yeah, I had this same thought, but I think making them the same precedence and not changing anything else would result in a worse situation (I think the pretty printer would print We could apply the “extra parens” rules for un-precedenced operators to these as well, and get the more explicit groupings. However, they’re already the lowest precedence, so if the handling of un-precedenced operators is handled as I suggest in my previous comment, we could just remove the precedence for logical operators and get the behavior you want. As for how they interact with the other operators with those precedences, I assume |
I’m a little surprised these are separate, but I think it makes sense. You’re unlikely to have an expression like |
I also wonder if |
import Unison.Prelude | ||
|
||
-- Precedence rules for infix operators. | ||
-- Lower number means higher precedence (tighter binding). |
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 doesn’t match the implementation – higher is higher (which is good).
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.
Good catch, this is a defunct comment
Top | ||
deriving (Eq, Ord, Show) | ||
|
||
data InfixPrecedence = Lowest | Level Int | Highest |
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.
Are Lowest
and Highest
only needed for pretty-printing the asymmetric semantics of un-precedenced operators?
If we switch to just giving them minimal precedence, can this become
data InfixPrecedence = Word8
If we still need some bounds-checking of infix precedences, could we get by with something like this?
lowestInfix, highestInfix :: Precedence
lowestInfix = InfixOp minBound
highestInfix = InfixOp maxBound
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.
The pretty-printer was kind of a rat's nest of hacks around precedences, where in certain cases it was using a precedence in between "infix application" and "function application", as well as between infix and "control structure". I think we could probably get away with just making this a word8, but doing it this way allowed me to leave the implementation largely unchanged.
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.
Yeah, I had that feeling as well. Seems reasonable to defer any simplification / cleanup.
Annotation -> Statement | ||
Statement -> Control | ||
Control -> InfixOp Lowest | ||
InfixOp Lowest -> InfixOp (Level 0) |
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 feel like Level Word
(or even Word8
) would be an improvement for the type, so that this case is definitely true.
Control -> InfixOp Lowest | ||
InfixOp Lowest -> InfixOp (Level 0) | ||
InfixOp (Level n) -> InfixOp (Level (n + 1)) | ||
InfixOp Highest -> Application |
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.
Could use a comment on this function that there’s a disconnect between InfixOp (Level n)
and InfixOp Highest
– or, (especially if the type is changed to Level Word8
) have a case like
InfixOp (Level 255) -> InfixOp Highest
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.
LGTM 👍🏼
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.
Nice!!! Appreciate all the round trip tests as well.
My take: let's merge this if @runarorama is happy with it and dogfood it a bit. We can always make more tweaks, nbd.
I might prefer that mixing &&
and ||
gets parens in the pretty-printer, even if parsing has &&
higher precedence than ||
. But it's probably fine as and people will expect that precedence. A tooltip on &&
and ||
that mentions this relative precedence could help as well, cc @rlmark @hojberg.
One cool idea I had for later is that the pretty-printer could produce grouping information. Which is ignored when converting to text, but the HTML renderer could make use of it to allow highlighting of subexpressions and "expand selection to parent".
Overview
This adds precedence rules for infix operators, so e.g.
7 == 1 + 2 * 3
will be parsed as7 == (1 + (2 * 3))
instead of((7 == 1) + 2) * 3
. The term printer will also print these back without parentheses.Previously infix operators would always parse left-associated with no precedence.
This change adds hard-coded rules for infix operators based on their unqualified names. The precedence levels are as follows (higher precedence binds tighter):
||
and|
&&
and&
==
,!==
,!=
, and===
<
,>
,>=
, and<=
+
and-
*
,/
, and%
^
,^^
, and**
Operators other than this have no precedence, and are still simply parsed from left to right. Or rather: the default for any operator is that it has higher precedence than any operator to its right in the expression, and lower precedence than any operator to its left.
Examples:
true || false && true
=>true || (false && true)
1 + 2 >= 3 + 4
=>(1 + 2) >= (3 + 4)
9 % 2 == 0
=>(9 % 2) == 0
0 == 9 % 2
=>0 == (9 % 2)
2 * 10 $ 20
=>(2 * 10) $ 20
($
has no precedence rule)1 * 2 $ 3 * 4 $ 5
=>(((1 * 2) $ 3) * 4) $ 5
Fixes #895, #3467, #3468
Implementation notes
When parsing an expression with an infix ("symboly") function application, the parser proceeds as follows:
a + b * c
, we first parse(a + b) * c
then rotate the parse tree toa + (b * c)
.b
above might itself be an infix application with lower precedence than*
.When printing, the term printer prints a whole chain of infix operator applications without parentheses, e.g.
1 + 2 * 3 > 4 * 5
only if either:Otherwise parentheses are added around subexpressions.
This PR also changes the way precedence is implemented in the term printer. Instead of a number from
-2
to12
, it's now a data typePrecedence
with a total order:Interesting/controversial decisions
true || false == true
which parses astrue || (false == true)
.Loose ends
There are no associativity rules included here, so all operators associate to the left as before.
Test coverage
Updated the roundtrip test transcripts to include a bunch of expressions with a mix of infix operators, to make sure the parser and printer agree.