Skip to content

Java: Fix overrides to not be transitive.#7132

Merged
aschackmull merged 4 commits into
github:mainfrom
aschackmull:java/overrides
Nov 17, 2021
Merged

Java: Fix overrides to not be transitive.#7132
aschackmull merged 4 commits into
github:mainfrom
aschackmull:java/overrides

Conversation

@aschackmull
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull commented Nov 15, 2021

The Method.overrides(Method) relation was accidentally transitive. This has been a minor bit of technical debt for a long time, but isn't expected to have much impact as uses are mostly of the transitive form a.overrides*(b) and the qldoc has been claiming that only direct overrides were being included.

@aschackmull aschackmull requested a review from a team as a code owner November 15, 2021 12:55
atorralba
atorralba reviewed Nov 15, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

So, this is now expected to only return the directly (i.e. first) overridden method in the hierarchy, instead of all the methods with the same signature in all supertypes, and because of that it allows to skip intermediate types that don't declare a method with that signature, right?

If I understood that correctly, it LGTM. Some test expectations need fixing, though:

FAILED: ql/java/ql/test/library-tests/overrides/ConstructedOverrides2.ql
FAILED: ql/java/ql/test/query-tests/dead-code/DeadMethod/DeadMethod.qlref

@aschackmull
Copy link
Copy Markdown
Contributor Author

aschackmull commented Nov 15, 2021 •
edited
Loading

and because of that it allows to skip intermediate types that don't declare a method with that signature, right?

It already allowed that. The following equation holds new_overrides = old_overrides+ new_overrides+ = old_overrides. I.e. all the places that mention Method.overrides* or Method.overrides+ should see the exact same tuples.

@atorralba
Copy link
Copy Markdown
Contributor

atorralba commented Nov 15, 2021

Yes, I was just trying to write down what I understood about the block after the or in the new overridesCandidateType predicate.

The following equation holds new_overrides = old_overrides+.

🤔 Isn't it new_overrides+ = old_overrides? As I understand it, overrides without the transitive closure should return less results now, right?

@aschackmull
Copy link
Copy Markdown
Contributor Author

aschackmull commented Nov 15, 2021

Isn't it new_overrides+ = old_overrides?

Yes, you're right. I wrote that a bit too quickly.

atorralba
atorralba previously approved these changes Nov 15, 2021
View reviewed changes
@aschackmull
Copy link
Copy Markdown
Contributor Author

aschackmull commented Nov 16, 2021

The queries java/missing-override-annotation and java/non-overriding-package-private produce fewer results. The removed results were essentially duplicates, so this is fine.

@aschackmull
Copy link
Copy Markdown
Contributor Author

aschackmull commented Nov 17, 2021

Timings look good now.

@aschackmull aschackmull merged commit 22ebe68 into github:main Nov 17, 2021
@aschackmull aschackmull deleted the java/overrides branch November 17, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@atorralba atorralba atorralba approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aschackmull @atorralba