Skip to content

A7-3-1: Improve performance, reduce false positives #365

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Oct 23, 2023
Merged

A7-3-1: Improve performance, reduce false positives #365

merged 11 commits into from
Oct 23, 2023

Conversation

lcartey
Copy link
Collaborator

@lcartey lcartey commented Sep 29, 2023 •
edited by rvermeulen
Loading

Description

Fixes #359 and #360.

This pull request addresses a number of issues with HiddenInheritedNonOverridableMemberFunction.ql:

  • Addresses potential performance issues and fix false positives related to an incorrect implementation of ordering between using declarations and function declarations in a class. We previously required the using declaration to appear first, but order does not matter.
  • Addresses potential performance issues related to matching functions by name.
  • Reduce duplication by reporting Functions not FunctionDeclarationEntrys in both the hiding and hidden declarations.
  • Exclude special member functions, which cannot be inherited.

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:
    • A7-3-1

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

@lcartey
A7-3-1: Refactor to avoid join on name
46e5eba 
Refactor to:

 1. Avoid any potential performance problems from equating names of two
    member functions.
 2. Report the Declaration, not the DeclarationEntry of the hidden
    function, which reduces duplication.
@lcartey
A7-3-1: Ignore order of using and decl
1263d08 
The filter on order between overriding declaration and using was
problematic because:

 1. It produced a bad join order related to start lines.
 2. It did not check if they were in the same file.
 3. It used the wrong declaration - the order of the overriding
    declaration and the using declaration doesn't matter.

I think the intention was to confirm that the hidden declaration
appeared before the using declaration (so that it was in scope), but
errors of this kind would be identified by the
DefinitionNotConsideredForUnqualifiedLookup.ql query, so there's no
need to factor that in here.
@lcartey
A7-3-1: Report the declaration not the entry
5283bc8 
There is no need to report every declaration entry.

This commit also fixes performance issues that occurred when making this
change.
@lcartey
A7-3-1: Do not report special member functions
2dc3c18 
Special member functions are not inheritable.
@lcartey
Add change note
40ef3c6
rvermeulen
rvermeulen reviewed Oct 2, 2023
View reviewed changes
rvermeulen
rvermeulen reviewed Oct 2, 2023
View reviewed changes
Copy link
Collaborator

@rvermeulen rvermeulen left a comment

Choose a reason for hiding this comment

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

Do we want to apply the same optimization for cpp/autosar/src/rules/A7-3-1/HiddenInheritedOverridableMemberFunction.ql?

lcartey and others added 3 commits October 6, 2023 00:03
@lcartey @rvermeulen
Update cpp/autosar/test/rules/A7-3-1/test.cpp
77ac3d0 
Co-authored-by: Remco Vermeulen <rvermeulen@users.noreply.github.com>
@lcartey
Merge branch 'main' into lcartey/a7-3-1-improvements
4a98c6a
@lcartey
A7-3-1: Enable using to be on any line
6e35de7 
Similar change to NonOverridable case.
@lcartey
Copy link
Collaborator Author

lcartey commented Oct 18, 2023

Do we want to apply the same optimization for cpp/autosar/src/rules/A7-3-1/HiddenInheritedOverridableMemberFunction.ql?

I updated HiddenInheritedOverridableMemberFunction.ql to apply the same logic around using declarations - e.g. not to be sensitive to the line location. I did not observe a need for the other optimizations.

rvermeulen
rvermeulen requested changes Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@rvermeulen rvermeulen left a comment

Choose a reason for hiding this comment

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

A suggestion to remove results that cannot be acted upon by developers.

@lcartey lcartey requested a review from rvermeulen October 22, 2023 23:10
rvermeulen
rvermeulen approved these changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@rvermeulen rvermeulen left a comment

Choose a reason for hiding this comment

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

We keep contraventions in macro expansions and rely on deviations to exclude those that are caused by patterns outside of the control of the developer (e.g., Qt framework)

@rvermeulen rvermeulen enabled auto-merge October 23, 2023 17:34
@rvermeulen rvermeulen added this pull request to the merge queue Oct 23, 2023
Merged via the queue into main with commit c719039 Oct 23, 2023
@rvermeulen rvermeulen deleted the lcartey/a7-3-1-improvements branch October 23, 2023 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvermeulen rvermeulen rvermeulen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Coding Standards Public Development Board
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A7-3-1: HiddenInheritedNonOverridableMemberFunction is very slow
2 participants
@lcartey @rvermeulen