Larastan: Unregistered service provider is executed during analysis

[kwebble]

• Larastan Version: 0.6.4
• --level used: 8

Description

During analysis of a package, processing stops because a LogicException is thrown in a provider. This is strange because it happens in code I wrote. That code should not be executed, just analyzed statically.

The provider is a test class that extends the actual exposed provider. It exists to test error handling of the provider and does it by creating a situation that triggers the exception.

The test provider is not registered and not exposed through composer.json.

Excluding the file from analysis does not solve the situation, the exception is still thrown.

[szepeviktor]

Thank you for your report Rob!

Could you provide any snippets we can work with?

[mfn]

That code should not be executed, just analyzed statically

This is an incorrect assumption of larastan because it actively boots your Laravel installation to extract more information at runtime to aid the static analysis.

Also not a fan per se of this but we do get lots of value back from this, so 🤷‍♀️

[szepeviktor]

Also not a fan per se of this

PHPStan itself recently worked dynamically! Analysis became static couple of releases ago: v0.12.26

[kwebble]

In the code of Larastan I saw that ApplicationResolver::resolve() does a search for concrete classes that extend \Illuminate\Support\ServiceProvider.
When the method isServiceProvider() is changed and always returns FALSE the error does not occur. But that's probably not what you want to happen for all providers.

Included is a short version of my code. It's an abstract base class FrameServiceProvider. Child classes in other packages can indicate they provide Laravel migrations by calling hasMigrations().

Those migrations are expected in a standardized directory. When booting a provider that includes migrations, a check is done to see if the required directory for those migrations exists.

If the subdirectory does not exist an exception if thrown. This situation is triggered by Test1Provider

    abstract class FrameServiceProvider extends ServiceProvider {
        private  bool $migrations = FALSE;

        public function boot (
        ): void {
            if ($this->migrations) $this->registerMigrations();
        }

        protected function hasMigrations (
        ): void {
            $this->migrations = TRUE;
        }

        private function registerMigrations (
        ): void {
            // Calculate $path for migrations based on static::class

            if (FALSE === $path) {
                throw new LogicException(
                    sprintf(
                        'Directory with database migrations %s does not exist.',
                        $path));
            }

            $this->loadMigrationsFrom($path);
        }
    }

    class Test1Provider extends FrameServiceProvider {
        public function __construct (
            $app
        ) {
            parent::__construct($app);
            $this->hasMigrations();
        }
    }

[mfn]

PHPStan itself recently worked dynamically! Analysis became static couple of releases ago: v0.12.26

@szepeviktor yes, but IMHO "runtime reflection" via autoloading vs. actually booting framework are not the same thing. Just IMHO 🤷‍♀️ 😄

[kwebble]

That code should not be executed, just analyzed statically

This is an incorrect assumption of larastan because it actively boots your Laravel installation to extract more information at runtime to aid the static analysis.

@mfn it's not just an assumption, this comes from Larastan's README.md:

Larastan focuses on finding errors in your code without actually running it.
...
Discovers bugs in your code without running it

[mfn]

@kwebble good point, never read the readme I guess ;)

[canvural]

Hi,

Yeah like others mentioned we bootstrap the framework to help us during the analysis. Not so good idea but otherwise we would have to write a huge amount of code to do the same thing statically. So there are some compromises.

So looks like there should be more logic in the https://github.com/nunomaduro/larastan/blob/master/src/ApplicationResolver.php class to handle this case.

I'd accept a PR that fixes this issue and doesn't break the current test suite.

[kwebble]

Her are some options I can think of:

Option 1: scan composer.json and only load classes defined in extra.laravel.providers. That would be enough for my project, but it has more effects:

1. it changes current behavior because not all service providers will be loaded.
2. It doesn't work for analyzing a Laravel application, its composer.json does not define the service providers.

Option 2: add a configuration to define which service providers to load, or to ignore.

Option 3: keep current detection method to scan the autoloader, but add a check to filter out classes that are located in directories under autoload-dev in composer.json. Then only service providers that are part of the application will be registered during analysis.

