Warn about possible access to typed property before initialization

[Lctrs]

Feature request

New typed properties (even nullable ones) in PHP 7.4 don't get null assigned as a default value, they are in a new uninitialized state. Trying to access an uninitialized property will raise a runtime error "Typed property Foo::$bar must not be accessed before initialization".

Could be nice if code like this one could trigger an error about it.

[muglug]

Related: #305, though this is simpler to implement – see here for an implementation in Psalm that uses a flag on the type to track whether it's been initialized.

[mfn]

Was bitten by this today due to a bug I made https://phpstan.org/r/0374c712-e2c4-40bc-b916-dd3e742360ec

<?php declare(strict_types = 1);

class Bar
{
	private ?string $foo;
	
	public function getFoo(): string {
		if (!$this->foo) {
			$this->foo = 'baz';
		}
		
		return $this->foo;
	}
}

Even the if (!…) doesn't protect in this case because it's still "uninitialized"

[icanhazstring]

@ondrejmirtes can you point us to where this should be checked in the code?
Maybe we can implement this - so you don't have to :)

[ondrejmirtes]

@icanhazstring I'm not sure what exactly do you want to check. Do you want to know about typed properties that haven't been assigned in the constructor? I've hesitated to implement such a check in PHPStan, because you can have code that doesn't fail in runtime, but would fail a PHPStan check - for example object where a setter is called before a getter.

[muglug]

@icanhazstring Psalm implements this with an initialized flag on union types, but also some slightly complex initialisation checks. Happy to go through some of the details with you in a chat.

[icanhazstring]

The code will always fail if you don't have a value (even null) assigned to the typed property directly, or not assigned a value in the constructor.

For example this will always fail an should be reported.

class Test
{
    private ?string $value;

    public function setValue(?string $value)
    {
        $this->value = $value;
    }

    public function getValue(): ?string
    {
        return $this->value;
    }
}

It doesn't matter what function you call (getValue, setValue) the execution will always fail.

-- edit:
I am wrong about setValue, this works properly ofc

[muglug]

See https://psalm.dev/articles/psalm-supports-php-7-4#prevent-property-access-before-initialisation for an overview

[icanhazstring]

@muglug exactly this would be nice to have for phpstan as well 👍

[ondrejmirtes]

I agree it'd be useful but it would also lead to unnecessary false positives. Even some code in PHPStan itself (that uses getters to prevent circular dependency problem in constructor) would fail this check. But we can add it as bleeding edge feature and if no one complains, it can be enabled in next major (1.0).

[muglug]

I warn you it's not a simple feature to implement – Psalm's property initialisation checks touch a lot of the code, but here is the entrypoint: https://github.com/vimeo/psalm/blob/ada2fe033e14f3b9feccee78371baed456681b48/src/Psalm/Internal/Analyzer/ClassAnalyzer.php#L1133

[ondrejmirtes]

@muglug Could you tell us in words what are the edge-cases to solve? My idea is:

1. Get properties with native type from the class
2. Subtract properties with default value
3. Go through properties assigned in constructor, subtract those
4. Report the result
5. Additionally, properties accessed in constructor before they're assigned, can also be reported

Also, what should we do about static properties?

[dktapps]

IMO statics without a default should always be considered uninitialized (unless assigned earlier in the same block).

[icanhazstring]

I think this would sum it up:
"If there is no write access on a property before read => report error"

1. __construct with the property is immediate write access
2. every thing else assigning a value to the property
3. the property already has a value by declaration inside the class

--
But I would agree that this might no be trivial. For me at least as I don't know the exact way how phpstan handles the scanning :)

[muglug]

The edge-cases are properties set in methods called in the constructor, which (in Psalm) requires a re-analysis of the constructor, checking for any manipulation of properties.

The most obvious example of that edge case is a constructor that calls its parent class constructor to initialise some properties, but there are many others. All the edge-cases that Psalm handles can be found in tests/PropertyTypeTest.php.

Psalm just ignores static properties atm - no easy way around those.

[ondrejmirtes]

Another annoying case is this one:

private function __construct()
{
}

public static function create(): self
{
    $self = new self();
    $self->foo = 'bar';
}

[muglug]

Also I wouldn’t be too worried about false-positives - people seem pretty happy with Psalm’s checks.

[muglug]

Another annoying case is this one

Yeah, but that’s sort of inarguably just a bad way to write PHP - you can instead do the initialisation in the private constructor, as you would in most other OOP languages, and everyone’s happy.

[icanhazstring]

Another annoying case is this one:

But even if that is the case. This is working code and should not be reported. Or am I missing something 🤔

[ondrejmirtes]

But at this point it's really hard for static analyser to figure such stuff out, at least without any performance impact...

[icanhazstring]

But at this point it's really hard for static analyser to figure such stuff out, at least without any performance impact...

I see. Ok that is the point where I am not fully familiar with the insides :)

[muglug]

But even if that is the case. This is working code and should not be reported.

I'm sure Ondřej and I could both give talks on how to write working PHP code that our tools cannot understand.

FWIW the same code fails in TypeScript and Hack.

[ondrejmirtes]

Just implemented this, in two steps:

1. Part of a previous rule I implemented yesterday is a new virtual node called ClassPropertiesNode that gathers data during the AST traversal so that this rule can be implemented without performance impact: phpstan/phpstan-src@48e5323#diff-98d6745a24a6e6a0a1850eb0f899b1ebR572-R604
2. This is the actual rule we're interested here: phpstan/phpstan-src@a1eb1f5

