Fix fatal error on `constant('')` #3013

staabm · 2024-04-12T19:12:09Z

there are only 2 other callers for this very same method in the codebase, and there we already early exit on '':

e.g.

phpstan-src/src/Type/Php/DefinedConstantTypeSpecifyingExtension.php

Lines 50 to 60 in 5c3a711

    
           if ( 
        
           	!$constantName instanceof ConstantStringType 
        
           	|| $constantName->getValue() === '' 
        
           ) { 
        
           	return new SpecifiedTypes([], []); 
        
           } 
        
           return $this->typeSpecifier->create( 
        
           	$this->constantHelper->createExprFromConstantName($constantName->getValue()), 
        
           	new MixedType(), 
        
           	$context,

phpstan-bot · 2024-04-12T19:21:21Z

This pull request has been marked as ready for review.

ondrejmirtes

I'd like to have a stub for Name constructor and all Name subclasses constructors that enforces non-empty-string.

fix fix fix fix fix Update Type.php Revert "Update Type.php" This reverts commit a239561a19109a244f370a59532891959438f0eb. fix tests fix fix Update phpstan-baseline.neon

staabm · 2024-04-12T20:30:48Z

using non-empty-string names everywhere would spread across the whole codebase. I fixed the places which worked without ugly catches and baselined the remaining ones

staabm · 2024-04-12T20:35:48Z

tested locally with regarding __construct() in all known Name classes

➜  phpstan-src git:(bug10867) ✗ cat test4.php 
<?php

$n = new \PhpParser\Node\Name('');

$r = new \PhpParser\Node\Name\Relative('');

$fq = new \PhpParser\Node\Name\FullyQualified('');
➜  phpstan-src git:(bug10867) ✗ php bin/phpstan analyze test4.php --debug
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
/Users/staabm/workspace/phpstan-src/test4.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  Line   test4.php                                                                                                                                                   
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  3      Parameter #1 $name of class PhpParser\Node\Name constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.                 
         💡 Type #1 from the union: '' is empty.                                                                                                                     
  5      Parameter #1 $name of class PhpParser\Node\Name\Relative constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.        
         💡 Type #1 from the union: '' is empty.                                                                                                                     
  7      Parameter #1 $name of class PhpParser\Node\Name\FullyQualified constructor expects non-empty-array<string>|PhpParser\Node\Name|non-empty-string, '' given.  
         💡 Type #1 from the union: '' is empty.                                                                                                                     
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 


                                                                                                                        
 [ERROR] Found 3 errors

also created a upstream PR for PHPParser: nikic/PHP-Parser#993

ondrejmirtes · 2024-04-12T20:39:16Z

using non-empty-string names everywhere would spread across the whole codebase.

Yes, that's the idea. I'm not looking for a quick fix but a correct fix. Please do not baseline anything. Baseline is not supposed to grow.

staabm · 2024-04-13T06:42:50Z

looks like this will be a breaking change and can only be merged for 2.0?
should the crash fix be separated?

ondrejmirtes

Please be more careful - in some places where we gain type safety thanks to the new PHPDocs, you instantly throw it away by throwing ShouldNotHappenException.

ondrejmirtes · 2024-04-14T13:19:56Z

conf/config.neon

@@ -175,6 +175,7 @@ parameters:
 		- ../stubs/ext-ds.stub
 		- ../stubs/ImagickPixel.stub
 		- ../stubs/PDOStatement.stub
+		- ../stubs/PhpParserName.stub


Please register this stub only in build/phpstan.neon. It's only for internal PHPStan purposes.

And move the stub elsewhere, probably to build/stubs.

ondrejmirtes · 2024-04-14T13:22:52Z

src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php

@@ -245,7 +245,16 @@ private function processKeyAndItemType(MutatingScope $scope, Type $keyType, Type
 	private static function createFunctionName(string $funcName): Name
 	{
 		if ($funcName[0] === '\\') {
-			return new Name\FullyQualified(substr($funcName, 1));
+			$fqcn = ltrim($funcName, '\\');
+			if ($fqcn === '') {


You can definitely write some userland code that will execute this condition

ondrejmirtes · 2024-04-14T13:22:56Z

src/Type/Php/ArrayFilterFunctionReturnTypeReturnTypeExtension.php

+		}
+
+		if ($funcName === '') {
+			throw new ShouldNotHappenException();


You can definitely write some userland code that will execute this condition

ondrejmirtes · 2024-04-14T13:24:09Z

src/Type/Php/ConstantHelper.php

-			$classConstName = new FullyQualified(ltrim($classConstParts[0], '\\'));
+			$fqcn = ltrim($classConstParts[0], '\\');
+			if ($fqcn === '') {
+				throw new ShouldNotHappenException();


What if you can constant('::aa')? That will execute this condition

ondrejmirtes · 2024-04-14T13:24:43Z

src/Rules/Generics/TemplateTypeCheck.php

@@ -66,6 +67,10 @@ public function check(
 	{
 		$messages = [];
 		foreach ($templateTags as $templateTag) {
+			if ($templateTag->getName() === '') {


This should be tackled in TemplateTag by typehinting non-empty-string.

ondrejmirtes · 2024-04-14T13:25:08Z

src/Reflection/InitializerExprContext.php

@@ -127,6 +138,10 @@ public static function fromStubParameter(

 	public static function fromGlobalConstant(ReflectionConstant $constant): self
 	{
+		if ($constant->getNamespaceName() === '') {
+			throw new ShouldNotHappenException('Namespace cannot be empty.');


This could be tackled by a better typehint in BetterReflection.

Fix fatal error on constant('')

8079876

staabm marked this pull request as ready for review April 12, 2024 19:21

ondrejmirtes requested changes Apr 12, 2024

View reviewed changes

PhpParser Name requires a non-empty-string or non-empty-array<string>

2561a62

fix fix fix fix fix Update Type.php Revert "Update Type.php" This reverts commit a239561a19109a244f370a59532891959438f0eb. fix tests fix fix Update phpstan-baseline.neon

staabm force-pushed the bug10867 branch from 554e941 to 2561a62 Compare April 12, 2024 20:27

toCodeString() also returns non-empty-string

9b1b043

staabm added 2 commits April 12, 2024 22:45

Discard changes to phpstan-baseline.neon

3b5cf54

fix

337342c

staabm mentioned this pull request Apr 12, 2024

Declare more precise phpdoc types nikic/PHP-Parser#993

Merged

staabm added 5 commits April 12, 2024 23:14

getObjectClassNames()

1de8bff

Update ReflectionProviderGoldenTest.php

4a68879

Update NodeScopeResolver.php

26d20dc

namespace cannot be empty-string

f41fd91

exception message

c90e17c

ondrejmirtes requested changes Apr 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fatal error on `constant('')` #3013

Fix fatal error on `constant('')` #3013

staabm commented Apr 12, 2024 •

edited

phpstan-bot commented Apr 12, 2024

ondrejmirtes left a comment

staabm commented Apr 12, 2024

staabm commented Apr 12, 2024 •

edited

ondrejmirtes commented Apr 12, 2024

staabm commented Apr 13, 2024

ondrejmirtes left a comment

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

ondrejmirtes Apr 14, 2024

	if (
	!$constantName instanceof ConstantStringType
	\|\| $constantName->getValue() === ''
	) {
	return new SpecifiedTypes([], []);
	}

	return $this->typeSpecifier->create(
	$this->constantHelper->createExprFromConstantName($constantName->getValue()),
	new MixedType(),
	$context,

Fix fatal error on constant('') #3013

Are you sure you want to change the base?

Fix fatal error on constant('') #3013

Conversation

staabm commented Apr 12, 2024 • edited

phpstan-bot commented Apr 12, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Apr 12, 2024

staabm commented Apr 12, 2024 • edited

ondrejmirtes commented Apr 12, 2024

staabm commented Apr 13, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

ondrejmirtes Apr 14, 2024

Choose a reason for hiding this comment

Fix fatal error on `constant('')` #3013

Fix fatal error on `constant('')` #3013

staabm commented Apr 12, 2024 •

edited

staabm commented Apr 12, 2024 •

edited