C++: Fix AV Rule 85 #371
C++: Fix AV Rule 85 #371
Conversation
We have to be careful to avoid giving alerts to functions that might be correctly defined, but we can't see the definition as it wasn't instantiated.
There was a problem hiding this comment.
The original query gave alerts for both templates and instantiations, and the alerts for instantiations are now causing issues if I understand you correctly. Why not fix that by removing the alerts for instantiations and only keeping them for uninstantiated templates and non-templates?
| | AV Rule 85.cpp:79:7:79:14 | MyClass8<T> | When two operators are opposites, both should be defined and one should be defined in terms of the other. Operator operator<= is declared on line 90, but it is not defined in terms of its opposite operator operator>. | | ||
| | AV Rule 85.cpp:79:7:79:14 | MyClass8<T> | When two operators are opposites, both should be defined and one should be defined in terms of the other. Operator operator> is declared on line 88, but it is not defined in terms of its opposite operator operator<=. | | ||
| | AV Rule 85.cpp:79:7:79:14 | MyClass8<T> | When two operators are opposites, both should be defined and one should be defined in terms of the other. Operator operator>= is declared on line 85, but it is not defined in terms of its opposite operator operator<. | | ||
| | AV Rule 85.cpp:79:7:79:14 | MyClass8<int> | When two operators are opposites, both should be defined and one should be defined in terms of the other. Operator operator< is declared on line 83, but it is not defined in terms of its opposite operator operator>=. | |
There was a problem hiding this comment.
This doesn't look right. Why do the alerts for MyClass{5,6,7} go away completely? The comments in the test source say they should still be there. And why is there no alert added for the new MyClass10?
There was a problem hiding this comment.
For 5, 6, 7 no function instantiations are extracted. For 10, no <= function instantiated is extracted.
I left the comments as they are still bad, even if we don't currently detect them.
There was a problem hiding this comment.
Perhaps we should modify the test to instantiate these functions, and add just one test case where the function is never instantiated to document that behaviour. This way the tests will be providing coverage for whatever issues they were originally intended to.
There was a problem hiding this comment.
These cases really are just testing the various cases where some things are and aren't used. Although the current query definition means that several of them get no alerts, I think they're still useful test cases for potential future changes to the query.
|
We don't get bodies for the templates, so we can't give alerts based on them. |
|
I'm still confused by this PR and by this query. The test seems to have produced alerts for code marked as Is it correct that if we get the body for an instantiation then we also get the body of its template? If that's the case, we should be able to ignore instantiations in this query. That seems a lot easier than trying to match instantiated operators with their templates by matching on the name. Matching by name is fragile in any case as operators can be overloaded. I'd much appreciate some QLDoc about which declarations and bodies are guaranteed to get extracted, especially now that it's changed after Ignoring instantiations doesn't by itself fix the problems with this query because we also need to suppress alerts from operators whose body did not get extracted. This might mean we'll end up suppressing all the same true positives that you're suppressing in this PR, but it should still make the query less complicated. |
| exists(Function f | f = c.getAMember() and | ||
| f.hasName(op) and | ||
| f.isDefined()))) | ||
| else any() |
There was a problem hiding this comment.
Would this be more readable without the if, as exists(Class temp1 | c.isConstructedFrom(temp1) and ...) or not exists(Class temp1 | c.isConstructedFrom(temp1))?
There was a problem hiding this comment.
I changed it to be an implies
|
174f1fe was PR 23883, which has description "It looks like the query is doing the wrong thing for templates, but this just adds a test of the status quo for now.". I've changed it to look at the template bodies instead of the instantiation bodies. Note that this means allowing a |
Data flow: Fix bug for sugared call arguments
Fix `ExprStmt` and `StmtExpr` in Boolean context
We have to be careful to avoid giving alerts to functions that might be correctly defined, but we can't see the definition as they weren't instantiated.