[icanhazstring]

Nice. Thank you 👍

[ondrejmirtes]

I'm trying this on a real-world codebase and it's really problematic. Sometimes the static analyser cannot know that the property is fine, because the object lifecycle is known only to the developer.

Properties are sometimes are also used to hand over values from one callback to the next...

I might have to make it an opt-in feature...

[ondrejmirtes]

Yes, it's going to be opt-in with checkUninitializedProperties: true: 10c4d5f

My main problem is that the behaviour doesn't match PHP implementation/behaviour. PHP doesn't require the property to be set after constructor is finished, only before the first access. It's not PHPStan's job to be more strict than the language itself.

[icanhazstring]

Hm. As I understood @muglug psalm is tracking the state of the typed property. So it raises an error if the property gets a read access before a write.

[ondrejmirtes]

I’m pretty sure it cannot be covered by static analysis capabilities.

[muglug]

It's not PHPStan's job to be more strict than the language itself.

Errr what? That’s something static analysis tools do all the time - here’s a very trivial example: https://phpstan.org/r/daaf50b4-095f-40db-a860-16cd4cfb2630

Psalm has had this for a few years, and people interested in fully-typed codebases seem to like the feature a lot. Hack (which runs on a couple of very real codebases) also has this rule.

[ondrejmirtes]

I added the feature, I just decided it's better as an opt-in.

Yeah, but strlen(5) isn't something that people want to do on purpose, same as count($nonCountable) which was accepted until recently and always returned 1, PHPStan also reported that for many years even if it was accepted in PHP.

But I don't want to report code that's perfectly fine, which makes the tool more annoying and less useful. Consider this example:

class FooPresenter extends \Nette\Application\UI\Presenter
{

    private Article $article;

    public function actionDefault()
    {
        $this->article = Article::find(1);
    }

    public function renderDefault()
    {
        echo $this->article->getTitle();
    }

}

This is perfectly valid code in Nette framework, but static analysis can't know that.

I'm just trying to do what I think is best for my users. As with security, you have to strike a balance with 100% reporting that no one would want to use with something useful and usable.

[muglug]

What’s to stop someone calling renderDefault on an uninitialised object?

[muglug]

Ah, on second thoughts it feels like the problem for your users isn't so much that it needs to be opt in – rather, you want them to be able to opt out of the checks for particular files/classes (e.g. ones named *Presenter).

That feels like a problem of granular issue handling, not of the feature itself.

[ondrejmirtes]

These classes aren't instantiated by the user, they're instantiated and called by the framework with the methods in a certain order. No one calls them by hand.

This is similar to PHPUnit's TestCases. Does Psalm consider (at least by a plugin) TestCase::setUp() to be a valid property initializer? I achieved this with the additionalConstructors setting: https://github.com/phpstan/phpstan-phpunit/blob/c6acfc9d251562c03f8693cd9127f329ba32ed39/extension.neon#L2-L3

I just don't want people to have this experience:

• PHPStan: Class X has an unitinialized property $x.
• User: But my code is fine, what do you mean?
• PHPStan: You have to set your properties in the constructor now.

I've always hesitated about this:

I've hesitated to implement such a check in PHPStan, because you can have code that doesn't fail in runtime, but would fail a PHPStan check

But when I saw a lot of examples in Slevomat's codebase that created hundreds of errors related to this feature (not just in Presenters), I decided I really don't want to put my users through this by default.

[muglug]

Does Psalm consider (at least by a plugin) TestCase::setUp() to be a valid property initializer?

No – in Psalm's own codebase I ignore the issue in the tests directory, and in most projects these property checks are turned off in Psalm by default (for example in PHPUnit's own Psalm config).

But they're turned on by default when Psalm is operating at its second-most-strict level (2), because they help people write code that's less magical than "traditional" PHP code – and whatever else, you have to admit that presenter code (like a lot of MVC code) is pretty magical.

[icanhazstring]

These classes aren't instantiated by the user, they're instantiated and called by the framework with the methods in a certain order. No one calls them by hand.

Now I understand your problem. Since you only analyze userland code (not vendor) you don't know if the code is even called.

So another thinking I had. What about reporting such an issue when the call happens in userland code?
Is this even possible to switch on/off?

So for example, your code above. This is defined in userland code and "magically" called in vendor code. So internally you could mark it as "risky". When this class gets used in userland code, you check if the read happens before a write - then raise the level from risky to error and report it.

Otherwise, if no one even calls the code in userland, you assume everything is fine.

[muglug]

Otherwise, if no one even calls the code in userland, you assume everything is fine.

But the error isn't triggered when the call $this->article->getTitle() call happens – it's triggered when the property isn't initialised in the constructor.

The purpose of having such rules – "all properties should be initialised in the constructor" – is to prevent doing more complex control-flow-based analysis for every property access in your application.

[icanhazstring]

But the error isn't triggered when the call $this->article->getTitle() call happens
Which error are we talking about? PHP will for sure raise an error that article is not initialized.

For PHPStan/Psalm - yes. But that was my point. For me it doesn't make sense to initialize everything in the constructor, this is just a "safeguard" so you don't need to keep track of those properties. You only need to keep track of them if they are not initialized in the constructor (or even by declaration).

[github-actions]

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.