C++: More builtins and information regarding this param refs #21164
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for new compiler builtin operations and enhances tracking of implicit object parameter accesses in C++.
Changes:
- Added support for three new C++ builtin type trait operations:
__is_bitwise_cloneable,__is_invocable, and__is_nothrow_invocable - Added database schema tracking for
param_ref_to_thisto identify when parameter references are to implicit object parameters - Updated compiler version requirements in test files to support the new builtins
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/exprs/BuiltInOperations.qll | Added QL classes for the three new builtin operations |
| cpp/ql/lib/semmle/code/cpp/exprs/Access.qll | Added isThisAccess() predicate to ParamAccessForType |
| cpp/ql/lib/semmlecode.cpp.dbscheme | Added new expression kinds (394-396) and param_ref_to_this table |
| cpp/ql/test/library-tests/builtins/type_traits/*.cpp | Updated compiler versions and added test cases for new builtins |
| cpp/ql/test/library-tests/builtins/type_traits/expr.expected | Updated expected test results |
| cpp/ql/lib/upgrades/*/upgrade.properties | Database upgrade metadata |
| cpp/downgrades/*/exprs.ql | Downgrade logic to handle new expression kinds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b0bb99f to
131ff95
Compare
131ff95 to
1dacd83
Compare
geoffw0
left a comment
There was a problem hiding this comment.
LGTM. Couple of questions about the downgrade script. Feel free to merge if you're confident of your answers to those questions. I'm assuming you've tested the upgrade / downgrade process as usual too.
| @@ -0,0 +1,4 @@ | |||
| description: Add new builtin operations and this parameter access table | |||
There was a problem hiding this comment.
Shouldn't the downgrade description be about removing these things ... or is it supposed to just match the upgrade description?
There was a problem hiding this comment.
Good question. There are two lines of thought here. Either do what you suggest, or keep them the same, as it describes the overall change and not what happens in the script. We have never really standardized on this. I just prefer the latter.
| @@ -0,0 +1,4 @@ | |||
| description: Add new builtin operations and this parameter access table | |||
| compatibility: partial | |||
There was a problem hiding this comment.
Is anything actually partial about this? I suppose there are some error exprs in your downgraded database (replacing the newly supported builtins), but would any CodeQL compatible with the downgraded DB actually break because they're present?
There was a problem hiding this comment.
Good question. What alternative would you propose? full?
Note that I'm going to merge this, but happy to discuss this further. This is something we can easily change after the fact.
There was a problem hiding this comment.
I would have considered full if those error exprs are always leaves on the AST, as they probably shouldn't affect any analysis written in older CodeQL versions. I'm not sure though - for example I believe we have some queries that look for error expressions in a particular function and avoid reporting things in them (like unused variables???), so I suppose results could be lost due to something like that.
... I guess if I'm not sure, partial is probably the best answer in the end.
There was a problem hiding this comment.
Ok let's keep it as partial then. It's at least the safe choice.
See internal PR for details.