Fix error reported on the wrong line

[VincentLanglet]

Closes phpstan/phpstan#14150

[phpstan-bot]

You've opened the pull request against the latest branch 2.2.x. PHPStan 2.2 is not going to be released for months. If your code is relevant on 2.1.x and you want it to be released sooner, please rebase your pull request and change its target to 2.1.x.

[VincentLanglet]

The failure on phpstan-doctrine seems legit since now it's report on getQuery and not on the start of the line
https://github.com/phpstan/phpstan-doctrine/blob/734ef36c2709b51943f04aacadddb76f239562d3/tests/Rules/Doctrine/ORM/data/query-builder-dynamic.php#L43

But maybe this behavior (and the initial PR #4967) should be behind bleedingEdge ?

[phpstan-bot]

This pull request has been marked as ready for review.

[staabm]

Do we still need this line?

[VincentLanglet]

Yes, because

• I compute the line from the node only if it's not set and this Check set the line.
• If I remove the ->line(...) calls, since the AttributesCheck calls the functionCallParametersCheck with another Node than the one from the ClassAttributesRule it makes a test from ClassAttributesRuleTest to fail.

[staabm]

I just had a look thru the codebase.

we have already PhpDocLineHelper::detectLine() which is called like line(PhpDocLineHelper::detectLine($node, $phpDocTag)) .

this makes me think we should not have this special fallback case here, but call something like NodeChainLineHelper::detectLine($node) right at the ErrorBuilder call-sites where ->line() is invoked.

[VincentLanglet]

this makes me think we should not have this special fallback case here, but call something like NodeChainLineHelper::detectLine($node) right at the ErrorBuilder call-sites where ->line() is invoked.

Lot of existing rules are not calling line() and rely on the defaut fallback which was already implemented in RuleErrortransformer $node->getStartLine().

There is tons of rule to fix, so it would be easier to modify the fallback.

Even binary operator are wrong
https://phpstan.org/r/090b54c3-0255-4027-9934-491d8ab962b7

And yes PipeOperator has the same issue too https://phpstan.org/r/7ab46b15-18c9-4d5b-bfc9-27bc78efe3d4

But there is so much to fix, I feel like we should first:

• validate a strategy with ondrej
• Fix/Implement it, step by step

I start to feel like we should fix rules one by one...

[staabm]

agree - thanks for looking into it

[VincentLanglet]

I tried another strategy with af0f984

I feel like it's safer to avoid a general strategy in RuleTransformer and instead to fix rules one by one. WDYT ?

[staabm]

yes, thats what I had in mind with my comment

[phpstan-bot]

This pull request has been marked as ready for review.

[VincentLanglet]

I think it can be merged like this.

I focused on method and properties.

[staabm]

I feel this looks really good