Skip to content

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Aug 7, 2020

Motivations: This would be potentially useful for class constant and static variable declarations that would previously need to use (some_value === b ? c : (some_value === d || some_value === d2 ? e : f)) instead of match (some_value) { b => c, d, d2 => e, default => f}

I'd forgotten to check if this was part of the original RFC earlier.
It seems like constant expressions using match can be thought of as a much
more concise form of conditional than chained ternary(a?b:c) operators.

It's already possible for constant expressions to throw, e.g.

  • Undeclared constants.
  • Wrong operands for unary or binary operators.

(But the UnhandledMatchError would be new,)

@nikic @iluuu1994 - thoughts on allowing this for PHP 8.0 or 8.1? I'd personally assumed this would be an implementation oversight since I don't remember seeing constant expressions discussed (it was a large PR)
Prior to this proposed change, the error message was always Constant expression contains invalid operations

I'd forgotten to check if this was part of the original RFC.
It seems like constant expressions using match can be thought of as a much
more concise form than chained ternary operators.

It's already possible for constant expressions to throw, e.g.
- Undeclared constants.
- Wrong operands for unary or binary operators.
@nikic
Copy link
Member

nikic commented Aug 7, 2020

I don't really see a usefulness for match in constant expression context, but I'd be okay with allowing it for the sake of consistency. You should probably start an internals thread about this, to decide which version to target.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Why not, could be useful in some cases (although probably rare) 🙂

--FILE--
<?php

// Here, this test uses strtolower because the locale dependence and change to the string
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, got sidetracked checking if constant expressions were proposed before and forgot to update this.

@TysonAndre
Copy link
Contributor Author

TysonAndre commented Aug 8, 2020

I don't really see a usefulness for match in constant expression context, but I'd be okay with allowing it for the sake of consistency. You should probably start an internals thread about this, to decide which version to target.

On second thought, I don't think I'll use this often enough to justify working on this. Both of the alternatives aren't perfect, but I'd probably still vote yes on either of them if someone else started the RFC process for them (e.g. based on this code)

  1. Allowing UnhandledMatchError would be a minor shift in how constants are declared, even if errors can already be thrown unintentionally
  2. Enforcing that matches had a default arm would require complicating the API of zend_bool zend_is_allowed_in_const_expr(zend_ast_kind kind) /* {{{ and would be the only node kind that had such a restriction in constants, and linters would have to end up reproducing the same check. A new issue message such as "Match expressions in constant expressions must have a default arm" would be user-friendlier but add complexity to complex expressions. (But I doubt many things outside of PHP use that API).

@TysonAndre TysonAndre closed this Aug 8, 2020
@@ -654,6 +654,51 @@ ZEND_API int ZEND_FASTCALL zend_ast_evaluate(zval *result, zend_ast *ast, zend_c
zval_ptr_dtor_nogc(&op1);
}
break;
case ZEND_AST_MATCH:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

zend_const_expr_to_zval would also benefit from being updated to support ZEND_AST_MATCH, but it'd only matter if this were to be supported in constant expressions

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

Successfully merging this pull request may close these issues.

3 participants