[CLEANUP] Rely on PHP to detect access to uninitialized properties

[oliverklee]

In our code, this exception that indicated that the property was access before initialization is never thrown.

So we can simplify our code by removing the check (and the exception) and make use of PHP's own mechanism for throwing an error if the property is accessed before initialization.

[coveralls]

coverage: 98.246% (+0.6%) from 97.669% — task/drop-throw/4 into main

[JakeQZ]

Looks good. I've suggested a couple of assert()s which may be worthwhile additions if you agree.

coverage: 98.244% (+0.6%) from 97.672% — task/drop-throw/4 into main

Not looked into the reports - do they indicate which lines of code are not exercised in tests? (It's impossible to test code which can never be executed.)

Can we get to 100%? Back-of-an-envelope caclulation suggests there might just be three other impossible assert-throwing branches to eliminate to get us there...

[JakeQZ]

Perhaps we should add after this line:

\assert($instance->xPath instanceof \DOMXPath);

This would maybe make it easier to debug any potential breakages in development, pinpointing the point of failure, rather than having to diagnose a less fathomable failure further down the line.

[JakeQZ]

Ditto re assert().

[JakeQZ]

We can similarly remove the nulls from this property - but as a separate PR (and perhaps likewise also add assertions for it being initialized).

[oliverklee]

#1621

[oliverklee]

Not looked into the reports - do they indicate which lines of code are not exercised in tests? (It's impossible to test code which can never be executed.)

Can we get to 100%? Back-of-an-envelope caclulation suggests there might just be three other impossible assert-throwing branches to eliminate to get us there...

We'll be able to come very near. I'll create a ticket for raising the code coverage with a list of not-yet-covered parts.

[JakeQZ]

Looks good. I've suggested a couple of assert()s which may be worthwhile additions if you agree.

I just checked, and PHPStan would give an error if such assert()s were added. However, we could set treatPhpDocTypesAsCertain to false in phpstan.neon to allow us to assert() complete construction. WDYT?

[oliverklee]

However, we could set treatPhpDocTypesAsCertain to false in phpstan.neon to allow us to assert() complete construction. WDYT?

I'd rather not.

In the production code, I'd prefer to keep doing this:

• add/keep exceptions for error cases that can actually happen, and cover those with tests
• add assertions for cases where we know the type etc. for sure, but PHPStan cannot deduct it (yet)
• add PHPDoc type annotations for the same kind of cases where an assert does not suffice (e.g, for generics)

Would you be okay with doing/continuing this?

[JakeQZ]

• add/keep exceptions for error cases that can actually happen, and cover those with tests
• add assertions for cases where we know the type etc. for sure, but PHPStan cannot deduct it (yet)
• add PHPDoc type annotations for the same kind of cases where an assert does not suffice (e.g, for generics)

I'm fine with all this. But using or not using treatPhpDocTypesAsCertain seems to be independent of the above, although closely related. Disabling it would allow us to catch programming mistakes with assert()s without PHPStan complaining.

E.g. suppose I added this to AbstractHtmlProcessor:

public static function fromNothing(): self
{
    $instance = new static();
    return $instance;
}

This would return an instance with null for xPath and domDocument, despite the PHPDoc saying it's not possible.

[oliverklee]

I've given treatPhpDocTypesAsCertain a go at #1622 and MyIntervals/PHP-CSS-Parser#1583. I would have expected the baseline to change a lot more, and now I'm fine with that configuration change. :-)

[oliverklee]

Rebased and added the assertions.

[JakeQZ]

I've given treatPhpDocTypesAsCertain a go at #1622 and MyIntervals/PHP-CSS-Parser#1583. I would have expected the baseline to change a lot more, and now I'm fine with that configuration change. :-)

I wonder if there's a way to get the best of both worlds, running PHPStan twice:

• once with treatPhpDocTypesAsCertain;
• a second time without, but with some other option (if one exists) to ignore assert()s that cannot fail based on the DocBlocks.

From phpstan/phpstan#12719, it seems this would require creating a PHPStan extension, which seems like quite a bit of effort for something that isn't much of an issue - though we would like to ensure our DocBlock types are correct and consistent with the code (and vice versa) when used internally.

[JakeQZ]

... it seems this would require creating a PHPStan extension, which seems like quite a bit of effort for something that isn't much of an issue - though we would like to ensure our DocBlock types are correct and consistent with the code (and vice versa) when used internally.

It doesn't look too onerous to create an extension to handle conditionalType.alwaysTrue, then look to see if it's within an assert(). Maybe slightly trickier to also cover an if ($alwaysFalse) { throw Exception() }. But once in place would allow us to use assert()s and guards without needing phpstan-ignore. WDYT?

[oliverklee]

It doesn't look too onerous to create an extension to handle conditionalType.alwaysTrue, then look to see if it's within an assert(). Maybe slightly trickier to also cover an if ($alwaysFalse) { throw Exception() }. But once in place would allow us to use assert()s and guards without needing phpstan-ignore. WDYT?

This sounds intriguing to me. I wouldn't want to put time into writing the extension, but I'd be willing to review it and see its benefits. :-)

[JakeQZ]

This sounds intriguing to me. I wouldn't want to put time into writing the extension, but I'd be willing to review it and see its benefits. :-)

I was easily able to create an extension to potentially disable some warnings, hook it in, but then unable to get context information (to disable the warning only within an assert), and unable to find relevant documentation. I logged phpstan/phpstan#14616 but will give up on this for now.

[JakeQZ]

I logged phpstan/phpstan#14616 but will give up on this for now.

Got some helpful pointers from some helpful people, so now have #1624...