Skip to content

C++: more conservative resolveClass #202

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 3 commits into from
Sep 19, 2018

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Sep 17, 2018

This PR ensures that resolveClass never returns more than one result and does not attempt to match classes by name if they are templates or are nested in a name space or another class. This fixes the problem on LGTM where ChakraCore analysis went into an infinite loop in getQualifiedName because a class appeared to be nested within itself due to name clashes.

A more thorough fix would be to match classes by fully-qualified name, either with a string or a newtype, but at this point in the release cycle I'm proposing a more minimal fix.

The code change here is the same as in #198, where the CPP-Differences job found no query changes. I've extended the tests to try and show the effects of each commit.

jbj added 3 commits September 17, 2018 15:48
These tests exercise the problematic cases where a variable can appear
to have multiple types because of how we fail to account for qualified
names when comparing type names.
Also exclude templates as their names are not canonical.

The test changes in `isfromtemplateinstantiation/` are the inverses of
what we got in 34c9892, which should be a good thing.
@jbj jbj added the C++ label Sep 17, 2018
@jbj jbj added this to the 1.18 milestone Sep 17, 2018
@jbj jbj requested a review from ian-semmle September 17, 2018 19:44
@ian-semmle
Copy link
Contributor

That job compared an ODASA build to itself. I've started another one here: https://jenkins.internal.semmle.com/job/Query-Changes/job/CPP-Differences/412/

@jbj
Copy link
Contributor Author

jbj commented Sep 18, 2018

Good catch! Does the CPP-Differences job not respect qlSubmoduleShaOverride? Or did I put the wrong SHA?

@ian-semmle
Copy link
Contributor

ian-semmle commented Sep 18, 2018

It doesn't know about qlSubmoduleShaOverride (and if it did, there ought to be a qlSubmoduleShaOverride0 too).

@jbj
Copy link
Contributor Author

jbj commented Sep 19, 2018

The query changes LGTM. They have the following summary:

=== Overall changes (new / fixed) ===

                                                                           10 / 2


=== Changes per project (new / fixed) ===

linux                                                                      10 / 0
boost                                                                       0 / 2


=== Changes per query (new / fixed) ===

FEfferentCoupling.ql                                                       10 / 1
IncorrectPointerScalingChar.ql                                              0 / 1

I assume the change to IncorrectPointerScalingChar.ql in boost is just the inverse of the change we saw in #7. It's in a file where there are multiple types named implementation but with different qualifiers, and these changes make it so some of them are considered different. I don't understand the code well enough to tell whether it's the old or new behaviour that's correct, but the new behaviour is certainly more conservative, which is the intention with this PR.

The FEfferentCoupling.ql changes are hard to judge. The alerts say "Outgoing dependencies per file too high (N). It is a warning for the value to be higher or equal to 120" for various values of N close to 120. It seems reasonable to me that this query wobbles a bit under these changes.

@ian-semmle ian-semmle merged commit 4b0ab60 into github:rc/1.18 Sep 19, 2018
aibaars added a commit that referenced this pull request Oct 14, 2021
HardCodedCredentials: fix query metadata comment
smowton pushed a commit to smowton/codeql that referenced this pull request Apr 16, 2022
Change locations of parameterized types to class file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants