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

concat without assigment #10389

Closed
staabm opened this issue Jan 5, 2024 · 10 comments
Closed

concat without assigment #10389

staabm opened this issue Jan 5, 2024 · 10 comments

Comments

@staabm
Copy link
Contributor

staabm commented Jan 5, 2024

Feature request

today I stumbled over a bug, in which someone missed to echo the result of a string-concatenation, effectively producing a useless concatenation.

would be great if phpstan could error about the concat beeing useless:
https://phpstan.org/r/b76066d6-5fb9-440b-a1f2-16651b6a062d

same could be true for other operations when no assignment is done
https://phpstan.org/r/ae8ba301-436a-4945-aa39-181d7536620c

Did PHPStan help you today? Did it make you happy in any way?

No response

@lulco
Copy link

lulco commented Jan 6, 2024

I'd do this as rule for BinaryOp using its attribute parent, but this attribute is no longer available. What is current best practice?

@lulco
Copy link

lulco commented Jan 6, 2024

I found ParentStmtTypesVisitor and it seems to be similar to what I want. So is this the way? Write visitor first and then use attribute? It looks I will need just to check if ParentNodeType is PhpParser\Node\Stmt\Expression

@staabm
Copy link
Contributor Author

staabm commented Jan 6, 2024

At least thats what I had in mind on how to implement it.

Create a new visitor and store in attributes what is required to make the rule work

@lulco
Copy link

lulco commented Jan 6, 2024

I've implemented it this way, please check phpstan/phpstan-src#2858

@ondrejmirtes
Copy link
Member

One way to look at this feature request is that it needs to be implemented in NoopRule where similar nodes are handled. And it would lead to simple code like 'a' . 'b'; being reported.

So it can definitely be added there in NoopRule.

But the underlying issue is much more complex. When you consider 'No'. $this->doSomething() you need to make sure that doSomething method is pure. And you need to actually implement that for all expression types.

And at that point this feature request should be implemented as well: #4426 (reporting that @phpstan-pure above a function isn't true because the function does something impure).

This needs to be done in NodeScopeResolver and be decided for all expressions and statements.

ExpressionResult and StatementResult need to get a new method describing whether they are pure or not.

Last time this was done was with "throw points". If you Ctrl+F in NodeScopeResolver for throwPoints, it yields about 400 results. That's the scope if this task :)

I'd appreciate if someone tackled this, in small steps.

@lulco
Copy link

lulco commented Jan 6, 2024

I see it is not that simple I thought. NodeScopeResolver is out of my knowledge :)

@ondrejmirtes
Copy link
Member

Done as part of bleeding edge thanks to impure points: phpstan/phpstan-src@a647052

Please note that functions/methods need @phpstan-pure annotation to make sure this is detected correctly.

@staabm
Copy link
Contributor Author

staabm commented Apr 3, 2024

@ondrejmirtes nice, thank you.

one note: I wonder why line 10 did not error:
https://phpstan.org/r/24869b6d-8b20-4542-a763-a4192e044b09

all other lines look great

@ondrejmirtes
Copy link
Member

Because it might throw DivisionByZeroError. So it's not really "side effect-free" if you want to perform it in order to throw an exception.

Copy link

github-actions bot commented May 5, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants