Contributors update #1 2023 - Error identifiers aboard!

[ondrejmirtes]

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:

echo []; // @phpstan-ignore-line echo.nonString

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:

// @ts-ignore TS2339
window.navigator.share({url: window.location.href});

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:

• Grouped by rules: https://github.com/phpstan/phpstan-src/blob/error-identifiers/rules.txt
• Grouped by identifiers: https://github.com/phpstan/phpstan-src/blob/error-identifiers/rulesByIdentifier.txt

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!

• RuleErrorBuilder will gain generics and will understand what RuleError interfaces will ->build() return.
• Bleeding edge will change Rule stub and require both PHPStan and custom rules to add ->identifier() into the RuleErrorBuilder calls and always return IdentifierRuleError.
• phpstan-ignore-line and phpstan-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.
• TableErrorFormatter (the default one) will output identifiers when PHPStan runs with -v.
• If there's an unmatched ignored error because of a wrong identifier, TableErrorFormatter will output identifiers in that file even without -v. So that users can easily see what identifier they should use.
• Checkstyle error formatter will add identifier in the source attribute (missing source attribute in checkstyle errorFormat #1205)
• A nice webpage will be generated from all the identifiers with click-throughs to the source code, serving as a documentation of all PHPStan capabilities.
• PHPStan Pro will offer a migration "wizard", letting you automatically convert all @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:

• Instead of registering and un-registering rule classes that run and report errors, we could let all of them be registered and run, but filter errors later based on identifiers.
• Let baseline use error identifiers instead of message regexes. But I fear that it would be less granular and more error-prone than the current situation.

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.

[stof]

• 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.

Would it make sense to support a -- delimiter allowing to write free-form text after it ? It might make sense to put an explanation about why the error is marked as ignored. This would match what eslint does: https://eslint.org/docs/latest/use/configure/rules#comment-descriptions

[VasekPurchart]

I would like this as well and as a cherry on top would propose to be able to require this comment.
Probably not by default, but as an opt-in setting?

[VincentLanglet]

If like proposed in #8981 (comment), identifiers are comma-separated, the -- separator shouldn't be even needed

@phpstan-ignore-next-line Identifier1,Identifier2 reason

Identifier would be separated by , and a space act as a separator for the comment.

[stof]

Eslint allows spaces around the comma-delimited idenfiers to increase readability, which is why they need the -- separator

[stof]

• With bleeding edge the only allowed thing after the PHPDoc tag is going to be an identifier.

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)

[Wirone]

Let baseline use error identifiers instead of message regexes. But I fear that it would be less granular and more error-prone than the current situation.

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 🙂

[ondrejmirtes]

I'd rather add the identifier just as a comment instead of another key that the whole "ignoring errors system" needs to work with.

[Wirone]

This is technical detail - the point is to provide both 🙂

[VasekPurchart]

I would also support the notion, that these options should be separate. Mainly because ignoreErrors can have different uses based on context we have generally 3 categories:

• baseline is generated automatically, so it should be as specific as possible, so including message (I would argue, that some messages are even too vague and hence cover too much).
• specific cases - "I have this one place, where I need to do this nasty thing, but not anywhere else" - again, be as specific as possible and hopefully these should probably be moved entirely to inline annotations
• generic error ignores - "we do not want to check this at all" (for various reasons) - ideal to be based primarily on identifiers, but could still use regexp as a way to limit the scope (with caveat, which must be understood, that it is more prone to breaking)

[Wirone]

@VasekPurchart my suggestion was strictly related to baseline files, that cover only 1st scenario you mentioned. How messages/identifiers are used in your manually crafted configs is a whole different story and I believe both will be supported 🙂

[pscheit]

that would help much when transferring errors from the baseline to local ignores

[alfredbez]

I think it would be nice to have dedicated pages for (certain) errors, just like shellcheck or psalm are doing it.

[ondrejmirtes]

That's already hapenning:

• https://phpstan.org/blog/whats-up-with-template-covariant
• https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static

Additionally, some errors are explained directly in the output: https://phpstan.org/blog/phpstan-1-10-comes-with-lie-detector#why-my-type-isn%E2%80%99t-accepted-here%3F

[alfredbez]

I'm aware of that and I like that you put so much love into describing these things ❤️

What I like about the other tools I mentioned: They link to these webpages in the output per default and I think this is a big improvement in terms of DX.

This is what I see when I run shellcheck on a file with an error:

I think shellcheck does this for every error it reports. PHPStan already does this in some cases, but I think it can even benefit from some community support to describe all possible errors and how to fix them on a dedicated page.

---

I (tried to) introduce PHPStan to some teams already and the initial learning curve to understand what it's telling me is sometimes pretty hard. It's technically correct, it makes all sense, someone doing that for years has no problem understanding this, but it would be really nice to get people new to the topic onboard faster.

[ondrejmirtes]

If there are some errors that aren't obvious, please open a feature request for explaining them. I don't think that we need to explain "int does not accept string".

