Michaelrfairhurst/declarations8 rule 6-2-3 do not duplicate source code#1112
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds new MISRA C++:2023 RULE-6-2-3 queries (and their tests) to detect duplicate entity implementations/types across files and misplaced template specializations, while also refactoring shared anonymous-namespace linkage logic used by multiple queries.
Changes:
- Add a new C++ rule package entry for
RULE-6-2-3and three associated query definitions. - Add unit tests (
.cpp/.h) and.expected/.qlreffiles covering duplicate type definitions and template specialization placement. - Refactor
Linkage.qllanonymous-namespace detection (introducingWithinAnonymousNamespace) and wire the new package into the exclusions metadata, with a change note.
Show a summary per file
| File | Description |
|---|---|
| rule_packages/cpp/Declarations8.json | Registers RULE-6-2-3 and its three queries in the rule package metadata. |
| cpp/misra/src/rules/RULE-6-2-3/SourceCodeImplementedOnlyOnce.ql | New query intended to detect multiple implementations of an entity across source locations. |
| cpp/misra/src/rules/RULE-6-2-3/TemplateSpecializationWrongLocation.ql | New query to flag template specializations declared outside the primary template / specialized-type file. |
| cpp/misra/src/rules/RULE-6-2-3/DuplicateTypeDefinitions.ql | New query to flag duplicate type definitions across files. |
| cpp/misra/test/rules/RULE-6-2-3/test.cpp | Primary test file containing compliant/non-compliant cases and includes for specialization tests. |
| cpp/misra/test/rules/RULE-6-2-3/test2.cpp | Secondary TU to exercise cross-file duplication behavior. |
| cpp/misra/test/rules/RULE-6-2-3/template.h | Defines primary templates and compliant specializations for location tests. |
| cpp/misra/test/rules/RULE-6-2-3/class.h | Defines types used as specialization arguments for location tests. |
| cpp/misra/test/rules/RULE-6-2-3/compliant_specialization.h | Compliant specialization cases (specialized type defined in same file). |
| cpp/misra/test/rules/RULE-6-2-3/noncompliant_specialization.h | Non-compliant specialization cases expected to be flagged. |
| cpp/misra/test/rules/RULE-6-2-3/SourceCodeImplementedOnlyOnce.qlref | Test reference to the production query. |
| cpp/misra/test/rules/RULE-6-2-3/SourceCodeImplementedOnlyOnce.expected | Expected results for the inline-entity duplication query. |
| cpp/misra/test/rules/RULE-6-2-3/TemplateSpecializationWrongLocation.qlref | Test reference to the production query. |
| cpp/misra/test/rules/RULE-6-2-3/TemplateSpecializationWrongLocation.expected | Expected results for the specialization-location query. |
| cpp/misra/test/rules/RULE-6-2-3/DuplicateTypeDefinitions.qlref | Test reference to the production query. |
| cpp/misra/test/rules/RULE-6-2-3/DuplicateTypeDefinitions.expected | Expected results for the duplicate-type-definition query. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Wires the new Declarations8 package into the exclusions metadata dispatch. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Declarations8.qll | Adds autogenerated exclusions metadata and query handles for Declarations8. |
| cpp/common/src/codingstandards/cpp/Linkage.qll | Refactors nested anonymous-namespace detection via WithinAnonymousNamespace. |
| change_notes/2026-04-19-refactor-nested-anonymous-namespace-logic.md | Change note covering the Linkage refactor impact statement. |
Copilot's findings
Comments suppressed due to low confidence (1)
cpp/misra/src/rules/RULE-6-2-3/SourceCodeImplementedOnlyOnce.ql:39
- This query only checks
Function.isInline(), but the alert message says "Inline variable". That’s misleading for users (and contradicts the stated implementation scope). Update the message (and any related wording) to refer to an inline function (or more generally an inline entity) rather than an inline variable.
select d1,
"Inline variable '" + d1.getName() +
"' is defined in multiple files, violating the source code uniqueness requirement."
- Files reviewed: 20/20 changed files
- Comments generated: 3
…larations8-rule-6-2-3-do-not-duplicate-source-code
| inline int16_t global_redefined = 0; // NON_COMPLIANT[False negative] | ||
| inline int16_t global_unique = 0; // COMPLIANT | ||
| inline int16_t global_redeclared = 0; // COMPLIANT | ||
| inline void func_redefined() {} // NON_COMPLIANT |
There was a problem hiding this comment.
is this actually technically compliant?
the rule says inline entities can be defined once per each translation unit, test and test2 are separate translation units? (as long as the definition is the same... however maybe we cant really check that so we should always flag, is that the logic?)
| @@ -0,0 +1,52 @@ | |||
| #include <cstdint> | |||
|
|
|||
| inline int16_t global_redefined = 0; // NON_COMPLIANT[False negative] | |||
There was a problem hiding this comment.
minor adjustment to NON_COMPLIANT[FALSE_NEGATIVE]
| "scope/system" | ||
| ], | ||
| "implementation_scope": { | ||
| "description": "This query does not detect duplicated definitions of inline variables.", |
There was a problem hiding this comment.
maybe note here that the rest of duplicated entities (noninline, vars etc) will be found in some other rules (list those ones)?
There was a problem hiding this comment.
and maybe also elaborate here on which precise case this rule does detect (fairly obvious from the name but maybe only bc I have also read the implementation)
|
|
||
| predicate isInline(FunctionDeclarationEntry d) { d.getDeclaration().isInline() } | ||
|
|
||
| predicate interestedInFunctions(FunctionDeclarationEntry f1, FunctionDeclarationEntry f2) { |
There was a problem hiding this comment.
classic case, I forget how to solve this , but this predicate seems to take a long time on opencv
knewbury01
left a comment
knewbury01
left a comment
There was a problem hiding this comment.
completed partial review, will come back for the rest at a later time, mostly minor comments so far only!
knewbury01
left a comment
knewbury01
left a comment
There was a problem hiding this comment.
another partial review - for duplicate type definitions portion
| "scope/system" | ||
| ], | ||
| "implementation_scope": { | ||
| "description": "This query does not detect duplicated definitions of inline variables.", |
There was a problem hiding this comment.
and maybe also elaborate here on which precise case this rule does detect (fairly obvious from the name but maybe only bc I have also read the implementation)
| "description": "Defining a type with the same fully qualified name in multiple files increases the risk of ODR violations and undefined behavior.", | ||
| "kind": "problem", | ||
| "name": "Duplicate type definitions across files", | ||
| "precision": "very-high", |
There was a problem hiding this comment.
this query finds pairs that are sometimes related, in openpilot the first result I see is one typedef and then a second def where the second one is only defined (in a header) ifndef the other one, which makes a lot of sense I think? maybe we can just lower the precision to high for this one as a result?
|
|
||
| class UserTypeDefinition extends TypeDeclarationEntry { | ||
| UserTypeDefinition() { | ||
| (isDefinition() or getDeclaration() instanceof TypedefType) and |
There was a problem hiding this comment.
ok, I am not sure of this myself but wanted to check
should this rule actually include typedefs at all?
typedefs can be redeclared if they refer to the same type so technically just the presence of 2 with the same name is not an ODR? also , if a typedef is a declaration not a defn, maybe incorrect duplicated typedefs (if the types are not the same) is another rule (not sure?) and wouldnt really be ODR? though I am having a hard time finding a resource on that, other than this which for types just says enum and class types are relevant for ODR
|
|
||
| from UserTypeDefinition t1, UserTypeDefinition t2 | ||
| where | ||
| not isExcluded(t1, Declarations8Package::duplicateTypeDefinitionsQuery()) and |
There was a problem hiding this comment.
I think this should have (maybe , double check, bc this then does cut out dups of things in /usr/include/ headers so not totally sure)
| not isExcluded(t1, Declarations8Package::duplicateTypeDefinitionsQuery()) and | |
| not isExcluded(t1, Declarations8Package::duplicateTypeDefinitionsQuery()) and | |
| not isExcluded(t2, Declarations8Package::duplicateTypeDefinitionsQuery()) and |
Description
This rule is somewhat interesting because 6-2-1 already mostly does things that 6-2-3 does. We can't expressly detect ODR violations, so 6-2-1 looks for repeated names across files. However, there's still room to make 6-2-1 stricter and that's what this query does. Essentially, 6-2-3 gives us permission to flag cases that we considered likely false positives in 6-2-1.
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-6-2-3Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
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.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
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
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.
Reviewer
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.