A5-2-2: Exclude results in uninstantiated templates, explain limitations #160

lcartey · 2023-01-09T23:18:09Z

Description

The scope of this rule is intended to be so-called "c-style" casts. Unfortunately, our query for this rule has both false positives and false negatives due the way such casts are modelled in CodeQL.

Firstly, the CStyleCast CodeQL class includes "function notation" casts such as:

int(x);
A(x); // where `A` is some type with a suitable single argument constructor

I don't believe these are intended to part of the AUTOSAR rule, as they are not legal C casts, and therefore I don't think they count as "C-Style". Furthermore, the distinction is lost at the database schema level, so there's no universal query-level approach to distinguish between c-style and functional notation casts.

Secondly, if a c-style (or functional notation) cast results in a constructor call to a single argument constructor, the CStyleCast is not added to the database, and instead is replaced with the direct ConstructorCall. Again, at a database schema level, there is no universal way to determine if a particular constructor call was created from a cast, as the information has been lost.

As a consequence this query:

Produces false positives when primitive types are cast using the "functional notation" syntax.
Produces false negatives when a C-style cast is converted to a ConstructorCall e.g. when the argument type is compatible with a single-argument constructor.

This PR:

Adds the clarifications above to the query, the tests and the implementation_scope.
Addresses one small edge case that we can control, by not reporting casts on template parameters, and instead relying on reporting casts in the specific instantiations of the class. This avoid false positives where the functional notation cast is used to "call" a constructor e.g. T(x), and where in the instantiation the cast will be replaced with a ConstructorCall and therefore not reported. In practice, I believe this will remove the most common cause of false positives.

Change request type

Release or process automation (GitHub workflows, internal scripts)
Internal documentation
External documentation
Query files (.ql, .qll, .qls or unit tests)
External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

No rules added
Queries have been added for the following rules:
- rule number here
Queries have been modified for the following rules:
- A5-2-2

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

The structure or layout of the release artifacts.
The evaluation performance (memory, execution time) of an existing query.
The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

Yes
No

🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

Confirmed

Query development review checklist

For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:

Author

Have all the relevant rule package description files been checked in?
Have you verified that the metadata properties of each new query is set appropriately?
Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
Are the alert messages properly formatted and consistent with the style guide?
Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Does the query have an appropriate level of in-query comments/documentation?
Have you considered/identified possible edge cases?
Does the query not reinvent features in the standard library?
Can the query be simplified further (not golfed!)

Reviewer

Have all the relevant rule package description files been checked in?
Have you verified that the metadata properties of each new query is set appropriately?
Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
Are the alert messages properly formatted and consistent with the style guide?
Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Does the query have an appropriate level of in-query comments/documentation?
Have you considered/identified possible edge cases?
Does the query not reinvent features in the standard library?
Can the query be simplified further (not golfed!)

Clarify what `CStyleCast` does and does not cover by adding a comment, expanding the test case and providing an implementation scope. In addition, exclude casts on template parameters to avoid unnecessary false positives.

Any cast in an uninstantiated template that is related to the template parameter may be converted to a `ConstructorCall` when the template is instantiated. To avoid the common false positive case where the functional cast notation is used to call a constructor, we exclude all results in uninstantiated templates and instead rely on reporting results in template instantiations instead.

jsinglet · 2023-01-11T17:53:00Z

Note for author and reviewer: https://en.cppreference.com/w/cpp/language/explicit_cast

Take a look at the "Ambiguity Resolution" section -- there are some interesting cases that look probable.

mbaluda · 2023-01-12T16:31:43Z

Note for author and reviewer: https://en.cppreference.com/w/cpp/language/explicit_cast
Take a look at the "Ambiguity Resolution" section -- there are some interesting cases that look probable.

These are good examples to test the extractor but do not seem directly useful for this query.
I created a DB and confirmed that ill-formed statements do not compile, and that function and object declarations don't get mixed up.

mbaluda

LGTM

github-actions · 2023-03-06T20:34:24Z

🤖 Beep Boop! Matrix Testing for this PR has been initiated. Please check back later for results.

💡 If you do not hear back from me please check my status! I will report even if this PR does not contain files eligible for matrix testing.

jsinglet · 2023-03-06T20:35:15Z

🤖 Beep Boop! gcc/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

jsinglet · 2023-03-06T20:35:18Z

🤖 Beep Boop! clang/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

