Skip to content

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Aug 11, 2023

This is a followup fix for #2531. That PR made the affected errors effectively unignorable, because it included the "in context of class ..." in path and consequently the \ from the class namespace were converted to / when the baseline was generated and so the path didn't match during analysis.

I checked and other trait errors are saved with the parent classes' path. So this PR does the same for the uninitialized readonly property errors.

I'm not sure what's the best way to write a test for this issue. Should I add it to e2e-tests.yml, or should I create a PR in phpstan/phpstan and add it to other-tests.yml?

Example

Foo.php:

<?php

namespace Readonly\Uninit\Bug;

trait Foo
{
	protected readonly int $x;

	public function foo(): void
	{
		echo $this->x;
	}

	public function init(): void
	{
		$this->x = rand();
	}
}

HelloWorld.php

<?php

namespace Readonly\Uninit\Bug;

class HelloWorld
{
	use Foo;

	public function __construct()
	{
		$this->init();
		$this->foo();
	}
}

test.neon

includes:
    - test-baseline.neon

parameters:
    level: 9

Before the fix

Baseline:

parameters:
	ignoreErrors:
		-
			message: "#^Access to an uninitialized readonly property Readonly\\\\Uninit\\\\Bug\\\\HelloWorld\\:\\:\\$x\\.$#"
			count: 1
			path: "bug/Foo.php (in context of class Readonly/Uninit/Bug/HelloWorld)"

		-
			message: "#^Readonly property Readonly\\\\Uninit\\\\Bug\\\\HelloWorld\\:\\:\\$x is assigned outside of the constructor\\.$#"
			count: 1
			path: bug/HelloWorld.php

The result of analysis with this baseline is:

 ------ ---------------------------------------------------------------------------------- 
  Line   Foo.php (in context of class Readonly\Uninit\Bug\HelloWorld)                      
 ------ ---------------------------------------------------------------------------------- 
  11     Access to an uninitialized readonly property Readonly\Uninit\Bug\HelloWorld::$x.  
 ------ ---------------------------------------------------------------------------------- 

 -- ------------------------------------------------------------------------- 
     Error                                                                    
 -- ------------------------------------------------------------------------- 
     Ignored error pattern #^Access to an uninitialized readonly property     
     Readonly\\Uninit\\Bug\\HelloWorld\:\:\$x\.$# in path                     
     /home/schlndh/devel/custom/phpstan-src/bug/Foo.php (in context of class  
     Readonly/Uninit/Bug/HelloWorld) was not matched in reported errors.      
 -- -------------------------------------------------------------------------

After the fix

Here is the diff of the baseline:

--- test-baseline.neon
+++ test-baseline-fixed.neon
@@ -3,7 +3,7 @@
 		-
 			message: "#^Access to an uninitialized readonly property Readonly\\\\Uninit\\\\Bug\\\\HelloWorld\\:\\:\\$x\\.$#"
 			count: 1
-			path: "bug/Foo.php (in context of class Readonly/Uninit/Bug/HelloWorld)"
+			path: bug/HelloWorld.php
 
 		-
 			message: "#^Readonly property Readonly\\\\Uninit\\\\Bug\\\\HelloWorld\\:\\:\\$x is assigned outside of the constructor\\.$#"

And when I run analysis there are no errors reported (as it should be).

@ondrejmirtes
Copy link
Member

I'm not sure what's the best way to write a test for this issue.

Both of your suggestions are fine :) I'd definitely like to see a failing test first, I don't yet understand the problem.

@schlndh schlndh force-pushed the fix-baselineForUninitializedTraitProperties branch from 6a45d62 to eb0c8ca Compare August 18, 2023 10:11
@schlndh
Copy link
Contributor Author

schlndh commented Aug 18, 2023

@ondrejmirtes Done. I added it as a separate commit, so that you can see the failure as well: https://github.com/phpstan/phpstan-src/actions/runs/5901436629/job/16007616894?pr=2568

The issue basically happens due to the difference in these two lines (notice the difference in slashes):

  Line   Foo.php (in context of class Readonly\Uninit\Bug\HelloWorld)                      
path: "bug/Foo.php (in context of class Readonly/Uninit/Bug/HelloWorld)"

@ondrejmirtes
Copy link
Member

Hi, when I check out your first commit, PHPStan generates this baseline:

parameters:
	ignoreErrors:
		-
			message: "#^Readonly property Readonly\\\\Uninit\\\\Bug\\\\HelloWorld\\:\\:\\$x is assigned outside of the constructor\\.$#"
			count: 1
			path: src/HelloWorld.php

When I try to run the analysis again with this baseline, it passes:

baseline-uninit-prop-trait (HEAD detached at cedb907ce) $ ../../bin/phpstan analyse --configuration test.neon
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 [OK] No errors

Then I noticed the behaviour you're describing happens only when you run PHPStan with --debug. Which suggests this is happening only because of some discrepancy between parallel and single thread mode.

I need to investigate that.

@ondrejmirtes
Copy link
Member

Oh, there's also a diffence in Access to an uninitialized readonly property Readonly\Uninit\Bug\HelloWorld::$x. not being reported at all without --debug.

@schlndh
Copy link
Contributor Author

schlndh commented Aug 18, 2023

@ondrejmirtes Thanks. I remember noticing that the error was not reported in some scenario, but I didn't look into it further. In any case, this discrepancy is probably an unrelated issue.

I had the unignorable access to uninitialized property happen to me in our code-base, where we definitely run phpstan without --debug. But if I remember correctly, there the issue didn't make it into the baseline, but it did make it into the normal analysis. 🤔

@ondrejmirtes
Copy link
Member

Hi @schlndh, first and foremost, I need to say that I'm impressed with the level of your contributions and I'm looking forward to seeing more of them! I'm not saying this lightly, this definitely does not happen every day :)

I made a few adjustments and brought this PR over the finish line.

Turns out the --debug/no --debug problem had the same root reason and broke not just the baseline, but the result cache as well. Your PR fixes it all :)

Thank you.

@ondrejmirtes ondrejmirtes force-pushed the fix-baselineForUninitializedTraitProperties branch from 6baf1cf to c3e57a0 Compare August 19, 2023 11:19
@ondrejmirtes ondrejmirtes merged commit 049ca32 into phpstan:1.10.x Aug 19, 2023
@schlndh schlndh deleted the fix-baselineForUninitializedTraitProperties branch August 19, 2023 11:38
@schlndh
Copy link
Contributor Author

schlndh commented Aug 19, 2023

@ondrejmirtes Nice, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants