Contributors update #1 2023 - Error identifiers aboard! #8981
Replies: 12 comments 25 replies
-
Would it make sense to support a |
Beta Was this translation helpful? Give feedback.
-
I suggest support a comma-separated list of identifiers instead of a single identifier to support ignoring multiple errors. My suggestion of using a comma as separator is based on the syntax used by Eslint (using a similar syntax helps for users already familiar with the Eslint syntax due to working with both PHP and JS) |
Beta Was this translation helpful? Give feedback.
-
Why not both? Currently baseline entry looks like: $ignoreErrors[] = [
'message' => '#^Method ... has no return type specified\\.$#',
'count' => 1,
'path' => __DIR__ . '/some/file.php',
]; but it could be: $ignoreErrors[] = [
'rule' => 'some.unique.identifier',
'message' => '#^Method ... has no return type specified\\.$#',
'count' => 1,
'path' => __DIR__ . '/some/file.php',
]; From the perspective of developer willing to fix errors from baseline, reading entries with identifier only wouldn't make much sense IMHO. Message (regex) is human-friendly, it shows what the problem exactly is and can help with actual fix, while identifier could help with getting rid of such entry by using identifier in the main config or inline annotation. Decision would be up to developer 🙂 |
Beta Was this translation helpful? Give feedback.
-
I think it would be nice to have dedicated pages for (certain) errors, just like shellcheck or psalm are doing it. |
Beta Was this translation helpful? Give feedback.
-
I am a little worried whether the identifiers are perhaps a little too vague, especially as can be seen in the "grouped by identifiers" file. In some cases the same identifier is used for different elements of the code. I do not think that this is a problem with the inline ignoring (which was probably more of a focus in the proposal), but rather in the case where you want to ignore whole class of reports in the config file upfront. This is concern mainly for more strict/opinionated rules, of course this does not really make sense for "hard" errors. But if we are speaking about ignoring in the config file, you would probably need to differentiate which of the many "sub"-cases this is really originating from, because perhaps most of them you want to check, but then there is the one, you don't. One option is to again use message+regexp, but that is not much better than what we have today. Another option is to have the ability to specify the identifier with different degrees of freedom.
And what you can do (I know you do know @ondrejmirtes ;) ) is to exclude the whole sniff, or just some of the granular codes. I am not proposing to match this exactly, because in Instead of just
etc. Again, even the last part should not have to be 1:1 to the rule. Then it could be up to you where you include/exclude |
Beta Was this translation helpful? Give feedback.
-
It would be great to support multi line "inline" ignoring annotations, for similar usecase see again eslint: This is a code style issue, but I can imagine similar problem for example with calling a 3rd party lib which has only a magic method and I need to pass parameters on separate lines - I don't want to write the same ignore on every line. The naming is of course up for debase, I think something like |
Beta Was this translation helpful? Give feedback.
-
how would this impact custom rules? will an error identifier be required or is it going to be optional? |
Beta Was this translation helpful? Give feedback.
-
I've made huge progress on error identifiers recently! All rules in PHPStan core and in all 1st party extensions are now labeled with error identifiers. Here's the complete list (automatically extracted & always up-to-date): https://phpstan.org/error-identifiers |
Beta Was this translation helpful? Give feedback.
-
Using RuleErrorBuilder to enrich reported errors in custom PHPStan rules: https://phpstan.org/blog/using-rule-error-builder The article also has important notes about the future requirements for custom rules - plain strings are being deprecated, and identifiers will have to be provided. |
Beta Was this translation helpful? Give feedback.
-
I continue working on this. Here's how the ignore comment can currently look like (optional reason goes into parentheses foo(); // @phpstan-ignore function.notFound (Tell me why) Some invalid examples:Unknown identifiers foo(); // @phpstan-ignore function.notFound Tell me why Parse error - non-identifier outside of parentheses (it's non-ASCII): foo(); // @phpstan-ignore function.notFound čičí Unclosed parenthesis: foo(); // @phpstan-ignore function.notFound (Tell me why There can be a comma Here's the implementation: phpstan/phpstan-src@2206c75 (I might have overengineered it a bit with a lexer inspired by phpdoc-parser 😅 ) |
Beta Was this translation helpful? Give feedback.
-
sometimes we want to fix a single problem across the whole codebase. maybe it would be helpful to allow filtering the PHPStan results (or baseline?) by a cli argument via a error identifier? |
Beta Was this translation helpful? Give feedback.
-
PHPStan 1.11 with error identifiers, PHPStan Pro reboot, checking truthiness of @phpstan-pure, and new callable-related PHPDoc tags, is almost finished & is going to be released one week from now! |
Beta Was this translation helpful? Give feedback.
-
Hello everyone!
This one is going to be about error identifiers, one of the most requested features. They're not useful by themselves, it depends on how we apply them to various PHPStan features.
First, they're going to be applied for local ignoring with
@phpstan-ignore-line
and@phpstan-ignore-next-line
. Right now these comments ignore everything on the marked line. With error identifiers, you'll be able to write it like this and ignore only errors of a specific type:How to approach the names of the identifiers? Some tools and languages take the easy way out and mark each error type with a number, which isn't really great to see what's going on and what exactly are we ignoring:
Another easy way out for PHPStan would be to just pair errors with the rule classes that report them. But I didn't want that either - I consider rule class names to be implementation details. They don't map 1:1 with errors understood by users. To name an example: there are three separate rule classes about array offset assignment. On the other hand, there are rules that report many different error types in a single class.
So we need to add a new thing, a new dimension, for each reported error. Which is what I spent the last three days doing. I took the opportunity to count how many of rules there are in PHPStan: 226 rule classes! And based on the identifiers I chose, there are 426 different error types reported.
The current draft is here:
If you want to comment on anything in this draft, please comment on this commit: phpstan/phpstan-src@c0bf80e
I encourage everyone interested in going through the list and spot any inconsistencies, and suggest any improvements. I had to make quite a few decisions so I hope you like them.
The Plan
There's a lot of things we can do with these identifiers once added to the codebase. Here are some of my ideas. That's what they are - no promises on implementing all of them, but I encourage you to let me know what you think!
->build()
return.->identifier()
into the RuleErrorBuilder calls and always return IdentifierRuleError.phpstan-ignore-line
andphpstan-ignore-next-line
will support identifiers (Use phpstan-ignore to ignore a specific error #3296). With bleeding edge the only allowed thing after the PHPDoc tag is going to be an identifier. I understand that currently some users might have written something else there, but only identifiers will be allowed in the future.-v
.-v
. So that users can easily see what identifier they should use.@phpstan-ignore-line
that ignore all errors into specific ignores for that one kind of error identifier that's on that line.There's even more stuff we could do for identifiers, but I'm gonna save that for later, if the need arises:
I'm gonna let the identifiers draft sit over the weekend, and starting next week, I'm gonna add those identifiers into the codebase, and implement the planned ideas in 1.11.x-dev.
I'm looking forward to your feedback! Thank you.
Beta Was this translation helpful? Give feedback.
All reactions