github-actions · 2023-03-06T20:35:20Z

🤖 Beep Boop! Matrix Testing for this PR has been initiated. Please check back later for results.

💡 If you do not hear back from me please check my status! I will report even if this PR does not contain files eligible for matrix testing.

jsinglet · 2023-03-06T20:35:29Z

🤖 Beep Boop! clang/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


QUERY                : TraditionalCStyleCastsUsed
RULE                 : A5-2-2
TEST_DIFFERENCE      : 
SUITE                : AUTOSAR
COMPILE_PASS         : False
COMPILE_ERROR_OUTPUT : [2023-03-06 20:35:28] [build-stderr] /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp:74:10: fatal error: 'macro_c_style_casts.h' file not found
                       [2023-03-06 20:35:28] [build-stderr] #include "macro_c_style_casts.h"
                       [2023-03-06 20:35:28] [build-stderr]          ^~~~~~~~~~~~~~~~~~~~~~~
                       [2023-03-06 20:35:28] [build-stderr] 1 error generated.
                       [2023-03-06 20:35:28] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql/tools/linux64/preload_tracer, clang++, -std=c++14, -fsyntax-only, /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp])
                       
TEST_PASS            : False
PACKAGE              : BannedSyntax

jsinglet · 2023-03-06T20:37:24Z

🤖 Beep Boop! clang/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

jsinglet · 2023-03-06T20:37:24Z

🤖 Beep Boop! gcc/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

jsinglet · 2023-03-06T20:37:28Z

🤖 Beep Boop! gcc/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


TEST_PASS            : False
SUITE                : AUTOSAR
QUERY                : TraditionalCStyleCastsUsed
PACKAGE              : BannedSyntax
COMPILE_PASS         : False
RULE                 : A5-2-2
COMPILE_ERROR_OUTPUT : [2023-03-06 20:37:27] [build-stderr] /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp:74:10: fatal error: macro_c_style_casts.h: No such file or directory
                       [2023-03-06 20:37:27] [build-stderr]  #include "macro_c_style_casts.h"
                       [2023-03-06 20:37:27] [build-stderr]           ^~~~~~~~~~~~~~~~~~~~~~~
                       [2023-03-06 20:37:27] [build-stderr] compilation terminated.
                       [2023-03-06 20:37:28] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql/tools/linux64/preload_tracer, g++, -std=c++14, -fsyntax-only, /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp])
                       
TEST_DIFFERENCE      :

jsinglet · 2023-03-06T20:37:32Z

🤖 Beep Boop! clang/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


COMPILE_PASS         : False
PACKAGE              : BannedSyntax
QUERY                : TraditionalCStyleCastsUsed
RULE                 : A5-2-2
COMPILE_ERROR_OUTPUT : [2023-03-06 20:37:31] [build-stderr] /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp:74:10: fatal error: 'macro_c_style_casts.h' file not found
                       [2023-03-06 20:37:31] [build-stderr] #include "macro_c_style_casts.h"
                       [2023-03-06 20:37:31] [build-stderr]          ^~~~~~~~~~~~~~~~~~~~~~~
                       [2023-03-06 20:37:31] [build-stderr] 1 error generated.
                       [2023-03-06 20:37:32] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql/tools/linux64/preload_tracer, clang++, -std=c++14, -fsyntax-only, /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp])
                       
TEST_PASS            : False
SUITE                : AUTOSAR
TEST_DIFFERENCE      :

jsinglet · 2023-03-06T20:37:47Z

🤖 Beep Boop! gcc/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


PACKAGE              : BannedSyntax
QUERY                : TraditionalCStyleCastsUsed
SUITE                : AUTOSAR
RULE                 : A5-2-2
COMPILE_PASS         : False
COMPILE_ERROR_OUTPUT : [2023-03-06 20:37:46] [build-stderr] /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp:74:10: fatal error: macro_c_style_casts.h: No such file or directory
                       [2023-03-06 20:37:46] [build-stderr]  #include "macro_c_style_casts.h"
                       [2023-03-06 20:37:46] [build-stderr]           ^~~~~~~~~~~~~~~~~~~~~~~
                       [2023-03-06 20:37:46] [build-stderr] compilation terminated.
                       [2023-03-06 20:37:46] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql/tools/linux64/preload_tracer, g++, -std=c++14, -fsyntax-only, /__w/codeql-coding-standards-release-engineering/codeql-coding-standards-release-engineering/codeql-coding-standards/cpp/autosar/test/rules/A5-2-2/test.cpp])
                       
