Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declaring more precise types and purity boundaries on ext-reflection symbols in .phpstub files #8722

Commits on Dec 6, 2022

  1. Declaring more precise types and purity boundaries on `ext-reflection…

    …` symbols in `.phpstub` files
    
    Also:
    
     * added PHP 8.2 stubs
     * refined types to make impossible scenarios more clear (like `ReflectionIntersectionType#allowsNull()`)
    
    This is a first attempt at refining these types: the structure of these stubs is quite confusing to me,
    so I don't know if this approach is correct, and if the stubs are merged together, or if entire symbols
    need to be completely re-declared for each PHP version.
    Ocramius committed Dec 6, 2022
    Copy the full SHA
    322cff6 View commit details
    Browse the repository at this point in the history
  2. Marking ReflectionProperty#$name as string rather than `non-empty…

    …-string`
    
    Because @weirdan is a party pooper (they poop at the parties)
    
    Ref: https://www.youtube.com/watch?v=gjwofYhUJEM
    Ref: vimeo#8722 (comment)
    Ocramius committed Dec 6, 2022
    Copy the full SHA
    d5cccba View commit details
    Browse the repository at this point in the history
  3. Prevent usage of callable objects in ReflectionFunction::__construct()

    As per @weirdan's feedback, we can prevent
    the usage of `object` instances that
    implement `__invoke()`, as well as `array`
    callables, by declaring the ctor argument of
    `ReflectionFunction` to be either a real `Closure`,
    or a `callable-string`.
    
    While this may not be 100% of scenarios, it is a
    healthy way to identify errors in userland.
    
    Ref: vimeo#8722 (comment)
    Ocramius committed Dec 6, 2022
    Copy the full SHA
    d9a0cc5 View commit details
    Browse the repository at this point in the history
  4. Removed templated parameters from ReflectionClass#isInstance()

    These templates were leading to false positives: assuming
    an `object` is given as input, the inferred return
    type would always have been `true`, which is obviously
    not valid.
    
    Removing them is the healthier alternative, for now.
    
    Ref: vimeo#8722 (comment)
    Ocramius committed Dec 6, 2022
    Copy the full SHA
    79a1a8b View commit details
    Browse the repository at this point in the history
  5. Refine ReflectionUnionType and ReflectionIntersectionType for PHP…

    … 8.1 and PHP 8.2
    
    * in PHP 8.0, `ReflectionUnionType` is composed on `ReflectionNamedType`s
    * in PHP 8.1, `ReflectionIntersectionType` is composed of `ReflectionNamedType`s
    * in PHP 8.2, `ReflectionUnionType` is composed of `ReflectionIntersectionType|ReflectionNamedType`s
    
    Slight variations for each PHP version.
    
    As per local testing, this doesn't work yet.
    
    ## Local testing setup:
    I did some digging to make sure that the stubs work as expected.
    
    Here's what I did to validate this patch locally (since I don't think it can really be fully automated)
    
    ## Create a dummy file to verify used symbols
    
    ```php
    <?php
    
    namespace Testing;
    
    /** @return \ReflectionClass<\stdClass> */
    function getAClass(): \ReflectionClass { throw new \Exception('irrelevant'); }
    function getAnUnionType(): \ReflectionUnionType { throw new \Exception('irrelevant'); }
    function getAnIntersectionType(): \ReflectionIntersectionType { throw new \Exception('irrelevant'); }
    
    // verifying that `getName()` is stubbed in all versions: this should always be a `class-string<\stdClass>`
    $name = getAClass()->getName();
    // union types should appear starting with PHP 8.0. Starting with PHP 8.2, they allow for intersections.
    $unionTypes = getAnUnionType()->getTypes();
    // intersection types should appear starting with PHP 8.1
    $intersectionTypes = getAnIntersectionType()->getTypes();
    
    $results = [$name, $unionTypes, $intersectionTypes];
    
    /** @psalm-trace $results */ // tracing this will show us the differences between versions
    return $results;
    ```
    
    ## Run the script against various `vimeo/psalm` versions
    
    ```sh
    docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace
    
    docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace
    
    docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace
    
    docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace
    
    ```
    
    ## Evaluate output
    
    ```
    ❯ docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace
    ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, mixed, mixed} (see https://psalm.dev/224)
    
    ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace
    ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, mixed} (see https://psalm.dev/224)
    
    ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace
    ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224)
    
    psalm on  feature/vimeo#8720-improve-types-and-purity-for-reflection-symbols [!?] via 🐘 v8.1.13 via ❄️  impure (nix-shell) took 4s
    ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace
    ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224)
    ```
    Ocramius committed Dec 6, 2022
    Copy the full SHA
    93c5df6 View commit details
    Browse the repository at this point in the history

Commits on Dec 7, 2022

  1. Mark Reflection(Method|Property)#setAccessible() as pure starting f…

    …rom PHP 8.1 onwards
    
    This will highlight unused code.
    
    Ref: php/php-src#5412
    Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
    Ref: php/php-src#5411
    
    Example https://3v4l.org/PNeeZ
    
    ```php
    <?php
    
    class Foo {
        private $bar = 'baz';
        private function taz() { return 'waz'; }
    }
    
    //var_dump((new ReflectionProperty(Foo::class, 'bar'))->getValue(new Foo));
    //var_dump((new ReflectionMethod(Foo::class, 'taz'))->invoke(new Foo));
    ```
    
    Produces (starting from PHP 8.1):
    
    ```
    string(3) "baz"
    string(3) "waz"
    ```
    Ocramius committed Dec 7, 2022
    Copy the full SHA
    ed2cde1 View commit details
    Browse the repository at this point in the history
  2. Copy the full SHA
    30a4963 View commit details
    Browse the repository at this point in the history
  3. Always load preloaded stub files when the analysis version is compatible

    Before this change, preloaded stubs would only be loaded when running on a PHP version lower than
    the one that is in the stubs.
    
    With this change, the analysis PHP version (defined via config, input parameter, or inferred from
    the runtime) becomes authoritative.
    
    Since the PHP-version-specific stubs are not just polyfills, but actually type refinements on top
    of the PHP core symbols at hand, this change always loads them, so that it is possible to get more
    precise type inference downstream.
    
    This will likely lead to downstream breakages, because the stubs do indeed provide better type
    resolution, but indeed formalizes the idea that these stubs will provide better value for finding
    problems in analyzed code.
    Ocramius committed Dec 7, 2022
    Copy the full SHA
    0423051 View commit details
    Browse the repository at this point in the history

Commits on Dec 8, 2022

  1. Load PHP-version-specific stubs based on analysis PHP version, and on…

    …ly when visiting stub files
    
    While `visitPreloadedStubFiles` seemed to work at first, it led to overriding symbols declared by
    PHP itself too eagerly.
    
    By only loading PHP-version-specific stubs in `visitStubFiles`, we ensure that the reflection-declared
    symbols take priority, and that our stubs overlay on top of that, without actually replacing the
    symbol entirely, but rather merging with its definition.
    
    This fixes current test failures too, and works with the code examples
    from vimeo#8722 (comment)
    Ocramius committed Dec 8, 2022
    Copy the full SHA
    68d88c5 View commit details
    Browse the repository at this point in the history