Make alert messages consistent across languages #9816
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from ControlStructure s, BlockStmt eb | ||
| where | ||
| emptyBlock(s, eb) and | ||
| not emptyBlockContainsNonchild(eb) and | ||
| not lineComment(eb) | ||
| select eb, "Empty block without comment" | ||
| select eb, "Empty block without comment." |
Check warning
Code scanning / CodeQL
Consistent alert message
| from BlockStmt s | ||
| where | ||
| (loopStmtWithEmptyBlock(s) or conditionalWithEmptyBlock(s)) and | ||
| not exists(CommentBlock c | c.getParent() = s) and | ||
| not exists(ForStmt fs | fs.getBody() = s and exists(fs.getAnUpdate())) | ||
| select s, "Empty block." | ||
| select s, "Empty block without comment." |
Check warning
Code scanning / CodeQL
Consistent alert message
erik-krogh
force-pushed
the
msgConsis
branch
2 times, most recently
from
e7e5f2d to
d55db77
Compare
erik-krogh
force-pushed
the
msgConsis
branch
5 times, most recently
from
8efba63 to
9eda368
Compare
| select v, | ||
| "Unused local variable " + v.getName() + | ||
| ". The variable is never read or written to and should be removed." | ||
| select v, "The value assigned to local variable '" + v.getName() + "' is never used." |
There was a problem hiding this comment.
The new message says that the variable is being assigned a value, but the old message specifically says that it wan't written to. Which is it?
There was a problem hiding this comment.
It seems to be "wasn't written to". The query behaves slightly different compared to the other languages.
I'll revert this change.
| @@ -58,4 +58,4 @@ where | |||
| not exists(AsmStmt s | f = s.getEnclosingFunction()) and | |||
| not v.getAnAttribute().getName() = "unused" and | |||
| not any(ErrorExpr e).getEnclosingFunction() = f // unextracted expr may use `v` | |||
| select v, "Variable " + v.getName() + " is not used" | |||
| select v, "The value assigned to local variable '" + v.getName() + "' is never used." | |||
There was a problem hiding this comment.
This seems wrong. The value might actually be used (as it might just have been another variable). It's just the variable that's not being used.
There was a problem hiding this comment.
I used the alert message from Python to create this alert message for CPP.
I'll do it the other way around instead 👍
| @@ -26,4 +25,4 @@ where | |||
| forall(int op | op = lhs.(BitwiseAndExpr).getAnOperand().getValue().toInt() | op < 0) and | |||
| // exception for cases involving macros | |||
| not e.isAffectedByMacro() | |||
| select e, "Potential unsafe sign check of a bitwise operation." | |||
| select e, "Sign check of a bitwise operation." | |||
There was a problem hiding this comment.
This seems less detailed, and make it less likely the user will inspect the alert?
There was a problem hiding this comment.
Python 👍
Overall question: Will changing these alert messages cause all current Code Scanning alerts to be closed, and new ones to pop up instead?
Grammar nitpick
I think the new alert messages that read User-provided value flows to here and is used in a command. sounds odd grammatically. Shouldn't they be quantified to read A user-provided value flows to here and is used in a command.
Reading that sentence over a few times, I'm also starting to wonder about flows to here, which sounds odd, I would always say flows here.
(I not a gramtic genius, these just didn't match how I would talk with a person)
Personally, I would suggest A user-provided value flows here and is used in a command.
Doesn't look like it. I tried to change an alert message here, and code-scanning reported no change in alerts.
There is a lot of those: https://cs.github.com/github/codeql?q=%22%5C%22User-provided+value%22 |
| @@ -16,4 +16,4 @@ where | |||
| e.isStrict() and | |||
| e.getGreaterOperand() instanceof BitwiseExpr and | |||
| e.getLesserOperand().(IntegerLiteral).getIntValue() = 0 | |||
| select e, "Sign check of a bitwise operation." | |||
| select e, "Potential unsafe sign check of a bitwise operation." | |||
There was a problem hiding this comment.
Shouldn't that be
| select e, "Potential unsafe sign check of a bitwise operation." | |
| select e, "Potentially unsafe sign check of a bitwise operation." |
There was a problem hiding this comment.
Either works gramatically, with different bindings:
Potential (unsafe check)
(Potentially unsafe) check
There was a problem hiding this comment.
Yeah, I just felt that the latter was closer to the intended message. But I guess both work semantically.
| from Name unused, LocalVariable v | ||
| where | ||
| unused_local(unused, v) and | ||
| // If unused is part of a tuple, count it as unused if all elements of that tuple are unused. | ||
| forall(Name el | el = unused.getParentNode().(Tuple).getAnElt() | unused_local(el, _)) | ||
| select unused, "The value assigned to local variable '" + v.getId() + "' is never used." | ||
| select unused, "Variable " + v.getId() + " is not used" |
Check warning
Code scanning / CodeQL
Consistent alert message
| @@ -35,4 +35,4 @@ where | |||
| access = cast.getAChild() and | |||
| access.getTarget().getDeclaringElement() = access.getEnclosingCallable() and | |||
| nodeBeforeParameterAccess(access.getAControlFlowNode()) | |||
| select cast, "Missing type-check before casting parameter to 'Equals'." | |||
| select cast, "Equals() method does not seem to check argument type." | |||
There was a problem hiding this comment.
Shouldn't @precision high mean high confidence in the result? If so, then we could change does not seem to check to does not check.
…t-in-equals` queries
The tests fail and I know it. The internal PR fixes the tests (see backref towards the bottom).
I recently added a QL-for-QL query for detecting inconsistent alert messages across languages.
This PR fixes most of the alerts found by that query such that the queries now use mostly the same message across languages.
I fixed the alerts by looking at the alert-message across all languages, and then I picked the message I liked the most.
That choice is sometimes somewhat arbitrary, so feedback is very welcome.
There are still some queries that use different messages, e.g. because they report a different level of detail, or because different alert messages better suit each language.
(Example: cpp really likes to use a callchain, and the alert message is therefore different from the other languages).
I fixed the alerts while developing the QL-for-QL query, and a lot of the fixed alert messages are not reported by the current query.
I tried to get the QL-for-QL query to less noisy, but I could only manage that if I also removed some TPs, so I might make the QL-for-QL query more noisy in the future.