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

Move operator definitions to objects #4543

Open
wants to merge 8 commits into
base: 3.x
Choose a base branch
from

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented Jan 19, 2025

No description provided.

@fabpot fabpot force-pushed the operator-classes branch 5 times, most recently from 4461487 to bd7a8df Compare January 19, 2025 18:42
doc/deprecated.rst Outdated Show resolved Hide resolved
src/ExtensionSet.php Outdated Show resolved Hide resolved
src/Operator/OperatorInterface.php Show resolved Hide resolved
src/Operator/OperatorInterface.php Outdated Show resolved Hide resolved
src/Operator/OperatorInterface.php Outdated Show resolved Hide resolved
src/Extension/ExtensionInterface.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/Operator/Binary/AbstractBinaryOperator.php Outdated Show resolved Hide resolved
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/Operator/Operators.php Outdated Show resolved Hide resolved
src/Operator/Operators.php Show resolved Hide resolved
src/Operator/Operators.php Show resolved Hide resolved
src/Operator/Operators.php Outdated Show resolved Hide resolved
src/Operator/Operators.php Outdated Show resolved Hide resolved
@fabpot fabpot force-pushed the operator-classes branch 5 times, most recently from 1749107 to 3d0e2f3 Compare January 26, 2025 14:28
@fabpot
Copy link
Contributor Author

fabpot commented Jan 26, 2025

New iteration where I have moved most pseudo-operators (logic hardcoded in ExpressionParser) to real operators like any other ones (think |, ., [, (, ??, ...).

@fabpot fabpot force-pushed the operator-classes branch 5 times, most recently from 339a8c3 to 4bf41b7 Compare January 26, 2025 14:53
@@ -0,0 +1,56 @@
Unary operators precedence:
Copy link
Contributor Author

@fabpot fabpot Jan 26, 2025

Choose a reason for hiding this comment

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

This doc is the most important part of this PR.
We need to review the precedence order carefully.

@fabpot fabpot force-pushed the operator-classes branch 2 times, most recently from 35ef5c7 to 0704840 Compare January 29, 2025 11:27
src/ExpressionParser.php Outdated Show resolved Hide resolved
src/Operator/Ternary/AbstractTernaryOperator.php Outdated Show resolved Hide resolved
src/Operator/Ternary/AbstractTernaryOperator.php Outdated Show resolved Hide resolved

public function getOperator(): string
{
return '(';
Copy link
Member

Choose a reason for hiding this comment

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

I find it really weird to treat parenthesis as a unary operator (its parsing implementation does not look like the parsing of an expression either btw)

300 |
.
[
(
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs more explanation to indicate that this parenthesis is representing the function call. I know no language that describe function calls as being a binary operator

Copy link
Member

Choose a reason for hiding this comment

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

this precedence looks weird to me compared to unary operators. How is - myfunc() parsed, before and after this change ? Same for - var[0]

Copy link
Member

Choose a reason for hiding this comment

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

Btw, what happens if an extension defines an operator with higher precedence that this "function call" operator ? Using a function call near this operator would suddenly not parse the function call in the expected way.

src/ExpressionParser.php Outdated Show resolved Hide resolved
public function __construct(array $items, int $lineno)
{
foreach ($items as $item) {
if (!$item instanceof ContextVariable) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to validate this here ? Isn't this also ensured by the parser when using parsing ?
For custom constructed nodes, we already have the parameter type defining the contract and we generally don't add runtime checks for array values when we have such contract.

src/Lexer.php Outdated Show resolved Hide resolved
$this->pushToken(Token::OPERATOR_TYPE, preg_replace('/\s+/', ' ', $match[0]));
$operator = preg_replace('/\s+/', ' ', $match[0]);
$type = Token::OPERATOR_TYPE;
// to be removed in 4.0
Copy link
Member

Choose a reason for hiding this comment

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

This removal will likely make things weird for the token stream (which will impact custom token parsers). After this removal, ( would generator an OPERATOR_TYPE token, while ) will not (will it stay a PUNCTUATION_TYPE ? it depends how the new PUNCTUATION constant will look like)

src/TokenParser/AbstractTokenParser.php Outdated Show resolved Hide resolved
@fabpot fabpot force-pushed the operator-classes branch 2 times, most recently from dc96f57 to ec728b6 Compare January 29, 2025 18:43
CHANGELOG Outdated
@@ -1,6 +1,9 @@
# 3.20.0 (2025-XX-XX)

* n/a
* Bump minimum PHP version to 8.1
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to update the PHP requirement in composer.json to make Composer aware of this bump.

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

Successfully merging this pull request may close these issues.

3 participants