Skip to content

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Mar 18, 2025

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This is very very valuable, but it needs two things:

  1. The new errors can be reported only with bleeding edge.
  2. The new code has to employ RuleLevelHelper. The idea is that "always wrong" like int is always reported, "sometimes wrong" like int|string is reported on level 7+, nullable like string|null is reported on level 8+ and mixeds are reported on level 9+. Just use RuleLevelHelper like it's used in all the rules and it will be taken care of automatically.

@zonuexe zonuexe marked this pull request as draft March 18, 2025 12:37
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Also please do this in two separate PRs:

  1. Let the rules handle Expr as $node->name to continue the error-reporting. So that:
$a = 'Foo';
echo Foo::{$a}

Is checked against a non-existent constant. This doesn't need bleeding edge.

  1. Introduce new reported errors that check $a in Foo::{$a} to be a string. This needs bleeding edge and RuleLevelHelper.

Thank you.

@ondrejmirtes
Copy link
Member

Also within those PRs add relevant regression tests for these issues: https://github.com/phpstan/phpstan-src/actions/runs/13923245791 (some of them might disappear once we do it for bleeding edge so it's nice to have this list now)

@zonuexe
Copy link
Contributor Author

zonuexe commented Mar 18, 2025

Ok, I'll freeze this branch and submit some PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Double $this typo not recognized

2 participants