Add support for PHP 8.5 `#[\NoDiscard]` #4253

DanielEScherzer · 2025-08-21T06:57:47Z

No description provided.

DanielEScherzer · 2025-08-21T06:58:10Z

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

DanielEScherzer · 2025-08-21T07:00:59Z

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

ondrejmirtes · 2025-08-21T08:04:07Z

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

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.
Add a check in FunctionCallParametersCheck that would ask about this. There's already a similar check for pure functions.
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).
Flag NoDiscard attribute as an error above a void function.

DanielEScherzer · 2025-08-28T20:06:01Z

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

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?

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?

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

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 · 2025-08-29T16:25:31Z

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 · 2025-08-29T20:30:00Z

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 · 2025-08-30T08:14:48Z

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 · 2025-08-30T08:16:36Z

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 · 2025-08-30T15:15:01Z

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

DanielEScherzer · 2025-08-30T15:52:48Z

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 · 2025-09-04T07:14:29Z

@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 · 2025-09-04T08:21:27Z

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

DanielEScherzer · 2025-09-04T08:22:20Z

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 · 2025-09-04T13:19:41Z

src/Reflection/Native/NativeFunctionReflection.php

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		foreach ($this->attributes as $attrib) {
+			if ($attrib->getName() === 'NoDiscard') {


Has to work even with wrong case: https://3v4l.org/WGdBI/rfc#vgit.master

ondrejmirtes · 2025-09-04T13:19:51Z

src/Reflection/Native/NativeMethodReflection.php

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		foreach ($this->attributes as $attrib) {
+			if ($attrib->getName() === 'NoDiscard') {


ondrejmirtes · 2025-09-04T13:20:31Z

src/Reflection/Php/EnumCasesMethodReflection.php

@@ -161,4 +161,9 @@ public function getAttributes(): array
 		return [];
 	}

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		return TrinaryLogic::createYes();


Doesn't seem like that: https://3v4l.org/X1Lhl/rfc#vgit.master

ondrejmirtes · 2025-09-04T13:20:39Z

src/Reflection/Php/PhpFunctionFromParserNodeReflection.php

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		foreach ($this->attributes as $attrib) {
+			if ($attrib->getName() === 'NoDiscard') {


ondrejmirtes · 2025-09-04T13:20:45Z

src/Reflection/Php/PhpFunctionReflection.php

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		foreach ($this->attributes as $attrib) {
+			if ($attrib->getName() === 'NoDiscard') {


ondrejmirtes · 2025-09-04T13:21:07Z

src/Reflection/Php/PhpMethodReflection.php

+	public function hasNoDiscardAttribute(): TrinaryLogic
+	{
+		foreach ($this->attributes as $attrib) {
+			if ($attrib->getName() === 'NoDiscard') {


dtto

This is probably worth creating a NoDiscardHelper class

ondrejmirtes · 2025-09-04T13:29:12Z

src/Rules/AttributesCheck.php

@@ -40,6 +40,7 @@ public function check(
 		array $attrGroups,
 		int $requiredTarget,
 		string $targetName,
+		bool $isPropertyHook = false,


Doesn't have to be optional.

ondrejmirtes · 2025-09-04T13:30:26Z

src/Parser/VoidCastVisitor.php

+	public function enterNode(Node $node): ?Node
+	{
+		if ($node instanceof Void_) {
+			$this->pendingVoidCast = true;


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

DanielEScherzer force-pushed the polyfill-NoDiscard-function branch from 0b700c1 to 24db3f7 Compare August 21, 2025 07:00

DanielEScherzer added 2 commits August 28, 2025 23:41

Add support for PHP 8.5 #[\NoDiscard] on functions

74df112

Trinary logic migration

131caa6

DanielEScherzer force-pushed the polyfill-NoDiscard-function branch from d3dc3d4 to 131caa6 Compare August 28, 2025 21:01

DanielEScherzer added 5 commits August 29, 2025 23:08

Void casts, methods and static methods

ebbfcd8

fixes

0a0f24b

parse errors

cd00a6e

missing use

44d2398

another

b348995

DanielEScherzer added 2 commits August 30, 2025 17:56

New VoidCastRule

ab3c28b

NoDiscard on hooks

d177206

DanielEScherzer closed this Aug 30, 2025

DanielEScherzer deleted the polyfill-NoDiscard-function branch August 30, 2025 15:13

DanielEScherzer restored the polyfill-NoDiscard-function branch August 30, 2025 15:14

DanielEScherzer reopened this Aug 30, 2025

DanielEScherzer added 2 commits August 30, 2025 18:38

Lint exclusions, unify implementations

014a282

fixes

f4e9feb

DanielEScherzer changed the title ~~Add support for PHP 8.5 #[\NoDiscard] on functions~~ Add support for PHP 8.5 #[\NoDiscard] Aug 30, 2025

ondrejmirtes requested changes Sep 4, 2025

View reviewed changes

Add support for PHP 8.5 #[\NoDiscard] #4253

Are you sure you want to change the base?

Add support for PHP 8.5 #[\NoDiscard] #4253

Conversation

DanielEScherzer commented Aug 21, 2025

Uh oh!

DanielEScherzer commented Aug 21, 2025

Uh oh!

DanielEScherzer commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Aug 21, 2025

Uh oh!

DanielEScherzer commented Aug 28, 2025

Uh oh!

ondrejmirtes commented Aug 29, 2025

Uh oh!

DanielEScherzer commented Aug 29, 2025

Uh oh!

ondrejmirtes commented Aug 30, 2025

Uh oh!

ondrejmirtes commented Aug 30, 2025

Uh oh!

DanielEScherzer commented Aug 30, 2025

Uh oh!

DanielEScherzer commented Aug 30, 2025

Uh oh!

DanielEScherzer commented Sep 4, 2025

Uh oh!

ondrejmirtes commented Sep 4, 2025

Uh oh!

DanielEScherzer commented Sep 4, 2025

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add support for PHP 8.5 `#[\NoDiscard]` #4253

Add support for PHP 8.5 `#[\NoDiscard]` #4253

DanielEScherzer commented Aug 21, 2025 •

edited

Loading