[VasekPurchart]

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.

phpcs is using something like this - there is the sniff name + code, for example DisallowIncrementAndDecrementOperators is the sniff name and then there are the codes:

• DisallowedPreIncrementOperator
• DisallowedPostIncrementOperator
• DisallowedPreDecrementOperator
• DisallowedPostDecrementOperator

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 phpcs the sniff names are mapped 1:1 to classes and hence the codes somewhat too, which I agree is not good and the proposed system here independent of classes is better, but I would propose to have them combined.

Instead of just class.nameCase, there could be for example:

• class.nameCase.classConstant
• class.nameCase.staticMethodCall

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 class.nameCase or just class.nameCase.staticMethodCall and you could avoid trying to include everything and then using a regexp.

[Wirone]

These are exactly my thoughts, because some time ago I said I am open to work on it and thought about it several times since (unfortunately not enough to propose something meaningful). I would also like some kind of hybrid model, not mapped to classes, but allowing user to easily ignore some group of rules or only some specific kinds of rules from a group. For example report function's unused arguments, but don't report if optional arguments are before required ones (deprecated, but works fine). This could allow people focus better and prioritise issues in their code, without need to dump large baselines.

[ondrejmirtes]

I do not think that this is a problem with the inline ignoring

This is what I want to focus now and make the feature nice and usable. The problem is that I don't want the user to type out class.nameCase.classConstant when the error is obvious from the surrounding code context, and class.nameCase only.

Also, more dots IMHO comes with the need to interpret partial identifiers like class.nameCase. Should that be compared with === against the reported error, or as class.nameCase.*? I want to avoid this question for now.

But I have a possible future expansion of this system already in mind. We can keep simple identifiers like class.nameCase but provide additional information through metadata. That would come in handy for a feature I have in mind: some kind of errors dashboard with statistics like "50 % of errors in your project is about this one class, you can get rid of them very easily". So class.nameCase might be paired with a name (Foo), but also a place (classConstant, staticMethodCall).

These metadata could then have other uses, including a whole new ignoreErrors format (error identifier + metadata instead of message regexp).

[Wirone]

@ondrejmirtes that sounds reasonable and I believe it would allow expand the ignore syntax in the future, if not by providing deeper level (class.nameCase.classConstant) then maybe by allowing some shorthands like class.nameCase#classConstant,staticMethodCall or class.nameCase[classConstant,staticMethodCall]. Anyway, it's good idea to start with basic feature, get some feedback and then expand if needed 🙂

[VasekPurchart]

@ondrejmirtes I like this metadata idea, but to slightly counter my original point, that it is not really about inline ignoring I would (sometimes) want to use this more precise ignore even with the inline ignores.

Especially when you imagine something like the other suggestion I mentioned here. This is exactly the code sample where I would like to apply the camelCase ignore only for the object properties, but if there was for example an arrow function, doing a transformation, with some params of course I would still like them to be checked to be camelCase (again, not really a theoretical situation, since these analytics code snippets are copied a lot of time from god knows where and using what standards...).

[VasekPurchart]

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 phpstan-ignore-start(+ end) would better fit with the other ones.

[Wirone]

Even though I 100% agree with this proposal, I think it's out of the scope of this discussion. It looks like separate feature request: ability to ignore chunk of code (support for identifiers could be added later).

[mad-briller]

how would this impact custom rules? will an error identifier be required or is it going to be optional?

[ondrejmirtes]

It’s mentioned in the text: with bleedingEdge and in PHPStan 2.0 rules will be required to provide an identifier. You can easily silence that analysis error but obviously it’s encouraged to comply with that.

The reason behind that is to provide nice experience to PHPStan users, I assume everyone will get used to errors having identifiers and being able to ignore them like that.

[mad-briller]

what i get for ctrl-f and not reading, thanks for pointing it out 😅

[ondrejmirtes]

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

[canvural]

This is so cool! Thank you!

Should 3rd party extension authors start adding the identifiers too? 👀

[ondrejmirtes]

I'll have an article about this ready next 😊

Spoiler alert: yes, the Rule stub in 1.11.x-dev bleedingEdge requires it 😊

[mad-briller]

That page is a little bit wonky on mobile unfortunately

[ondrejmirtes]

Should be better now. Thanks!

[mad-briller]

miles better!

[ondrejmirtes]

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.

[ondrejmirtes]

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 Tell, me, and why:

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 , and any whitespace between each identifier.

Here's the implementation: phpstan/phpstan-src@2206c75 (I might have overengineered it a bit with a lexer inspired by phpdoc-parser 😅 )

[staabm]

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?

[Wirone]

That would be interesting. When I introduce PHPStan to a project or I want to bump analysis level I often look out for similar errors to fix and commit at once. Filtering the output would be better that scanning through it 🙂👍.

[ondrejmirtes]

I'm afraid such option would be misunderstood and misused.

But you can write your own custom error formatter, or you can use the json formatter and postprocess it however you want.

[ondrejmirtes]

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!