Skip to content

Update operator precedence table - #2625

Open
jrfnl wants to merge 3 commits into
php:masterfrom
jrfnl:feature/2622-update-operator-precedence-table
Open

Update operator precedence table#2625
jrfnl wants to merge 3 commits into
php:masterfrom
jrfnl:feature/2622-update-operator-precedence-table

Conversation

@jrfnl

@jrfnl jrfnl commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

Updates the table to match the information per the parser.

This adds entries for:

  • =>
  • include, include_once, require, require_once
  • fn
  • throw

Ref: https://github.com/php/php-src/blob/2e2416825d2270d256f7597904e14e386cc0d854/Zend/zend_language_parser.y#L54-L82
Ref: https://github.com/php/php-src/blob/82a15338e298142f52854b64d803695d4e5252df/Zend/zend_language_parser.y#L53-L82 (Sept 2026)

Fixes ++Related to++ #2622

@jrfnl
jrfnl force-pushed the feature/2622-update-operator-precedence-table branch from 9016bac to 0a3e857 Compare July 30, 2023 13:23
@Girgias
Girgias requested a review from iluuu1994 July 31, 2023 13:30
@Girgias

Girgias commented Jul 31, 2023

Copy link
Copy Markdown
Member

@iluuu1994 could you double-check the accuracy?

Comment thread language/operators.xml Outdated
<entry>
<link linkend="language.types.array">array</link>,
<link linkend="control-structures.match">match</link>&listendand;
<link linkend="functions.arrow">fn</link>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this precedence is there exclusively to disambiguate [yield $expr => $expr]. All other uses of => are unambiguous due to braces.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the link block to only mention yield and linked to the language page for that.

Comment thread language/operators.xml Outdated
</row>
<row>
<entry>(n/a)</entry>
<entry><literal>fn</literal></entry>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREC_ARROW_FUNCTION is used for the backup flags, rather than the global expression precedence. I think this can be dropped.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @bwoebi can verify, I don't know exactly how %prec works in Bison. Maybe it's there to disambiguate fn() => (1 + 2) vs (fn() => 1) + 2

@bwoebi bwoebi Aug 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREC_ARROW_FUNCTION is actually defining the real precedence, but it's not about +, but about the logical operators. If it weren't for %precedence PREC_ARROW_FUNCTION, fn() => 1 or 2 would be (fn() => 1) or 2. Basically overriding the precedence of the T_DOUBLE_ARROW and resolving the s/r conflict with yield.

@bwoebi bwoebi Aug 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the precedence does not really apply to fn, but to the T_DOUBLE_ARROW used in a fn() expression. (backup_fn_flags is explicitly at the place it is, to apply it's precedence there, exactly where the => is.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. how should I change this in that case ? Cause that sounds like it should be a double arrow, but then we'd end up with the double arrow twice in the table, once for yield and once for fn. Did I understand that correctly ? And is that what you'd like me to change it to ?

@iluuu1994 iluuu1994 Sep 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. I suppose this case is somewhat special, in that the precedence is only applies in the arrow function context. Maybe we can make that clear by using fn (...) => as the operator, or we just leave this out as an implementation detail.

I.e. we're trying to decrease the precedence of => in that context to something lower than T_LOGICAL_*, so that fn() => 1 or 2 is parsed as fn() => (1 or 2) instead of (fn() => 1) + 2 (thanks Bob for the explanation!).

@jrfnl
jrfnl force-pushed the feature/2622-update-operator-precedence-table branch from 0a3e857 to 75f7c14 Compare August 1, 2023 16:18
@jrfnl

jrfnl commented Aug 18, 2023

Copy link
Copy Markdown
Contributor Author

Anything I can do to move this forward ?

@Girgias
Girgias requested review from bwoebi and iluuu1994 August 18, 2023 23:03
@lacatoire

Copy link
Copy Markdown
Member

@jrfnl Sorry for the delay. The precedence table has moved since this pull request was opened: language/operators.xml was split by #2799, and the table now lives in language/operators/precedence.xml. That is what the conflict comes from.

@jrfnl

jrfnl commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@jrfnl Sorry for the delay. The precedence table has moved since this pull request was opened: language/operators.xml was split by #2799, and the table now lives in language/operators/precedence.xml. That is what the conflict comes from.

Thanks for the pointer, but that didn't answer my question, which I asked well before a conflict ever got created for this PR.

Either way, I'll rebase the PR while keeping the current page split in mind to get rid of the conflict.
I'll also re-compare against the precedence table just in case something new was introduced and not documented yet.

@jrfnl

jrfnl commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Oh and the underlying issue (throw missing from the precedence table) this PR was trying to solve has been solved by another PR #5083 since.... though this PR still has value for the other things it does....

@jrfnl
jrfnl force-pushed the feature/2622-update-operator-precedence-table branch from 75f7c14 to 9ef4f87 Compare September 9, 2026 12:01
Comment on lines +316 to +317
<entry><literal>=&gt;</literal></entry>
<entry><link linkend="functions.arrow">fn arrow</link> disambiguity</entry>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREC_ARROW_FUNCTION is attached to the empty backup_fn_flags rule, which sits after expr in the arrow function rule, so what it governs is where the body stops, not how => binds:

At fn() => 1 • or 2 the parser chooses between reducing that empty rule and shifting or; since PREC_ARROW_FUNCTION is below T_LOGICAL_OR it shifts. That is a different thing from the => of yield, whose precedence is carried by the token itself in L1383 (T_YIELD expr T_DOUBLE_ARROW expr) and declared at L61.

Putting => in the operator column for both rows therefore reads as two contradictory precedences for one token, which is what makes the entry look odd. The effect is still worth a row, since it is the one case the table cannot currently predict:

$x = 1 or 2;            // $x is 1                    -> ($x = 1) or 2
yield 'k' => 1 or 2;    // yields 1                   -> (yield 'k' => 1) or 2
fn() => 1 or 2;         // Closure, body returns true -> fn() => (1 or 2)

Naming the construct instead of the token says it without the contradiction. The wording holds for every operator: no binary operator is declared below PREC_ARROW_FUNCTION, only T_THROW, which cannot follow an expression anyway.

Suggested change
<entry><literal>=&gt;</literal></entry>
<entry><link linkend="functions.arrow">fn arrow</link> disambiguity</entry>
<entry><literal>fn</literal></entry>
<entry>
The body of an <link linkend="functions.arrow">arrow function</link>
extends as far to the right as possible
</entry>

The row's position is right as it stands and should not move: between include/require and throw.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lacatoire This has been discussed before above: #2625 (comment) - also your take reads as AI generated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, it's the T_DOUBLE_ARROW, and it would appear twice. Let's go with iluuu1994's first suggestion. Write the operator as fn (...) => rather than a bare =>, so the two rows can't be confused.
The behaviour is worth the row: fn() => 1 or 2 puts the or inside the body, which nothing else in the table lets you predict. I'll merge as soon as you've rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants