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

Improve intersecting constant array with general array #1429

Merged
merged 1 commit into from Jun 16, 2022

Conversation

rvanvelzen
Copy link
Contributor

No description provided.

@@ -683,6 +684,13 @@ public static function intersect(Type ...$types): Type
return 1;
}

if ($a instanceof ConstantArrayType && !$b instanceof ConstantArrayType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment above the sort callback might need adjustments

@ondrejmirtes
Copy link
Member

The first failure in question is about this PHPDoc:


     * @psalm-return array{
     *   __OFFSET: array<string, int>&array{__FILE: string},
     *   setting?: array<string, string>,
     *   extension_versions?: array<string, array{version: string, operator: string}>
     * }&array<
     *   string,
     *   string|array{version: string, operator: string}|array{constraint: string}|array<int|string, string>
     * >
     * 

PHPStan says about it:


  188    Method PHPUnit\Util\Annotation\DocBlock::requirements() return type    
         has no value type specified in iterable type array.                    

I don't see why - there's no array without a specified value there.

@ondrejmirtes
Copy link
Member

The second failure:

  265    Property PHPUnit\Util\Annotation\DocBlock::$parsedRequirements         
         (array<string, mixed>|null) does not accept array<int|string, mixed>.  

It's about this piece of code:


    /**
     * @var null|array<string, mixed>
     *
     * @psalm-var null|(array{
     *   __OFFSET: array<string, int>&array{__FILE: string},
     *   setting?: array<string, string>,
     *   extension_versions?: array<string, array{version: string, operator: string}>
     * }&array<
     *   string,
     *   string|array{version: string, operator: string}|array{constraint: string}|array<int|string, string>
     * >)
     */
    private $parsedRequirements;

And the value assigned into the property:


        return $this->parsedRequirements = array_merge(
            $requires,
            ['__OFFSET' => $recordedOffsets],
            array_filter([
                'setting'            => $recordedSettings,
                'extension_versions' => $extensionVersions,
            ])
        );

I'm not sure who's wrong here - whether the type inference near the array_merge call, or the interpretation of the property PHPDoc type.

Link to the file in question: https://github.com/sebastianbergmann/phpunit/blob/9.5.12/src/Util/Annotation/DocBlock.php

@rvanvelzen
Copy link
Contributor Author

I think that is mostly due to differences in how Psalm and PHPStan interpret these array shape intersections. Notably, array{key1: string}&array{key2: string} is *NEVER* in PHPStan, but array{key1: string, key2: string} in Psalm.

I believe in the context of how PHPStan handles it this PR is consistent, but it might make moving to (or at least borrowing ideas from) Psalm's model harder in the future.

@ondrejmirtes
Copy link
Member

The root issue about these types is that "array shape" from PHPDoc and a "literal array in code" should be two different Type implementations. There are certain differences about them and right now ConstantArrayType tries to be both of them and is bad at it.

Some examples:

  1. Array shape allows arrays with extra keys passed into it. This means that count isn't really exactly the number of keys, but that number or bigger. (Example: we have array{foo: int}, but the count should be int<1, max>.
  2. With literal array, we know exactly how the array looks like, we even know the order of keys which should be useful when comparing the array against another one with ===.
  3. With array shape, the order of keys isn't significant and some comparisons should behave more loosely.
  4. With literal array, an intersection of two different arrays is usually going to lead to NeverType because there's no way that ['foo'] === ['bar'] is ever going be true
  5. With array shape I'm open to discussing what should happen with an intersection in the PHPDoc. Also there's a feature request for "an array shape but with additional keys of a certain type" and I'm of the opinion there should be a new syntax for this instead of misusing intersection with a general array.

Of course introducing new type implementation isn't trivial because we have to think and implement all the interactions with all the other types...

@ondrejmirtes
Copy link
Member

Anyway, this PR is fine, thank you :)

@ondrejmirtes ondrejmirtes merged commit 2b0ba44 into phpstan:1.7.x Jun 16, 2022
@rvanvelzen rvanvelzen deleted the constant-array-intersect branch June 16, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants