Skip to content

Java: Improve RefType.inherits(Member) #8660

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Apr 4, 2022

Changes:

Remaining:

  • A class with abstract method m declared in a superclass and methods with the same name and subsignature as m in superinterfaces should inherit all those methods (JLS 17 §8.4.8) (if I understood it correctly); currently CodeQL only considers the abstract method to be inherited but ignores the interface methods, see AbstractInheritingClashing test class added by this pull request

These changes make the inherits predicate functionally more complete (based on the JLS), but I am not sure if this really adds any value and whether these changes are therefore worth it.
I have only executed the newly added tests, but have not checked what effects these changes have to other usage of this predicate.

If you want I can also instead only create a pull request for the missing field package access check.

@Marcono1234 Marcono1234 requested a review from a team as a code owner April 4, 2022 13:04
@github-actions github-actions bot added the Java label Apr 4, 2022
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Seems like a valid improvement that could surprise third-party code. @aschackmull do you think this is worth a deprecation dance that would also give us the opportunity to make the name reflect what it does: deprecate this predicate explaining that it only considers fields and methods, then add declaresOrInherits(Member m) that considers all Members?

or
not memberType.isPrivate() and
(memberType.isPackageProtected() implies memberType.getPackage() = this.getPackage()) and
// And member type is not hidden by another member type with the same name (regardless of visibility)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also https://docs.oracle.com/javase/tutorial/java/IandI/hidevariables.html which would enable you to treat types and fields the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems to only be about fields though, isn't it? Do you mean fields could hide member types and member types could hide fields? I am not sure if that is possible, at least the JLS does not seem to mention it.

Or do you mean I should try to combine the common checks here for Field and MemberType?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant fields hiding fields and types hiding types and forgot about the possibility of one hiding the other.

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Apr 10, 2022

deprecate this predicate explaining that it only considers fields and methods, then add declaresOrInherits(Member m) that considers all Members?

Just to clarify, the other option is also to just update the documentation, fix field inheritance and reject this pull request. I am not sure if the added functionality here adds any value; maybe no one ever needs to check whether member types are inherited.

Regarding the "Remaining" point in the description I am also not sure if it is worth fixing this rare corner case, since it might be closely connected with how CodeQL defines method overrides (have not investigated this extensively yet).

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.

Java: RefType.inherits(Member) does not work for member types
2 participants