@canvural what do you think about these options?

I might try to implement option 1 or 3.

[kwebble]

For now I have added a custom version of the NunoMaduro\Larastan\ApplicationResolver class to my package.

A rule in the autoloader-dev section of composer.json configures the autoloader to use my version instead of the implementation of Larastan:

"autoload-dev": {
    "psr-4": {
        "NunoMaduro\\Larastan\\": "src/main/other/nunomaduro/larastan"
    }
}

[canvural]

I think option 3 you described is the best. :+1:

[kwebble]

I also prefer option 3.

For now I have a fix by using the custom ApplicationResolver, but I prefer to have a solution in the package. If I have some time I'll try to implement option 3.

[kwebble]

On my machine I have a new version of NunoMaduro\Larastan\ApplicationResolver that solves the problem.

When I run composer test on my machine (Windows, PHP 7.4.9) some tests fail. But they also fail on the original code I cloned, so I don't think it's because of my changes.

The errors are from PHPUnit tests and look database related, they mention models and query. This is part of the output:

1) Tests\Rules\ModelPropertyRuleTest::testModelPropertyRuleOnBuilder
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (

• 3 => 'Property 'foo' does not exist...model.'
• 4 => 'Property 'foo' does not exist...model.'
• 8 => 'Property 'foo' does not exist...model.'
• 18 => 'Property 'foo' does not exist...model.'
• 23 => 'Property 'foo' does not exist...model.'
• 25 => 'Property 'foo' does not exist...model.'
• 26 => 'Property 'foo' does not exist...model.'
• 27 => 'Property 'foo' does not exist...model.'
• 30 => 'Property 'foo' does not exist...model.'
  )

C:\source\github\larastantests\Rules\ModelPropertyRuleTest.php:16

Other tests that fail:

2) Tests\Rules\ModelPropertyRuleTest::testModelPropertyRuleOnRelation
3) Tests\Rules\ModelPropertyRuleTest::testModelPropertyRuleOnModel
4) Tests\Rules\ModelPropertyStaticCallRuleTest::testModelPropertyRuleOnStaticCallsToModel
5) Tests\Rules\ModelPropertyStaticCallRuleTest::testModelPropertyRuleOnStaticCallsInClass
6) Tests\Rules\NoUnnecessaryCollectionCallRuleTest::testNoFalseNegativesEloquent
7) Tests\Rules\NoUnnecessaryCollectionCallRuleTest::testNoFalseNegativesQuery

Is this something known that I can ignore or do I need to fix this first?

[canvural]

I think this is an expected/known issue in Windows. There was somebody else who got the same issue.

For this issue, there is no need to fix it. You can ignore.

But if you have some spare time, I'd be grateful if you could investigate this error ! I don't use Windows and don't have access to a Windows machine, so I have no idea why it fails.

[kwebble]

Had a quick look at the test errors. The filename passed to findErrors In Tests\RulesTest uses a mix of Windows and Unix directory separators, for example:

C:\source\github\larastan\tests\Rules/Data/model-property-builder.php

This is different from the keys of $this->execLarastan($filename)['files'] which uses Windows separators only:

C:\source\github\larastan\tests\Rules\Data\model-property-builder.php

Adding realpath() to line 28 in Rulestest solves these errors for me:

$errors = $this->findErrors(realpath($filename));

[canvural]

Great! Can you make a separate PR for that?

[kwebble]

Perhaps, I'm having trouble with GitHub Desktop showing errors when trying to send the change to GitHub. I'll try again tomorrow.

[kwebble]

For a pull request I have to supply information that I don't know, because I don't know enough of the code to know the effects of the change.

Normally I would have added some tests, but I cant find a unit test for ApplicationResolver. And to me it looks like it's not easy to create a test for.

The change is available in this branch, can't you get it from there?

[canvural]

You can create the PR, no need to fill in everything in the template. Then we will also see if it broke something in the pipeline.

[canvural]

Released as https://github.com/nunomaduro/larastan/releases/tag/v0.6.8 Please try again with the new version.

[kwebble]

This solves the error during analysis for me.