Autoload source locator startLine check can cause a file to not be autoloaded

[mglaman]

Bug report

If a ClassLike node – Traits, Class, Interface – has any code before the definition it causes a start line mismatch and the file is not located. For example, a file may have a conditional to set a class alias due to various reasons.

This was uncovered while working on mglaman/phpstan-drupal#149.

The start line for the trait is line 17, however PHP internal reflection says the start line of the file is 10. This makes sense as line 10 has logic conditions. But PHPStan exits due to the fact the identifier starts on line 17. So PHPStan should properly check against the node name start line not the file logic start time.

if ($startLine !== $classNode->getNode()->getStartLine()) {
  continue;
}

Should look more like

if ($startLine !== $classNode->getNode()->name->getStartLine()) {
  continue;
}

Code snippet that reproduces the problem

Due to it relating to autoloading, I cannot think of a way to test on the Playground. But here's the code in general.

<?php

namespace Drupal\Tests;

use Drupal\TestTools\PhpUnitCompatibility\RunnerVersion;

// In order to manage different method signatures between PHPUnit versions, we
// dynamically load a compatibility trait dependent on the PHPUnit runner
// version.
if (!trait_exists(PhpunitVersionDependentTestCompatibilityTrait::class, FALSE)) {
  class_alias("Drupal\TestTools\PhpUnitCompatibility\PhpUnit" . RunnerVersion::getMajor() . "\TestCompatibilityTrait", PhpunitVersionDependentTestCompatibilityTrait::class);
}

/**
 * Makes Drupal's test API forward compatible with multiple versions of PHPUnit.
 */
trait PhpunitCompatibilityTrait {

Expected output

It says the symbol is not found as the autoloader exits due to mismatched start lines. It should find the file since it loaded the source.

[mglaman]

I triaged this during a live stream code session with debugging if that would help explain the issue.

[ondrejmirtes]

Hi, I don't fully understand this problem, and I'm not sure that the proposed fix is correct.

You're saying:

The start line for the trait is line 17, however PHP internal reflection says the start line of the file is 10. This makes sense as line 10 has logic conditions. But PHPStan exits due to the fact the identifier starts on line 17.

I don't follow the "This makes sense as line 10 has logic conditions." part. Class reflection always points to the class definition, i.e. line 17. Verified here: https://3v4l.org/uTXdp

What's actually on line 10 in the original trait is the class_alias call. But I wasn't able to replicate that issue either, both calls point to line 3 here: https://3v4l.org/qFuiS

So is that something specific to aliasing traits? 🤔

[mglaman]

So, I'll work on a test and explain more later today. But one thing I did realize: the file which caused issues only has problems with the autoload source locator. If it's discovered with scanDirectories everything is fine.

I haven't looked yet, but I wonder if that locator has the same checks on start line? And is that why this has been near impossible to reproduce? I wrote tests months ago for this but could never reproduce the problem. But the tests don't try to autoload the fixture files, which explains why the bug didn't surface. It's specifically in the autoload locator.

I'll work on this more today.

[mglaman]

Well, I just don't get it. I made an updated example: https://3v4l.org/IfhZY and https://3v4l.org/5HOOW. Part of my feels like it's not reproducible unless using separate files. Because the reflection from PHP had the start line not at the trait definition.

I do know the bug we're experiencing with this funky aliased trait is fixed when discovered with scanDirectories. The only difference is that the autoloader checks the start lines before passing to $nodeToReflection->__invoke, whereas the OptimizedSingleFileSourceLocator and OptimizedDirectorySourceLocator do without validating the start lines.

It was added in this commit: phpstan/phpstan-src@e2fe1ef.

I don't know why this randomly started breaking, honestly. That trait has been there for quite some time.

• Trait added Oct 2019: https://git.drupalcode.org/project/drupal/-/commit/30e8f31b35dfe2f8205bf4f03bf835fca6ecb95d
• Start line check added Jun 2020: phpstan/phpstan-src@e2fe1ef

The weird part is that things stopped working at 0.12.43. So I'm going to comb over phpstan/phpstan-src@0.12.42...0.12.43 and try to see. I'm really at a complete loss. Like was it something in \PHPStan\Reflection\BetterReflection\SourceLocator\FileNodesFetcher::fetchNodes and how it's parsed? I think I'm getting closer.

[mglaman]

Okay, so \PHPStan\Parser\Parser::parseFile returned the invalid start line of 10.

[mglaman]

Going to apologize for the comment spam – but I'm trying to deep dive this between calls 😬 .

Got to \PHPStan\Parser\RichParser::parseString and the file was parsed with the incorrect start line. The parser is \PhpParser\Parser\Php7

So, I'm guessing it is something due to nikic/php-parser when it was bumped in phpstan/phpstan-src@ff092a0.

[mglaman]

Ugh, okay. The bug belongs in nikic/php-parser, not here.

[mglaman]

I'll close this in favor of the php-parser issue: nikic/PHP-Parser#738

[ondrejmirtes]

Hi, thank you for taking the time, I appreciate it :) Don't worry about spamming here, I have a similar procedure when investigating issues :D

[mglaman]

It looks like it may be due to the handling of optional attributes in traits. I finally have a failing test somewhere for this: nikic/PHP-Parser#739. Hopefully, we can find a fix that gets released, PHPStan can use it, and phpstan-drupal is finally unblocked 🥳

[ondrejmirtes]

Nothing stands out to me in the trait file, what are optional attributes in traits?

[mglaman]

I'm guessing it's something in the parsing of PHP 8 attributes. I just took an assumption from nikic's comment

Probably related to empty optional attributes before the trait.

So there's nothing wrong with the trait, just the parsing of it and supporting PHP 8 attributes. I think. And I think I know where.

[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.