-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for indexing of impacts of changes on fields on downstream dependencies #221
Conversation
…rt-field-downstream
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.
Overall looks really good! Just a couple of comment
// Collect public methods with non-primitive return types. | ||
// Used to collect callers of each method. |
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.
Should this comment be before line 90?
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.
Thanks for catching this. Fixed it in 15c8e1e
@@ -96,6 +96,16 @@ public void addFieldDeclaration(FieldDeclaration fieldDeclaration) { | |||
this.fields.add(new FieldDeclarationRecord(fieldDeclaration)); | |||
} | |||
|
|||
/** | |||
* Checks if the class contains a field with the given name. |
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.
This method is doing something different than the docs. It seems to look for a single field declaration that declares all the names in the Set
given as a parameter. We should update the docs and consider renaming the method.
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.
@msridhar Yes, I agree. Regarding field declarations (or OnFIeld
instances) we have to document somewhere how they are created. Throughout the entire core
module, every OnField
instance contains all inline field names for the declaration, for instance for a declaration Foo a, b, c
, all OnField
instances targeting this declaration, contain all a
, b
and c
on their variables
property. This has been done at deserializing fixes here. That is the reason why I used equals
instead of checking if it contains it. Will change the name and the documentation now.
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.
Updated javadoc for this method 71b901d.
Will work on a more detailed documentation with revisiting all methods working on fields in a followup PR.
try { | ||
Files.deleteIfExists(path); | ||
Files.createDirectories(path.getParent()); | ||
Files.createFile(path); |
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 think this is modifying logic introduced in #220. Still not sure why we are creating an empty file here.
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.
Yes, this is fixed in #220 now.
…irtualInjector.java Co-authored-by: Manu Sridharan <[email protected]>
…irtualInjector.java Co-authored-by: Manu Sridharan <[email protected]>
…rt-field-downstream
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.
LGTM! Just one very minor comment, can land after that
annotator-core/src/main/java/edu/ucr/cs/riple/core/registries/field/ClassFieldRecord.java
Outdated
Show resolved
Hide resolved
…field/ClassFieldRecord.java Co-authored-by: Manu Sridharan <[email protected]>
This PR adds support for indexing impact of changes on fields on downstream dependencies. Prior to this PR, Annotator might make a field
@Nullable
that can trigger errors on downstream dependencies, with this PR, Annotator is aware of these impacts and if changes on fields, create an unresolvable error on downstream dependencies, the containing fix tree is rejected.See example below:
On Target
On Dependency (Dep)
Prior to this PR, Annotator would have made
f0
@Nullable
even thought it causes an unresolvable error inDep
module. With this PR,f0
@Nullable
f1
@Nullable
is resolvable by makingf2
@Nullable
, therefore it includes the corresponding fix (f2
) in any tree containingf1
.