search

github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.52k stars 1.5k forks source link

LGTM.com should respect Java's @SuppressWarnings annotation #2076

Closed shemnon closed 4 years ago

shemnon commented 4 years ago

Java has a standard mechanism (the @SuppressWarnings annotation) to suppress linting warnings that is used by Javac, FindBugs, Errorprone, and may other linters. LGTM should support it.

Specifically, this warning is already supporessed - https://lgtm.com/projects/g/hyperledger/besu/snapshot/02f6b9656d63edcaa1edff38c513a797d3a644ae/files/crypto/src/main/java/org/hyperledger/besu/crypto/PRNGSecureRandom.java?sort=name&dir=ASC&mode=heatmap#x9c62677733e6345:1

aschackmull commented 4 years ago

We have discussed this a bit internally and opted to improve the query to only report immediate overrides, which should remove this FP. The problem with @SuppressWarnings annotations is that although the annotation itself is a standard mechanism, the suppression strings are far from standard, and each tool tends to have its own set of recognized strings. Probably the closest thing we have to a standard list of accepted suppression strings is the short list that's supported by javac (as shown by javac -X).

shemnon commented 4 years ago

I am confused as to why a lack of standard suppression strings results in a close-won't-fix. You can simply use the same query-id that are going into the suppression comments. That's how @SuppressWarnings was designed to be used and is used in practice. Google's error-prone does this. Putting in tool-sepecific strings in @SuppressWarnings is common Java practice (it takes an array of strings as well).

aschackmull commented 4 years ago

If we were to add support for this it would probably look something like @SuppressWarnings("lgtm[java/non-sync-override]"), since it would be natural to use the query id "java/non-sync-override" in the suppression string, so this wouldn't match the suppression that's already in place. Is that what you had in mind?

shemnon commented 4 years ago

That or @SuppressWarnings("java/non-sync-override") or @SuppressWarnings("non-sync-override"). Since it's the java handler the lgtm[java/...] could be implied and any lookups that failed would be ignored.

For a point of reference comment based inspection suppression is banned by our coding style, because @SuppressWarnings exists and the tools we use all support it.

aschackmull commented 4 years ago

We are looking into this, see https://github.com/Semmle/ql/pull/2152