phan / phan

Phan is a static analyzer for PHP. Phan prefers to avoid false-positives and attempts to prove incorrectness rather than correctness.
https://github.com/phan/phan/wiki
Other
5.53k stars 359 forks source link

Create a plugin for PHPUnit library mock return types #338

Open Seldaek opened 7 years ago

Seldaek commented 7 years ago

I have this slightly nasty case here, I am not sure if this can be addressed to be honest but I thought I'd report anyway so it can be discussed.

This utility method in a test class creates a mock, and is typed as returning the proper class:

    protected function getIdentityFormatter(): FormatterInterface
    {
        $formatter = $this->createMock(FormatterInterface::class);
        // ...stuff...
        return $formatter;
    }

However since it is instantiated inside PHPUnit, based on the classname, the type gets lost and I get: PhanTypeMismatchReturn Returning type \PHPUnit_Framework_MockObject_MockObject but getIdentityFormatter() is declared to return FormatterInterface

As I said, tricky one, but maybe it can be special cased for PHPUnit mocks, or maybe we shouldn't care ;)

P.S.: while I am here, I wanted to say thanks for the awesome work on phan, it's progressed quite a bit since I first tried it about a year ago! :) Almost no false positives anymore which is great.

morria commented 7 years ago

Hi @Seldaek. I just pushed https://github.com/etsy/phan/commit/3b8dbbc5c3cbf8dcfdce0bb752275eaba8ef0eea which adds the ability to write plugins that get a chance to update the CodeBase before most analysis happens. This lets you set the return types on functions/methods so you could tell what things like createMock are doing.

I created this demo patch that shows a method C::stringToClass for which a plugin (PreAnalysisPlugin) assigns it a return type based on whats passed into it.

Does that help?

Seldaek commented 7 years ago

Not sure where to see your demo patch, but tbh I don't know if I would care enough about the couple createMock-related false positives to go the full length of creating a plugin. I suppose one could make a generic phpunit plugin handling this and perhaps other issues and then make it part of the phan distro somehow, that I might see value in. Doing it on a per-project basis doesn't seem worth the time unless you have a ton of false positives.

morria commented 7 years ago

@Seldaek: Oops, here's the patch I referred to https://gist.github.com/morria/8ea8b4fe5dceb932fc65dbbef19a76f6

TysonAndre commented 6 years ago

The next phan release (0.10.1) will introduced Phan\PluginV2\ReturnTypeOverrideCapability which should make writing this a lot easier.

The new capability can be used by an extension to hook into createMock (This may still require more work to get inheriting classes to also use that plugin)

ContextNode->resolveClassNameInContext() (Name may change) can be used to get the fully qualified class name from the Node for FormatterInterface::class

https://github.com/phan/phan/blob/master/src/Phan/Plugin/Internal/ArrayReturnTypeOverridePlugin.php

AJenbo commented 6 years ago

Would it be possible to support the phpstorm meta format for magic function? https://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata

TysonAndre commented 6 years ago

I had plans for something similar, but preferably within a single file for user defined code.

Those would be in phpdoc, within the same file

Phpstorm's model doesn't really go well with phan's model.

Also, PHPStorm's stubs don't seem to be included with composer libraries themselves, in my experience, so it's harder to use them (not sure if that would even work)

It might be possible for someone to write a third party plugin capable of parsing the phpstorm meta format for magic functions, and use that to generate ReturnTypeOverrides (E.g. provide that plugin with a config to a directory, use Config::getInstance()->get_value('plugin_phpstorm_directory') to get that directory, then list and process the directory, then return the overrides


There were plans to convert T[] to a return type of T, T to T[], etc. via a plugin or native functionality. Could also try to later add phpdoc syntax for mapping literals to varying kinds (e.g. @phan-pattern-match {$param1=="foo"} {return=FooInstance} 'foo' => FooInstance (Almost definitely not exactly that))

E.g. via something like @template, but supporting static methods, as well as basing template types on passed in param types/values

TysonAndre commented 6 years ago

Related: @template-typeof in psalm: https://github.com/vimeo/psalm/blob/master/docs/supported_annotations.md#template-and-template-typeof

TysonAndre commented 6 years ago

Vaguely related: https://github.com/phan/phan/blob/master/.phan/plugins/PHPUnitNotDeadCodePlugin.php is just used for reducing false positives in --dead-code-detection right now, but would be a starting point for any plugins that affect only unit tests.

But return type overrides (as mentioned in https://github.com/phan/phan/issues/338#issuecomment-337277001) would be more useful for this specific use case.

PregRegexCheckerPlugin uses AnalyzeFunctionCallCapability, and might be a place to start for modifying variable types after assertInstanceof, assertTrue, etc.

TysonAndre commented 5 years ago

This will be doable via stubs or phpdoc annotation as well in the next Phan release. (by excluding the original file, replace it with a modified version of the original)

Phan will also be able to infer that methods/functions create instances of the passed in class name. (phan-template, phan-param, and phan-return can be used to avoid conflicts with other tools)

Hopefully, a more user friendly way to configure this will be available in subsequent releases.

/**
 * @template T
 * @param class-string<T> $className
 * @return T
 */
public function createMock(string $className) {
}

Unrelated: Also see assertinstanceof in https://github.com/phan/phan/blob/master/.phan/plugins/PHPUnitAssertionPlugin.php (API is still being finalized)

TysonAndre commented 5 years ago

Recently, phpunit (8?) has added support for template types in createMock, but it requires intersection type support, which Phan currently doesn't have. (it also requires T of BaseType, which Phan doesn't have).

I'd have to double check if similar annotations would work when the function is excluded from analysis. Templates also don't work well when using the daemon/language server in the background.

    /**
     * Returns a test double for the specified class.
     *
     * @param string|string[] $originalClassName
     *
     * @psalm-template RealInstanceType of object
     * @psalm-param class-string<RealInstanceType>|string[] $originalClassName
     * @psalm-return MockObject&RealInstanceType
     */
    protected function createMock($originalClassName): MockObject