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

Update PHPStan #251

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Update PHPStan #251

merged 1 commit into from
Sep 18, 2023

Conversation

ondrejmirtes
Copy link
Contributor

PHPStan 1.10.31 introduces NonAcceptingNeverType (phpstan/phpstan#6485, phpstan/phpstan#9133) that ensures that a literal never type in the code does not accept any other type except never.

As a side-effect of this change a type description or an error message about this changed from *NEVER* to never. I'm sending an update to your test suite because of this :)

@clue
Copy link
Member

clue commented Aug 30, 2023

@ondrejmirtes Thank you for looking into this and suggesting improvements for the work done in #247. Briefly eyeballed your changes and noticed there are still a couple of *NEVER* types in tests/types/resolve.php, can you help me understand the reason for what currently looks like an inconsistency?

@ondrejmirtes
Copy link
Contributor Author

The code in question:

assertType('React\Promise\PromiseInterface<*NEVER*>', resolve(true)->then(function (bool $value): never {
    throw new \RuntimeException();
}));

There are two "never" types in PHPStan:

  • NeverType (*NEVER*) - used basically everywhere - in cases like empty arrays
  • NonAcceptingNeverType (never) - used when the user types never literally in PHPDoc or native return type

For anonymous functions, PHPStan can infer always-throwing function even without a native never return type. In that case, *NEVER* is used. But there's probably isn't a good reason for that, so I changed it: phpstan/phpstan-src@3e03e9d

It will be released at the earliest convenience.

@clue
Copy link
Member

clue commented Sep 1, 2023

@ondrejmirtes Thanks for the insights and the update in PHPStan! 👍

Your explanation sounds reasonable. It's my understanding this is more of a maintenance topic that doesn't currently have high priority, so we probably want to keep this open until the next PHPStan is released and we can update this consistently in a single shot. Thank you, will keep an eye on this!

@WyriHaximus WyriHaximus added this to the v3.1.0 milestone Sep 10, 2023
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

Thanks @ondrejmirtes for filing this, had a quick row with this while working on the initial templates implementation. Liking these changes 👍 .

@ondrejmirtes
Copy link
Contributor Author

Just update PHPStan version and all *NEVER* to never. This should be good to go :)

Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@ondrejmirtes Thanks for the update, changes LGTM! :shipit:

@clue clue merged commit 7053e3c into reactphp:3.x Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants