eclipse-jdtls / eclipse-jdt-core-incubator

Eclipse Public License 2.0
8 stars 1 forks source link

JavacCompilationUnit.resolve() should use a `new CancelableNameEnvironment(project,...)` #494

Open mickaelistria opened 3 months ago

mickaelistria commented 3 months ago

JavacCompilationUnitResolver.resolve() is failing in some cases, eg ASTConverterBugsTests.testBug212100a(). The reason is that the current implementation uses a NameEnvironment built from an empty classpath. If we compare to ECJ CompilationUnitResolver, the environment is built with

            if (javaProject == null) {
                Classpath[] allEntries = new Classpath[classpaths.size()];
                classpaths.toArray(allEntries);
                environment = new NameEnvironmentWithProgress(allEntries, null, monitor);
            } else {
                environment = new CancelableNameEnvironment((JavaProject) javaProject, owner, monitor);
            }

JavacCompilationUnitResolver should mimic it.

datho7561 commented 3 months ago

In order to get that test working properly, we need to be able to append to the list of binding keys. The tests do this by accessing the instance of the compilation unit in the ASTRequestor and calling a method that will request additional methods.

This field is package private, and from my understanding only ever accessed by the CompilationUnitResolver and within ASTResolver.

IMHO we should narrow this type; i.e. change the field from a CompilationUnitResolver into a new interface that contains just the IBinding createBinding(String key); method, then have CompilationUnitResolver implement that method. In addition to helping with the implementation, this will prevent ASTRequestors from playing with the state of the Compiler. This will involve more upstream changes. Are you okay with this @mickaelistria @robstryker ?

mickaelistria commented 3 months ago

OK, this should probably be part of the interface. Please submit a PR with a suggestion. Also, can you please submit a PR that fixes the environment stuff in parallel? I think those can be treated as 2 distinct changes, can't they?

datho7561 commented 3 months ago

I have an API mostly extracted, but I'm still playing around with the code locally, because the functionality to resolve additional bindings still doesn't work.