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

Incorrect unused import warning #1809

Closed
wants to merge 1 commit into from
Closed

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Jan 3, 2024

Fixes #1808
The issue happens for imports used in the permits clause when that import is

  • declared as a nested class
  • is used only in a permits clause The fix checks if member types are imported and marks the import as used.

Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jukzi
Copy link
Contributor

jukzi commented Feb 14, 2024

@srikanth-sankaran i understand your comment #1808 (comment) that the added test should fail.

@snjeza snjeza changed the title Incorrect unused import warning [WIP] Incorrect unused import warning Feb 15, 2024
Fixes eclipse-jdt#1808
The issue happens for imports used in the permits clause when that import is
- declared as a nested class
- is used only in a permits clause
The fix checks if member types are imported and marks the import as used.

Signed-off-by: Snjezana Peco <[email protected]>
@snjeza snjeza changed the title [WIP] Incorrect unused import warning Incorrect unused import warning Feb 15, 2024
@snjeza
Copy link
Contributor Author

snjeza commented Feb 15, 2024

@srikanth-sankaran
Copy link
Contributor

I'll take a look at this post 4.31 - this is too late for 4.31 anyway and I am also hard pressed for time right now. OK?

@srikanth-sankaran
Copy link
Contributor

Hello! This fix may work but does not look correct to me.

What I mean by that is: I am first of all surprised that there is a new method org.eclipse.jdt.internal.compiler.lookup.Scope.checkUsed(char[], Scope, ReferenceBinding) which walks through the imports and sets the Used bit and returns a binding.

I would expect existing code that handles import declarations and "serves" types should handle this - if it doesn't then the question is why not -

I believe it is very likely permits clause is processed either in the wrong phase/time or in the wrong environment - In the "right solution", things should automatically "just work" - A sketch of the right solution would not call findMemberType AT ALL above the checkUsed call - as it would be the case if insideTypeAnnotation is true.

When permits clause is resolved we are outside the class declaration in the class header - the member types are not in scope - so we should not be calling findMemberType - AT ALL.

@srikanth-sankaran
Copy link
Contributor

Hello! This fix may work but does not look correct to me.

What I mean by that is: I am first of all surprised that there is a new method org.eclipse.jdt.internal.compiler.lookup.Scope.checkUsed(char[], Scope, ReferenceBinding) which walks through the imports and sets the Used bit and returns a binding.

I would expect existing code that handles import declarations and "serves" types should handle this - if it doesn't then the question is why not -

I believe it is very likely permits clause is processed either in the wrong phase/time or in the wrong environment - In the "right solution", things should automatically "just work" - A sketch of the right solution would not call findMemberType AT ALL above the checkUsed call - as it would be the case if insideTypeAnnotation is true.

When permits clause is resolved we are outside the class declaration in the class header - the member types are not in scope - so we should not be calling findMemberType - AT ALL.

The type should be "served" by the import and so the import should be seen to used - what the present PR is doing is the opposite of it - the type is served by scope (member type) and then we set the used bit on the import. The import should be first of all seen to be necessary to resolve/materialize the type.

Not have a type and then set the import's used bit.

Am I being clear ? :)

Please let me know if this is urgently required - if not if you can wait till M3, I can work on it then, If it is super urgent I can try and squeeze in some time sooner.

@srikanth-sankaran
Copy link
Contributor

Also the changes in org.eclipse.jdt.internal.compiler.lookup.ClassScope.findPermittedtype(TypeReference) don't look right to me. An error should be issued for

public sealed interface X permits B { 
    record B(int data)  {}
}

because we fail to find B and not because we find B and then check the imports and not find it there!

Finally, see that this code:

interface Y {
    final class B implements X {}


    public sealed interface X permits B { 

    }
}

compiles on master and on javac but fails to compile with the patch because the new code in org.eclipse.jdt.internal.compiler.lookup.ClassScope.findPermittedtype(TypeReference) rejects it.

@srikanth-sankaran
Copy link
Contributor

I suggest you debug this program:

import java.util.List;
import java.util.ArrayList;

public class X {
	List<X> lx = null;
}

to see what I mean by a type is served by an import. You see a warning on ArrayList import and not List import. Studying why would tell you how types are served by imports and how imports are seen to be used or not used.

@srikanth-sankaran
Copy link
Contributor

I have pushed a different fix. This PR may be closed without further action

@srikanth-sankaran
Copy link
Contributor

This PR is superseded by #2608

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.

[sealed-classes] Incorrect unused import warning
3 participants