-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 Member
s?
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
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). |
Changes:
MemberType
to the results; resolves Java:RefType.inherits(Member)
does not work for member types #5596Remaining:
m
declared in a superclass and methods with the same name and subsignature asm
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, seeAbstractInheritingClashing
test class added by this pull requestThese 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.