Add support for PHP 8.5 #[\NoDiscard]

[DanielEScherzer]

No description provided.

[DanielEScherzer]

Wasn't able to figure out instance methods or static methods, I'll work on those separately later

[DanielEScherzer]

PHP 8.5 linting failure is unrelated, caused by yours truly in php/php-src@c416191 // php/php-src#19154

[ondrejmirtes]

This is a wrong approach. We don't need a new rule but instead:

1. Add a new method on FunctionReflection and ExtendedMethodReflection that would reflect this. Maybe something like mustUseReturnValue(): TrinaryLogic or mustNotDiscardReturnValue(): TrinaryLogic although I don't like double negatives.
2. Add a check in FunctionCallParametersCheck that would ask about this. There's already a similar check for pure functions.
3. Do not forget about (void) and add proper support for this. This means support in MutatingScope::resolveType and also NodeScopeResolver (if not already covered by the common Cast parent node type).
4. Flag NoDiscard attribute as an error above a void function.

[DanielEScherzer]

This is a wrong approach. We don't need a new rule but instead:

1. Add a new method on FunctionReflection and ExtendedMethodReflection that would reflect this. Maybe something like mustUseReturnValue(): TrinaryLogic or mustNotDiscardReturnValue(): TrinaryLogic although I don't like double negatives.

Per the docs https://phpstan.org/developing-extensions/backward-compatibility-promise

Interfaces with @api in their PHPDoc can be implemented and extended

so can this be added to FunctionReflection?

2. Add a check in FunctionCallParametersCheck that would ask about this. There's already a similar check for pure functions.

FunctionCallParametersCheck doesn't include pure anywhere in the file?

3. Do not forget about (void) and add proper support for this. This means support in MutatingScope::resolveType and also NodeScopeResolver (if not already covered by the common Cast parent node type).

Is this necessary? The (void) is for runtime suppression, and a comment can be used for suppressing the phpstan issue

4. Flag NoDiscard attribute as an error above a void function.

Is that necessary? That means a whole other rule for something that PHP would complain about at compile time anyway

[ondrejmirtes]

so can this be added to FunctionReflection?

Yes, some interfaces cannot be extended. If you try that you get an error: https://phpstan.org/r/847a20b4-ce41-4763-b7f4-aa088ea30bca

FunctionCallParametersCheck doesn't include pure anywhere in the file?

I'm sorry, I was mistaken. I had these lines in mind (

phpstan-src/src/Rules/FunctionCallParametersCheck.php

Lines 275 to 284 in 8bb2a20

	if (
	!$funcCall instanceof Node\Expr\New_
	&& !$scope->isInFirstLevelStatement()
	&& $scope->getKeepVoidType($funcCall)->isVoid()->yes()
	) {
	$errors[] = RuleErrorBuilder::message($voidReturnTypeUsed)
	->identifier(sprintf('%s.void', $nodeType))
	->line($funcCall->getStartLine())
	->build();
	}

). They produce errors like "Result of method X (void) is used." We actually need to check the opposite here.

Call to pure functions without using their result is covered by these rules:

• CallToFunctionStatementWithoutSideEffectsRule
• CallToMethodStatementWithoutSideEffectsRule
• CallToStaticMethodStatementWithoutSideEffectsRule

But I think we first should try to report this in FunctionCallParametersCheck. $scope->isInFirstLevelStatement() is great for that.

Is this necessary? The (void) is for runtime suppression

The user already expresses their intention with (void). So PHPStan should not produce this message. If you'd ask $scope->isInFirstLevelStatement() it'd be taken care of automatically.

Is that necessary? That means a whole other rule for something that PHP would complain about at compile time anyway

Yes. PHPStan users expect this to be reported. We should report all the situations from the "Constraints" section in the RFC (https://wiki.php.net/rfc/marking_return_value_as_important).

[DanielEScherzer]

So PHPStan should not produce this message. If you'd ask $scope->isInFirstLevelStatement() it'd be taken care of automatically.

I couldn't figure out using the scope, so I added a visitor to indicate method calls that use (void)
A few e2e tests are now failing, I think because of #[\NoDiscard] attributes that are now being complained about, I should be able to address that tomorrow

Yes. PHPStan users expect this to be reported. We should report all the situations from the "Constraints" section in the RFC (https://wiki.php.net/rfc/marking_return_value_as_important).

What I mean is, is this necessary for this patch? Errors about when the attribute is applied are different from errors about when the methods with the attribute are called

[ondrejmirtes]

 What I mean is, is this necessary for this patch?

@DanielEScherzer My process for adding support for new PHP features is that I read the whole RFC and I update PHPStan with everything that should be added about the feature.

If you don't do that, then I'd have to keep in mind to do it myself.

[ondrejmirtes]

Another important thing from the RFC:

As the result of a (void) cast is not defined, the (void) cast is a statement, not an expression. Using it within an expression will result in a syntax error.

Another rule checking the (void) cast with isInFirstLevelStatement would be great for that.

[DanielEScherzer]

Sorry, tried to rename the branch but didn't realize that that closes the PR :(

[DanielEScherzer]

Okay, I have

• added a rule to complain about (void) in expressions
• added a case to the existing attribute.target issues for using NoDiscard on property hooks
• fixed the handling of instance and method calls to address the false positives

[DanielEScherzer]

@ondrejmirtes my understanding is that this is awaiting review, is there something else I should do? (Don't want to nag, just want to make sure you aren't waiting for me to do something while I'm waiting for you or another reviewer)

[ondrejmirtes]

Please wait for the next review, I'm working through my queue 😊

[DanielEScherzer]

Okay, just wanted to make sure you were not waiting for something from me, take your time

[ondrejmirtes]

This is starting to get in a good shape 👍

I'm missing checking whether #[NoDiscard] is above a void-returning function/method. FunctionDefinitionCheck would be a natural place for that.

[ondrejmirtes]

dtto

This is probably worth creating a NoDiscardHelper class

[ondrejmirtes]

I don't get the need for this visitor, can you elaborate please?

[DanielEScherzer]

I wasn't able to figure out how to, within a rule, associate a node for a method call with the previous node to check if the previous node was a (void) cast, so I added a visitor to do that instead

[ondrejmirtes]

Hello, I'd like to ask: Are you going to finish this or should I take over? Thank you.

[DanielEScherzer]

I'm missing checking whether #[NoDiscard] is above a void-returning function/method. FunctionDefinitionCheck would be a natural place for that.

done, only thing not tested is a void-returning property hook since property hooks do not support type hints

Hello, I'd like to ask: Are you going to finish this or should I take over? Thank you.

I'll finish it up

---

There are some CI failures, but they seem unrelated to this?
If this looks good I'll squash it with a clear commit message

[ondrejmirtes]

I removed the visitor and no tests are failing. Can you try breaking the test suite now with some code that the visitor should have covered?

[ondrejmirtes]

FYI I'm taking over and finishing this PR. Just let me know about the visitor, thanks 😊

[ondrejmirtes]

Seems complete now. Feel free to check out my commits what additional things I added 😊

[ondrejmirtes]

Thank you!