Conversation
- `??` and `??=` errors used the start line of the whole expression, which is the first line of the left operand. When the left operand spans several lines, the error landed above the operator. - The AST does not keep the operator token, so the error now uses the last line of the left operand: the operator is on that line or below it, so a line that was already correct stays. - When `??` starts its own line, the error is still one line above the operator.
Comment on lines
+71
to
+74
| $error = RuleErrorBuilder::message($error->getMessage()) | ||
| ->identifier($error->getIdentifier()) | ||
| ->line($this->getOperatorLine($node)) | ||
| ->build(); |
Contributor
There was a problem hiding this comment.
Feels wrong to me to rebuild an error manually from another.
Can't issetCheck and/or checkUnnecessaryNullCoalesce use the right operatorLine directly ?
Author
There was a problem hiding this comment.
Done in 2902a04: IssetCheck::check() now takes an optional line and every error it builds uses it, and checkUnnecessaryNullCoalesce() sets it too, so nothing is rebuilt. isset() and empty() pass no line and report where they did before. I also added cases for an unnecessary ?? null and a property fetch spanning lines.
- IssetCheck::check() takes an optional line and passes it to every error it builds, so NullCoalesceRule no longer copies an error to change its line and nothing else is lost. isset() and empty() pass no line and keep reporting where they did. - The unnecessary `?? null` error gets the same line. - Tests cover an unnecessary `?? null` and a property fetch whose left side spans lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs phpstan/phpstan#10714
??and??=errors used the start line of the whole expression, which is the first line of the left operand. When the left operand spans several lines, as in the issue, the error lands above the operator. The errors now use the last line of the left operand.The AST does not keep the
??token, so the exact operator line is not available to rules. Of the lines the AST has, the last line of the left operand is the one that never moves a correct report:$a ??then$bon the next line$athen?? $bon the next lineSince the operator is always on that line or below it, every error that was already on the right line stays there, so existing
@phpstan-ignorecomments keep working. Using the start of the right operand instead would fix the last row but move the second one down a line, whichtestBug14213catches.The remaining case,
??at the start of a line, needs the token position. As far as I can tell that means a new rich parser visitor, which perCLAUDE.mdalso needs itsturbo-extmirror, so I kept it out of this PR. I left the details on the issue.The new test covers the issue's layout, the operator at the end and at the start of a line, a single line, and
??=on the same layouts.isset()andempty()are unchanged.