eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
165 stars 130 forks source link

Annotations analysis breaks in a fragment #3278

Open basilevs opened 2 days ago

basilevs commented 2 days ago

Test steps

Create a class in an Eclipse plugin:

import java.io.Closeable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import org.eclipse.jdt.annotation.NotOwning;
import org.eclipse.jdt.annotation.Owning;

public class TestNotOwning implements Closeable {
    private final List<Closeable> toClose = new ArrayList<>();

    @NotOwning
    public <T extends Closeable> T register(@Owning
    T closeable) throws IOException {
        closeable.close();
        return closeable;
    }

    @Override
    public void close() throws IOException {
        for (Closeable closeable : toClose.reversed()) {
            closeable.close(); // Ignore error handling for this demonstration
        }
    }

}

Enable annotation analysis and ensure that:

try (TestNotOwning t = new TestNotOwning()) {
    var a = new Closeable() {} ; // produces warning
    var b = t.register(new Closeable() {} ); // produces no warning
}

Create a fragment of the same plugin. Enable annotation analysis. Try same sample:

try (TestNotOwning t = new TestNotOwning()) {
    var a = new Closeable() {} ; // produces warning
    var b = t.register(new Closeable() {} ); // produces warning
}

Expected result

no warnings for line var b = ... or a warning requesting a missing dependency

Workaround: add an explicit dependency on org.eclipse.jdt.annotation in the fragment.

For some reason annotation analysis ignores implicit dependency on org.eclipse.jdt.annotation from host plugin.

stephan-herrmann commented 2 days ago

I'll try to have a look during the next release cycle.

If there is a bug indeed, it will likely be an issue of PDE, because JDT knows nothing of bundles, fragments and all that. All that JDT works on is a readily resolved classpath.

basilevs commented 2 days ago

JDT annotations are absent from classpath (as they are not used in the fragment).

stephan-herrmann commented 2 days ago

JDT annotations are absent from classpath (as they are not used in the fragment).

We'll have to check: shouldn't PDE implicitly put them on the classpath? Do you see them when you expand "Plug-in Dependencies" in the Package Explorer?

basilevs commented 2 days ago

Attached is a reproducer for pure JDT. A "client" project has no direct dependency on annotations package, but still produces the potential resource leak warning. notowning_reproducer.zip

To reproduce, import the whole ZIP (three projects) into workspace, compile, check problem view.

Adding a dependency makes little sense (as annotations are not used in this project) but fixes the issue.

stephan-herrmann commented 2 days ago

Attached is a reproducer for pure JDT. A "client" project has no direct dependency on annotations package, but still produces the potential resource leak warning. notowning_reproducer.zip

To reproduce, import the whole ZIP (three projects) into workspace, compile, check problem view.

Adding a dependency makes little sense (as annotations are not used in this project) but fixes the issue.

I'm no longer sure which exact bug you are concerned about.

From the initial description I understood you had problems with a fragment that didn't see the dependency of its host.

The latest comment seems to point in a different direction, viz. that you expect compilation of a client to understand the semantics of @NotOwning without having access to that annotation.

Which issue(s) do you want to report? If it's two issues let's capture them in separate issues please.

basilevs commented 2 days ago

These are same issues - instead of using the annotations, the project in question is using an annotated method. I did not realize, that any kind of "access" to annotations is required if these annotations are not referenced anywhere in a project. It would be nice for compiler to output a warning if annotations analysis is enabled, but annotations are not available.