Implement RULE-2-8, project should not contain unused object definitions #784

MichaelRFairhurst · 2024-10-25T06:27:51Z

Description

Implement RULE-2-8, project should not contain unused object definitions

Also add a new AlertReporting shared query library for deduplicating results across macro definitions/invocations/etc.

Split __attribute__((unused)) variables (and similar) to a Strict pair of queries.

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-2-8
Queries have been modified for the following rules:
- rule number here

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!)

…ons. Also add a new AlertReporting shared query library for deduplicating results across macro definitions/invocations/etc. Split __attribute__((unused)) variables (and similar) to a Strict pair of queries.

lcartey

Thanks! Providing some initial feedback. I still need to review the deduplicate macro code.

cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll

c/misra/test/rules/RULE-2-8/test.c

c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql

…lement-deadcode-2-unused-object-definitions

MichaelRFairhurst · 2024-12-10T18:04:44Z

Apologies, I forgot I had unaddressed feedback here! Incorporated.

lcartey

Thanks! A few minor housekeeping bits, and some suggestions around the macro deduplication pieces.

lcartey · 2025-01-07T23:00:38Z

docs/development_handbook.md

@@ -51,6 +51,8 @@ Each coding standard consists of a list of "guidelines", however not all the gui

 For some of the rules which are not amenable to static analysis, we may opt to provide a query which aids with "auditing" the rules. For example, AUTOSAR includes a rule (A10-0-1) "Public inheritance shall be used to implement 'is-a' relationship.". This is not directly amenable to static analysis, because it requires external context around the concept being modeled. However, we can provide an "audit" rule which reports all the public and private inheritance relationships in the program, so they can be manually verified.

+For other rules, there may be means of indicating that a contravention is intentional, and where requiring a _devation report_ may be extra burdensome on developers and require double-entry. These results should be reported under a "strict" query. For instance, `RULE-2-8` "A project should not contain unused object definitions," where adding `__attribute__((unused))` may be preferable in order to suppress compiler warnings (which _deviation reports_ do not do) and are highly indicative of an intentional contravention by a developer.


There's a versioning header that needs to be updated at the top of this document.

lcartey · 2025-01-07T23:00:44Z

docs/user_manual.md

@@ -74,6 +74,7 @@ For each rule we therefore identify whether it is supportable or not. Furthermor

 - **Automated** - the queries for the rule find contraventions directly.
 - **Audit only** - the queries for the rule does not find contraventions directly, but instead report a list of _candidates_ that can be used as input into a manual audit. For example, `A10-0-1` (_Public inheritance shall be used to implement 'is-a' relationship_) is not directly amenable to static analysis, but CodeQL can be used to produce a list of all the locations that use public inheritance so they can be manually reviewed.
+- **Strict only** - the queries for the rule find contraventions directly, but find results which are strongly indicated to be intentional, and where adding a _deviation report_ may be extra burden on developers. For example, in `RULE-2-8` (_A project should not contain unused object definitions_), declaring objects with `__attribute__((unused))` may be preferable to a _deviation report_, which will not suppress relevant compiler warnings, and therefore would otherwise require developer double-entry.


There's a versioning header that needs to be updated at the top of this document.

c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql

lcartey · 2025-01-07T23:48:15Z

cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll

