Skip to content
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

fix: prevent removal of static field import #4331

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

algomaster99
Copy link
Contributor

The failing test case reproduces ASSERT-KTH/sorald#602.

@algomaster99 algomaster99 force-pushed the static-field-import branch 2 times, most recently from 51845a3 to b6f593b Compare December 6, 2021 16:33
@algomaster99
Copy link
Contributor Author

The handleTargetedExpression ignored instances of CtFieldRead. This caused the removal of import statements which imported static fields. I have added a method isReferenceToFieldPresentInImports which checks if the field reference is present in imports, and if it does, it is added to computedImports.

@algomaster99 algomaster99 marked this pull request as ready for review December 6, 2021 17:05
@algomaster99
Copy link
Contributor Author

@slarse @MartinWitt @monperrus could you please look at this?

@algomaster99
Copy link
Contributor Author

algomaster99 commented Dec 6, 2021

At another glance, I think the fix should belong here. I am unsure about it though.


There are two ways to fix this problem.

  1. Ensure import static fieldImport.UtilityLibrary.FirstUtility.LIMIT; is added to computedImports so that it is not removed later in onCompilationUnitProcessed.
  2. Add a condition in onCompilationUnitProcessed which will not remove the import statement from exisitngImports even if it does not exist it computedImports.

This PR implements the 1. I don't know about the contract of the computedImports so I can't really pin-point which is the correct method to go about.


  1. Add a condition in onCompilationUnitProcessed which will not remove the import statement from exisitngImports even if it does not exist it computedImports.

Second approach doesn't seem correct to me. The contract of onCompilationUnitProcessed seems like that it removes imports from existingImports if it cannot find it in computedImports. Therefore, a better way to fix this bug to go with 1 and this PR does exactly that.

@monperrus
Copy link
Collaborator

Cool! Do we need the three changes at line 90, 179 and 196 to fix the single failing test case?

@algomaster99
Copy link
Contributor Author

Yes. Let me explain it case by case.

Line 90

This allows the instance of FieldRead to be passed to addImport.

Line 179

Since we are not in addImport (courtesy of line 90), we need to do something about it. This conditions checks if the ref is present in any of the import statements. If it is not present, we return so that the import is not added to the final output.

Line 196

This bit is quite tricky, but I have two reasons to support this diff.

  1. When the import, import static fieldImport.UtilityLibrary.FirstUtility.LIMIT;, is present, we move forwards to check other conditions if it needs to be added in computedImports. The previous condition, without my diff, passed because package of the compilation unit and the field reference were same. We need to pass this condition when the references to types are considered.
  2. The comment below the line changed further supports the diff because we only need to check for TypeReferences in same packages and not field references.

@monperrus monperrus merged commit 4f44d9f into INRIA:master Dec 7, 2021
@monperrus
Copy link
Collaborator

Thanks for the explanation, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants