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(ImportCleaner): resolve imports of parent types #4353

Merged
merged 6 commits into from
Dec 15, 2021

Conversation

algomaster99
Copy link
Contributor

Reference: ASSERT-KTH/sorald#598

Type B in A.B x = new A.B() was not being resolved to import somePackage.A even though this import statement was present in the compilation unit.

I apologise for the bad test case. I wasn't able to reproduce the bug in a smaller test file. The reason was that whenever it used to visit B in A.B x, the visit was ended due to this condition. However, this line didn't interfere with the original test resource which brought about this bug.

@algomaster99
Copy link
Contributor Author

@slarse @MartinWitt @I-Al-Istannen can you please look at it?

@I-Al-Istannen
Copy link
Collaborator

I-Al-Istannen commented Dec 15, 2021

I think this breaks the following class (import is removed and Entry fully qualified):

import java.util.Map.Entry;

class Hello {
	public void hey() {
		Entry<String, String> f = null;
	}
}

@algomaster99
Copy link
Contributor Author

Thanks for pointing it out. I am looking into it.

@algomaster99
Copy link
Contributor Author

I think this breaks the following class (import is removed and Entry fully qualified):

You were right about it. I also added a condition which would check if the subtype is present. However, the reference to subtype here is java.util.Map.Entry<String, String> while the reference to import is java.util.Map.Entry (without type arguments). Thus, I skipped checking for TYPE_ARGUMENT while I compute their equality. Maybe we can put that method in EqualVisitor which allows you to skip for certain roles while getting the equality. What do you think?

@monperrus monperrus merged commit dbb1841 into INRIA:master Dec 15, 2021
@algomaster99 algomaster99 deleted the resolve-parent-imports branch December 15, 2021 14:33
@monperrus
Copy link
Collaborator

thanks @algomaster99

@slarse
Copy link
Collaborator

slarse commented Dec 15, 2021

Maybe we can put that method in EqualVisitor which allows you to skip for certain roles while getting the equality. What do you think?

@algomaster99 I don't think that's a good idea. What you're suggesting is some form of partial comparison, i.e. not equality.

@algomaster99
Copy link
Contributor Author

I thought it would be beneficial in comparing references. For example, in this case, I needed to compare two references - reference to an import and a field's type. I just wanted to skip comparison for TYPE_ARGUMENT role because type argument cannot be there in import references.

@slarse
Copy link
Collaborator

slarse commented Dec 17, 2021

I get the use case, but that's not equality, it's a different kind of comparison. So I don't think it's appropriate for the EqualVisitor.

Put simply, a reference with type arguments is not equal to a reference without type arguments, even if the referenced type is the same.

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.

4 participants