vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

Add target for @psalm-suppress. #8016

Open rzvc opened 2 years ago

rzvc commented 2 years ago

I'm not sure if this has been brought up before (I searched, but nothing came up), so sorry if it's a duplicate.

I often find myself wanting to specify which variable/parameter/whatever I'm applying the suppression for. Preferably I would like to be able to do that all the time.

Consider a constructor that takes several parameters and one is unused. If @psalm-supress is used, it will suppress the issue for all other parameters too, even tho the other ones might not remain unused on purpose.

AndrolGenhald commented 2 years ago

I'm not sure, I wonder if it'd be better to allow the suppression to be applied more specifically instead like this. Are there cases where that wouldn't work?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/821d4d3716 ```php
rzvc commented 2 years ago

The only issue I have with that way of applying suppressions is that it makes the code more difficult to read, but otherwise it seems even more specific than the $target approach.

Maybe we can have both?

AndrolGenhald commented 2 years ago

The difficulty I see with having some sort of target is:

If you or someone else can find a way to do this while handling these issues well enough (perfection isn't possible here since the description is arbitrary, but as long as the vast majority of cases won't cause problems it should be ok) then it sounds cool, but I'm having trouble seeing how exactly it would work.

weirdan commented 2 years ago

Specifically for unused params / vars Psalm has an alternative shortcut: prefixing the variable name with underscore (https://psalm.dev/r/b3ec66ab6f)

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/b3ec66ab6f ```php
rzvc commented 2 years ago
  • How does it work when suppressing multiple issues (fyi, @psalm-suppress Issue1, Issue2, Issue3 is valid!), does it not work at all, apply to all issues, something else?

Maybe something like this: @psalm-suppress Issue1:$target, Iussue2:function()?, Issue3 Some random comment.

This syntax would solve the function targeting too.

Edit: Also, if there are multiple targets for the same issue, I see no problem with parenthesis.

AndrolGenhald commented 2 years ago

Edit: Also, if there are multiple targets for the same issue, I see no problem with parenthesis.

For some reason I was caught up in having a space after the issue name, but for multiple instances of an issue I wonder if @psalm-suppress Issue1:$foo, Issue1:$bar would be better.

One issue that still exists is if the targeted code includes a comma, since that would interfere with the issue list parsing. We could just require that the target code is in parentheses (either in that case or for all cases), but then the parser is much more complicated since it can't just be an explode on ",".

rzvc commented 2 years ago

Unless you need something more sophisticated, this regex will parse all that:

/^[\t ]*@psalm-suppress
  ([\t ]+([a-z]\w*)
    (?:
     :(
       \$[a-z]\w*
       |
       [a-z]\w+(?4)
     )
     |
     (\((?:[^\(]|(?4))*?\))
    )?
    (?:,\s*(?1))?
  )?/gmix

https://regex101.com/r/GZ4QGP/2