Add linkage1 package #1016
Add linkage1 package #1016
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new "Linkage1" package containing shared implementations for two MISRA-C++-2023 rules (RULE-6-0-2 and RULE-6-5-1). These rules share implementation with existing AUTOSAR rules (A3-1-4 and A3-3-1). The PR refactors existing code to use shared query implementations, migrates tests to common locations, and updates metadata accordingly.
Changes:
- Adds Linkage1 package with shared implementations for external linkage array size and declaration rules
- Refactors AUTOSAR rules A3-1-4 and A3-3-1 to use shared implementations
- Adds MISRA-C++-2023 rules RULE-6-0-2 and RULE-6-5-1 that reuse these shared implementations
Reviewed changes
Copilot reviewed 27 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Updates package assignments for RULE-6-0-2 and RULE-6-5-1 to use new Linkage1 package |
| rule_packages/cpp/Scope.json | Renames short_name to include "Autosar" suffix and adds shared_implementation_short_name |
| rule_packages/cpp/Linkage1.json | New package definition for MISRA-C++-2023 linkage rules |
| rule_packages/cpp/Includes.json | Adds shared_implementation_short_name to existing A3-3-1 rule |
| cpp/misra/test/rules/RULE-6-5-1/ExternalLinkageNotDeclaredInHeaderFileMisra.testref | Test reference pointing to shared test implementation |
| cpp/misra/test/rules/RULE-6-0-2/ExternalLinkageArrayWithoutExplicitSizeMisra.testref | Test reference pointing to shared test implementation |
| cpp/misra/src/rules/RULE-6-5-1/ExternalLinkageNotDeclaredInHeaderFileMisra.ql | New MISRA query using shared implementation |
| cpp/misra/src/rules/RULE-6-0-2/ExternalLinkageArrayWithoutExplicitSizeMisra.ql | New MISRA query using shared implementation |
| cpp/common/test/rules/externallinkagenotdeclaredinheaderfile/test.hpp | Shared test header file |
| cpp/common/test/rules/externallinkagenotdeclaredinheaderfile/test.cpp | Shared test implementation with COMPLIANT/NON_COMPLIANT cases |
| cpp/common/test/rules/externallinkagenotdeclaredinheaderfile/ExternalLinkageNotDeclaredInHeaderFile.ql | Shared test query |
| cpp/common/test/rules/externallinkagenotdeclaredinheaderfile/ExternalLinkageNotDeclaredInHeaderFile.expected | Expected test results |
| cpp/common/test/rules/externallinkagearraywithoutexplicitsize/test.cpp | Shared test implementation with COMPLIANT/NON_COMPLIANT cases |
| cpp/common/test/rules/externallinkagearraywithoutexplicitsize/ExternalLinkageArrayWithoutExplicitSize.ql | Shared test query |
| cpp/common/test/rules/externallinkagearraywithoutexplicitsize/ExternalLinkageArrayWithoutExplicitSize.expected | Expected test results |
| cpp/common/src/codingstandards/cpp/rules/externallinkagenotdeclaredinheaderfile/ExternalLinkageNotDeclaredInHeaderFile.qll | Shared implementation for external linkage declaration rule |
| cpp/common/src/codingstandards/cpp/rules/externallinkagearraywithoutexplicitsize/ExternalLinkageArrayWithoutExplicitSize.qll | Shared implementation for array size rule |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Scope.qll | Renames query to include "Autosar" suffix |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Imports and registers new Linkage1 package |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Linkage1.qll | New exclusions file for Linkage1 package |
| cpp/autosar/test/rules/A3-3-1/ExternalLinkageNotDeclaredInHeaderFile.testref | Updated to point to shared test |
| cpp/autosar/test/rules/A3-3-1/ExternalLinkageNotDeclaredInHeaderFile.qlref | Removed (migrated to testref) |
| cpp/autosar/test/rules/A3-1-4/test.cpp | Removed (migrated to common tests) |
| cpp/autosar/test/rules/A3-1-4/ExternalLinkageArrayWithoutExplicitSizeAutosar.testref | Updated to point to shared test |
| cpp/autosar/test/rules/A3-1-4/ExternalLinkageArrayWithoutExplicitSize.qlref | Removed (migrated to testref) |
| cpp/autosar/test/rules/A3-1-4/ExternalLinkageArrayWithoutExplicitSize.expected | Removed (migrated to common tests) |
| cpp/autosar/src/rules/A3-3-1/ExternalLinkageNotDeclaredInHeaderFile.ql | Refactored to use shared implementation |
| cpp/autosar/src/rules/A3-1-4/ExternalLinkageArrayWithoutExplicitSizeAutosar.ql | New file using shared implementation (renamed) |
| cpp/autosar/src/rules/A3-1-4/ExternalLinkageArrayWithoutExplicitSize.ql | Removed (renamed to ExternalLinkageArrayWithoutExplicitSizeAutosar.ql) |
| change_notes/2026-01-15-a3-1-4-extern-to-full.md | Change note documenting the refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Introducing a function or object with external linkage outside of a header file can | ||
| * cause developer confusion about its translation unit access semantics. |
There was a problem hiding this comment.
The documentation comment describes the wrong issue. This file is about arrays with external linkage declared without explicit size, but the comment describes "Introducing a function or object with external linkage outside of a header file" which is actually the description for the other rule (ExternalLinkageNotDeclaredInHeaderFile). The comment should describe the array size issue instead.
| * Introducing a function or object with external linkage outside of a header file can | |
| * cause developer confusion about its translation unit access semantics. | |
| * Declaring an array with external linkage without explicitly specifying its size | |
| * can lead to unclear or inconsistent expectations about the array's bounds. |
There was a problem hiding this comment.
Seems copilot is right here
| "' with external linkage doesn't specify the size explicitly." and | ||
| hasExternalLinkage(v) and | ||
| not arrayType.hasArraySize() and | ||
| // Holds is if declEntry is an array variable declaration (not a definition) |
There was a problem hiding this comment.
Grammatical error: "Holds is if" should be "Holds if". Remove the word "is".
| // Holds is if declEntry is an array variable declaration (not a definition) | |
| // Holds if declEntry is an array variable declaration (not a definition) |
knewbury01
requested review from
MichaelRFairhurst,
jeongsoolee09,
jketema and
mbaluda
and removed request for
jketema
MichaelRFairhurst
left a comment
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
Just small suggestions!
This looks great!
And ❗ great find ❗ that, not only could this logic be shared, but also, it could be improved!! 🎉
| @@ -0,0 +1,2 @@ | |||
| - `A3-1-4` - `ExternalLinkageArrayWithoutExplicitSizeAutosar.ql`: | |||
| - `ExternalLinkageArrayWithoutExplicitSize.ql` has been renamed to `ExternalLinkageArrayWithoutExplicitSizeAutosar.ql` to reflect shared query implementation. Additionally the query previously only detected explicit uses of `extern` to determine external linkage, and now would catch other cases that are possible where it is external linkage and an array is declared without an explicit size. | |||
There was a problem hiding this comment.
Very nice to catch and fix this!
| * Introducing a function or object with external linkage outside of a header file can | ||
| * cause developer confusion about its translation unit access semantics. |
There was a problem hiding this comment.
Seems copilot is right here
| // Structs must have at least one non-flexible array member. | ||
| int foo; | ||
|
|
||
| // static data members have external linkage - but not currently detected in |
There was a problem hiding this comment.
Can you add a case for this?
static int[] x; // NON_COMPLIANT[False negative]
| "The declared array '" + declEntry.getName() + | ||
| "' with external linkage doesn't specify the size explicitly." and | ||
| hasExternalLinkage(v) and | ||
| not arrayType.hasArraySize() and |
There was a problem hiding this comment.
I'm surprised that hasArraySize was working before..!
Below are a bunch of thoughts that kind of go down a rabbit hole. Consider it 100% up to you how you want to proceed -- you could file a bug and copy my comment, or you can explore it, add tests, etc.
We may not be able to do much about it, but it may be worth adding some .hpp test cases to see if the following are FNs:
// test.hpp
int header_only[]; // NON_COMPLIANT
int header_and_cpp[]; // NON_COMPLIANT
// test.cpp
#include "test.hpp"
// definition associated with a declaration from test.hpp
int header_and_cpp[10] = ...
Basically, I fear that header_and_cpp[] will be a false negative. It should be extracted as one Variable, with one Type -- that type probably being an int array of size 10.
If this is a FN we can write NON_COMPLIANT[False negative] along with an explanatory comment, and I'm not sure if we have any recourse to fix it if so. However, we likely don't have to fix it.....if it is a FN, we probably also want to add a test like:
// test.hpp
// an array declaration that's defined in test1.cpp but not test2.cpp
int defined_in_test1_cpp[]; // NON_COMPLIANT
// test1.cpp
int defined_in_test1_cpp[10] = ...;
// test2.cpp
// This file exists to _not_ do the following:
// int defined_in_test1_cpp[10] = ...;
// So that we can test that a violation of the One Definition Rule is caught in this case
My guess is that this will be correctly reported as non compliant because we would have two variables (a violation of ODR), and one would have no size. This would means the NON_COMPLIANT[False negative] is low impact.
Let me know if any of this is unclear, feel free to reach out via chat etc. Up to you what next steps you think may be best!
2 participants
Description
adds a package for linkage1 - contains 2 rules which are shared implementations
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
Release 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.