Skip to content

C++: Retrieve namespace attributes #19773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 17, 2025
Merged

Conversation

IdrissRio
Copy link
Contributor

  • This PR changes the dbscheme. It adds a new table namespaceattributes to store namespace attributes.
  • Add a predicate getAnAttribute to Namespace to retrieve a namespace attribute.

@IdrissRio IdrissRio added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jun 16, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/namespace-attributes branch from af93668 to 9c2cc35 Compare June 16, 2025 12:27
@IdrissRio IdrissRio force-pushed the idrissrio/namespace-attributes branch from 9c2cc35 to 09bc57a Compare June 16, 2025 12:34
@IdrissRio IdrissRio marked this pull request as ready for review June 17, 2025 07:30
@Copilot Copilot AI review requested due to automatic review settings June 17, 2025 07:30
@IdrissRio IdrissRio requested a review from a team as a code owner June 17, 2025 07:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for storing and querying attributes on C++ namespaces by adding a new database table and a corresponding QL predicate.

  • Add namespaceattributes table to the database schema for mapping namespaces to attribute specs
  • Implement getAnAttribute in Namespace.qll to fetch namespace attributes
  • Update upgrade/downgrade scripts and change notes to reflect the new feature

Reviewed Changes

Copilot reviewed 5 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cpp/ql/lib/upgrades/.../upgrade.properties Add description and backwards compatibility for the new upgrade
cpp/ql/lib/semmlecode.cpp.dbscheme Define new namespaceattributes table with refs to namespace and attribute
cpp/ql/lib/semmle/code/cpp/Namespace.qll Implement getAnAttribute predicate in the Namespace class
cpp/ql/lib/change-notes/2025-06-16-namespace-attributes.md Document the addition of namespace attribute support
cpp/downgrades/.../upgrade.properties Add downgrade step to remove namespaceattributes table
Comments suppressed due to low confidence (2)

cpp/ql/lib/semmle/code/cpp/Namespace.qll:104

  • There are no tests covering this new predicate. Consider adding QueryTest cases to verify that getAnAttribute correctly retrieves attributes for namespaces under various scenarios.
Attribute getAnAttribute() {

cpp/ql/lib/semmle/code/cpp/Namespace.qll:103

  • [nitpick] The doc comment could be more precise about the behavior: does it return all attributes or just one? Also clarify what happens if a namespace has no attributes.
/** Gets an attribute of this namespace. */


/** Gets an attribute of this namespace. */
Attribute getAnAttribute() {
namespaceattributes(underlyingElement(this), unresolveElement(result))
Copy link
Preview

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The predicate call isn’t assigned to the result variable, so the method won’t return any values. It should read:

result = namespaceattributes(underlyingElement(this), unresolveElement(result));
Suggested change
namespaceattributes(underlyingElement(this), unresolveElement(result))
result = namespaceattributes(underlyingElement(this), unresolveElement(result))

Copilot uses AI. Check for mistakes.

@jketema
Copy link
Contributor

jketema commented Jun 17, 2025

Could you add a new test directory to cpp/ql/test/library-tests/attributes that has a test in it that uses the new predicate?

@IdrissRio
Copy link
Contributor Author

Could you add a new test directory to cpp/ql/test/library-tests/attributes that has a test in it that uses the new predicate?

I added the tests as you suggested. I noticed that some namespaces show up with the location file://:0:0:0:0, while others point to the actual test file (see below). I'm not sure why this happens, but it doesn't seem related to the attributes.

| file://:0:0:0:0 | NamespaceTest | test.cpp:3:26:3:37 | maybe_unused |
| test.cpp:4:53:4:61 | MultiAttr | test.cpp:4:26:4:35 | deprecated |

@jketema
Copy link
Contributor

jketema commented Jun 17, 2025

Looks like that has to do with there being multiple namespace declarations vs just one.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@IdrissRio IdrissRio merged commit a702f7a into main Jun 17, 2025
15 of 16 checks passed
@IdrissRio IdrissRio deleted the idrissrio/namespace-attributes branch June 17, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants