issue #9807 refactored syntax for better understanding #10687
Conversation
| $left_var_id = '$<tmp coalesce var>' . (int) $left_expr->getAttribute('startFilePos'); | ||
| $left_var_id = '$<psalm_coalesce_temp_var>' | ||
| . (int) $left_expr->getAttribute('startFilePos') | ||
| . ': array<array-key, mixed>'; |
There was a problem hiding this comment.
Not sure why array<array-key, mixed> is hardcoded here. And it doesn't seem it adds anything useful to the message as TypeContradiction would still list the inferred type anyway.
There was a problem hiding this comment.
The hardcoded type hint array<array-key, mixed> in the variable assignment was initially introduced to provide explicit information about the expected type of the temporary variable. However, after careful consideration and feedback, it appears that this information is already effectively conveyed by TypeContradiction.
To improve code readability and adhere to the feedback received, should I make the necessary changes?
There was a problem hiding this comment.
yes, please remove it, I think it add confusion instead of reducing it
This pull request addresses readability and uniqueness issues in the CoalesceAnalyzer class. The primary focus is on improving the temporary variable name used in the transformation of the coalesce operator into a fake ternary. The changes aim to make the variable name more descriptive and less likely to conflict with existing variables in the scope.
Key Changes:
Temporary Variable Name:
Replaced the existing temporary variable name with a more descriptive and unique name: $<psalm_coalesce_temp_var>.
Added a type hint to the temporary variable: array<array-key, mixed>.
Context:
The previous naming convention ($XXX) could potentially clash with existing variables in the scope, leading to bugs. The new naming convention improves clarity and ensures uniqueness. Additionally, the type hint provides information about the expected type of the temporary variable.
These changes should enhance the readability of the code and mitigate potential issues related to variable naming conflicts in the CoalesceAnalyzer.
fixes #9807