Add @psalm-api annotation

[jack-worman]

• Added new @psalm-api
  • Added documentation
• Method param usages are checked regardless of if the class or method has references

[danog]

I think a better way to implement this is by adding a @psalm-public-api annotation that can be used on classes and methods to signal that they're part of a public API (possibly in isClassReferenced), and thus cannot be marked as unused even if no references can be found within the current codebase.

[jack-worman]

I think a better way to implement this is by adding a @psalm-public-api annotation

I don't think this PR and a @psalm-public-api annotation are mutually exclusive.
This PR still has value because Psalm is making an unsafe assumption that just because it can't find usages of a class it is not worthy of further analysis.

This also has the advantage of being more consistent. For examples, I have an UnusedClass but a bunch of UnusedMethod and UnusedProperty issues are being masked. For some reason, the class gets a reference (maybe a test was added to it). Those masked issues now start to trigger. This is annoying and unintuitive.

Wouldn't @psalm-public-api just be equal to @psalm-suppress UnusedClass?

[orklah]

A test would be welcome here, I was quite confused at first.

So if I get it, tomorrow, an unused class with 50 methods will emit 51 errors because the class and each methods are unused?

I get the point for properties (that can be used inside methods that are seen as unused) but for methods, I'm not sure, seems like unnecessary noise

[jack-worman]

but for methods, I'm not sure

In my code base, I found unused method arguments being masked because because the class was unused.

seems like unnecessary noise

So, you would be in favor of adding a new option/config? This will allow end users to opt-in. Perhaps, default it to on in Psalm 6.

[danog]

I don't think adding a new config is a good idea, because it's the wrong way of solving the problem.

The problem is that code that will get used by users of a public API is incorrectly marked as unused.
To solve this issue, a new attribute signaling to Psalm that a class or method is public API should be created.

Wouldn't @psalm-public-api just be equal to @psalm-suppress UnusedClass?

This is not the same thing, because if for example the isClassReferenced is made to always return true if a @psalm-public-api annotation is present, checkPropertyReferences and checkMethodReferences will be invoked; @psalm-suppress UnusedClass does not have the same effect.

[jack-worman]

I don't think adding a new config is a good idea, because it's the wrong way of solving the problem.

I was suggesting the config as a BC layer. I would eventually want (in Psalm 6) to remove the config option and have it on all the time.

But anyways, I'll experiment with trying to implement your @psalm-public-api annotation.

[danog]

Thanks!
Could you also add some docs? :)

[jack-worman]

Could you also add some docs? :)

Done.

But, this solution still doesn't solve my original concern. UnusedParam issues in classes marked with @psalm-public-api are still suppressed.

I still want a way to say, I don't care if the class is not used, still check my properties, methods, and method args for usages.

[weirdan]

Classes like Symfony controllers and event subscribers are not referenced directly in code and are still used

This sounds like the problem the corresponding plugin should be solving. It's not that hard either; it was one of the first things I did in the PHPUnit plugin.

@api tag would still be helpful for libraries, though.

UnusedParam issues in classes marked with @psalm-public-api are still suppressed.

Then it's something we need to improve.

[weirdan]

Prior art:

• https://docs.phpdoc.org/guide/references/phpdoc/tags/api.html
• https://github.com/TomasVotruba/unused-public#skip-false-positives

So we may want to use @api and @psalm-api instead.

[danog]

++, and now that I think of it, it would be cool to change the default of findUnusedCode to true, now that we have this @api annotation that can be used to cleanup false positives (possibly mentioning the @api annotation in the docs for the unusedmethod/unusedclass issues).

[danog]

LGTM!

[orklah]

I never used this config, does it makes sense to keep it?

[jack-worman]

This config is used by default. It should be used if no other config file is given.

[orklah]

Thanks!