TEST_DIFFERENCE      : 
TEST_PASS            : False

jsinglet · 2023-03-06T20:37:47Z

🤖 Beep Boop! Matrix Testing for this PR has been completed. If no reports were posted it means this PR does not contain things that need matrix testing!

jsinglet · 2023-03-06T20:38:40Z

🤖 Beep Boop! Matrix Testing for this PR has been completed. If no reports were posted it means this PR does not contain things that need matrix testing!

github-actions · 2023-03-06T21:05:28Z

🤖 Beep Boop! Matrix Testing for this PR has been initiated. Please check back later for results.

💡 If you do not hear back from me please check my status! I will report even if this PR does not contain files eligible for matrix testing.

jsinglet · 2023-03-06T21:06:22Z

🤖 Beep Boop! gcc/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

jsinglet · 2023-03-06T21:06:24Z

🤖 Beep Boop! clang/c/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test!

jsinglet · 2023-03-06T21:07:13Z

🤖 Beep Boop! clang/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


RULE                 : A5-2-2
QUERY                : TraditionalCStyleCastsUsed
COMPILE_PASS         : True
TEST_DIFFERENCE      : 
TEST_PASS            : True
PACKAGE              : BannedSyntax
SUITE                : AUTOSAR
COMPILE_ERROR_OUTPUT :

jsinglet · 2023-03-06T21:07:13Z

🤖 Beep Boop! gcc/cpp/x86_64 Matrix Testing for this PR has been completed. See below for the results!


SUITE                : AUTOSAR
TEST_DIFFERENCE      : 
COMPILE_PASS         : True
COMPILE_ERROR_OUTPUT : 
QUERY                : TraditionalCStyleCastsUsed
PACKAGE              : BannedSyntax
RULE                 : A5-2-2
TEST_PASS            : True

jsinglet · 2023-03-06T21:07:54Z

🤖 Beep Boop! Matrix Testing for this PR has been completed. If no reports were posted it means this PR does not contain things that need matrix testing!

lcartey added 3 commits January 9, 2023 22:59

A5-2-2: Clarify c-style casts scope, exclude templates

dc0d14a

Clarify what `CStyleCast` does and does not cover by adding a comment, expanding the test case and providing an implementation scope. In addition, exclude casts on template parameters to avoid unnecessary false positives.

Add change note

7dbc9ad

lcartey changed the title ~~A5-2-2: Exclude template parameters, explain limitations~~ A5-2-2: Exclude results in uninstantiated templates, explain limitations Jan 11, 2023

lcartey self-assigned this Jan 11, 2023

lcartey requested a review from mbaluda January 11, 2023 17:37

mbaluda approved these changes Jan 12, 2023

View reviewed changes

Merge branch 'main' into lcartey/c-style-cast-improvements

14b057f

lcartey enabled auto-merge January 26, 2023 15:43

ghost approved these changes Mar 6, 2023

View reviewed changes

Fix test formatting

602ab4b

mbaluda disabled auto-merge March 6, 2023 20:34

Merge branch 'main' into lcartey/c-style-cast-improvements

816ca62

Include custom library

396bdf0

mbaluda enabled auto-merge (squash) March 6, 2023 21:10

mbaluda merged commit 5a13000 into github:main Mar 6, 2023

nicolaswill mentioned this pull request Jan 25, 2024

Missing attestations: @mbaluda #505

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A5-2-2: Exclude results in uninstantiated templates, explain limitations #160

A5-2-2: Exclude results in uninstantiated templates, explain limitations #160

lcartey commented Jan 9, 2023 •

edited by mbaluda

jsinglet commented Jan 11, 2023

mbaluda commented Jan 12, 2023

mbaluda left a comment

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

A5-2-2: Exclude results in uninstantiated templates, explain limitations #160

A5-2-2: Exclude results in uninstantiated templates, explain limitations #160

Conversation

lcartey commented Jan 9, 2023 • edited by mbaluda

Description

Change request type

Rules with added or modified queries

Release change checklist

Query development review checklist

Author

Reviewer

jsinglet commented Jan 11, 2023

mbaluda commented Jan 12, 2023

mbaluda left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

jsinglet commented Mar 6, 2023

lcartey commented Jan 9, 2023 •

edited by mbaluda