+  }
+
+  string getMessageVariedResultInAllExpansions(Macro m) {
+    result = "Macro '" + m.getName() + "' defines unused object of varied names, for example, '$@'."


Suggestion on wording:

Suggested change

result = "Macro '" + m.getName() + "' defines unused object of varied names, for example, '$@'."

result = "Macro '" + m.getName() + "' defines unused object with an invocation dependent name, for example, '$@'."

lcartey · 2025-01-07T23:52:05Z

cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll

+ *     report.getOptionalPlaceholderMessage()
+ * ```
+ *
+ * Note that this does *not* (currently) generate a result for elements not contained by a macro.


I'm wondering if it would create a cleaner interface to do both in this library? That would reduce the number of queries required in this case to two queries, instead of four. It would also mean we could replace/integrate MacroUnwrapper, which is only currently used by one query.

lcartey · 2025-01-07T23:57:04Z

cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll

+   * `ResultElement`, indicating that the real problem lies with that other `Macro` instead of with
+   * this particular invocation.
+   */
+  class IsolatedMacroExpansionWithResultElement extends ResultMacroExpansion {


I wonder if we should exclude LibraryMacros from this case, for consistency - as results grouped by a library macro will be excluded (because the location is outside the source archive).

See A5-2-2 for an example where we already do this.

lcartey · 2025-01-07T23:57:34Z

cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll

+ * To do report such cases, either add support for that in this module, or write a separate query
+ * that reports `InvalidFoo` cases where not `.isInMacroExpansion()`.
+ */
+module DeduplicateMacroResults<


I believe we could use this module in A5-2-2, which already has some manual handling for duplicate macro results, but could benefit from the improvements in this module. I don't think we need to do it in this PR, but should open an issue to track this as a follow up item.

lcartey · 2025-01-07T23:57:51Z

cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll

+class PotentiallyUnusedLocalVariable extends BasePotentiallyUnusedLocalVariable {
+  PotentiallyUnusedLocalVariable() {
+    // Ignore variables declared in macro expansions
+    not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion())


The existing queries simply ignore variables declared in macro expansions - we should add an issue to review backporting the macro handling capabilities from this PR.

MichaelRFairhurst added 2 commits October 24, 2024 23:23

Implement RULE-2-8, project should not contain unused object definiti…

1340cdb

…ons. Also add a new AlertReporting shared query library for deduplicating results across macro definitions/invocations/etc. Split __attribute__((unused)) variables (and similar) to a Strict pair of queries.

Fix strict misra tag in rules schema

ff562f9

MichaelRFairhurst requested review from lcartey and data-douser October 25, 2024 06:58

lcartey reviewed Nov 25, 2024

View reviewed changes

cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll Show resolved Hide resolved

c/misra/test/rules/RULE-2-8/test.c Show resolved Hide resolved

c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql Show resolved Hide resolved

MichaelRFairhurst added 4 commits December 9, 2024 16:37

Address review feedback

125650c

Merge remote-tracking branch 'origin/main' into michaelrfairhurst/imp…

daa68d3

…lement-deadcode-2-unused-object-definitions

Format

9492933

format c

8c31c8c

MichaelRFairhurst requested a review from lcartey December 10, 2024 18:04

lcartey requested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RULE-2-8, project should not contain unused object definitions #784

Implement RULE-2-8, project should not contain unused object definitions #784

MichaelRFairhurst commented Oct 25, 2024

lcartey left a comment

MichaelRFairhurst commented Dec 10, 2024

lcartey left a comment

lcartey Jan 7, 2025

lcartey Jan 7, 2025

lcartey Jan 7, 2025

lcartey Jan 7, 2025

lcartey Jan 7, 2025

lcartey Jan 7, 2025

lcartey Jan 7, 2025

		@@ -51,6 +51,8 @@ Each coding standard consists of a list of "guidelines", however not all the gui

		For some of the rules which are not amenable to static analysis, we may opt to provide a query which aids with "auditing" the rules. For example, AUTOSAR includes a rule (A10-0-1) "Public inheritance shall be used to implement 'is-a' relationship.". This is not directly amenable to static analysis, because it requires external context around the concept being modeled. However, we can provide an "audit" rule which reports all the public and private inheritance relationships in the program, so they can be manually verified.

		For other rules, there may be means of indicating that a contravention is intentional, and where requiring a _devation report_ may be extra burdensome on developers and require double-entry. These results should be reported under a "strict" query. For instance, `RULE-2-8` "A project should not contain unused object definitions," where adding `__attribute__((unused))` may be preferable in order to suppress compiler warnings (which _deviation reports_ do not do) and are highly indicative of an intentional contravention by a developer.

	result = "Macro '" + m.getName() + "' defines unused object of varied names, for example, '$@'."
	result = "Macro '" + m.getName() + "' defines unused object with an invocation dependent name, for example, '$@'."

Implement RULE-2-8, project should not contain unused object definitions #784

Are you sure you want to change the base?

Implement RULE-2-8, project should not contain unused object definitions #784

Conversation

MichaelRFairhurst commented Oct 25, 2024

Description

Change request type

Rules with added or modified queries

Release change checklist

Query development review checklist

Author

Reviewer

lcartey left a comment

Choose a reason for hiding this comment

MichaelRFairhurst commented Dec 10, 2024

lcartey left a comment

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment

lcartey Jan 7, 2025

Choose a reason for hiding this comment