-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Java: Add data flow for record getters. #4123
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
Conversation
| recf.getDeclaringType() = r and | ||
| getter.getNumberOfParameters() = 0 and | ||
| getter.getName() = recf.getName() and | ||
| not exists(getter.getBody()) and |
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 suppose abstract records are not allowed, so checking for the absence of a body is fine, however, wouldn't it be better to use getter.fromSource() instead
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.
Records are implicitly final and therefore can't be abstract. It looks like checking fromSource() would work as well, but I didn't expect it based on the qldoc of fromSource() - it states:
* Holds if this element pertains to a source file.
*
* Elements pertaining to source files may include generated elements
* not visible in source code, such as implicit default constructors.
So I would actually expect fromSource() to hold for a generated getter when the corresponding record is fromSource(). I wonder why that isn't the case.
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.
Ok, sounds like it's best to keep using the empty body check.
| @@ -0,0 +1,28 @@ | |||
| public class A { | |||
| record Pair(Object x, Object y) { } | |||
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.
Could you also add some test with fields that have explicit "get" methods? I think one field with a sensible get method that just returns the field (has flow) and one silly get method that returns a constant (no flow)
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.
Done.
aibaars
left a comment
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.
Looks good to me. Could you add some more tests and perhaps use fromSource()?
This adds support for flow through Java 14 records.