Skip to content

Java: Fix Field.getInitializer() matching non-initializer assignments #6485

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 2 commits into from
Aug 24, 2021

Conversation

Marcono1234
Copy link
Contributor

Previously Field.getInitializer() would erroneously match assignments which occurred in the intializer of a different field. For example:

class FieldInitializer {
    String f = "a";
    // Erroneously considered `f = "b"` to be initializing `f`
    String otherF = f = "b";
}

Query:

import java

from Field f
where f.getCompilationUnit().hasName("FieldInitializer")
select f, f.getInitializer()

@Marcono1234 Marcono1234 requested a review from a team as a code owner August 14, 2021 20:57
@github-actions github-actions bot added the Java label Aug 14, 2021
Comment on lines 587 to 590
exprStmt.getExpr() = e and
// This check also rules out assignments in explicit initializer blocks
// (CodeQL models explicit initializer blocks as BlockStmt in initializer methods)
exprStmt.getParent() = im.getBody()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add pragma[only_bind_out] here again? If so to which of these predicate usages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried reproducing the bad join order observed in #6187, but the latest codeql doesn't produce that order any longer. We should just run a performance evaluation on this to see if it behaves badly.

@smowton smowton added the no-change-note-required This PR does not need a change note label Aug 24, 2021
@smowton smowton force-pushed the marcono1234/field-initializer-fix branch from 68b00a5 to 5a2dfda Compare August 24, 2021 13:04
@smowton smowton merged commit 2689c13 into github:main Aug 24, 2021
@Marcono1234 Marcono1234 deleted the marcono1234/field-initializer-fix branch August 24, 2021 21:06
@Marcono1234
Copy link
Contributor Author

Thanks for adding the test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants