Bug 150657 - [DOM] AST API reusable type binding environment (open up compiler loop)
Summary: [DOM] AST API reusable type binding environment (open up compiler loop)
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Alex Boyko CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
: 40096 148224 (view as bug list)
Depends on:
Blocks: 340145 337514
  Show dependency tree
 
Reported: 2006-07-14 11:09 EDT by Martin Aeschlimann CLA
Modified: 2024-02-01 03:17 EST (History)
11 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Aeschlimann CLA 2006-07-14 11:09:29 EDT
3.2

As discussed on the JDT summit:
We need a more flexible way to create multiple ASTs besides the AST queue:
 - We don't know all the required ASTs before starting the queue
 - How many ASTs can we build before we run out of memory

The idea is that jdt.core can provide a new API 'BindingEnvironment' that can used to create new AST's and be asked to resolve more bindings.
Internally, the environment will recreate the bindings in case the number of resolved binings hits a memory bound or if an AST is requested that already has been resolved.
When doing that, DOM Binding stay identical. Idea was that using the binding key (or similar) they can rebind the reference to the compiler binding. Or instead of keeping a reference to a compiler binding, only know the binding key and access compiler bindings with a lookup using the key.
Comment 1 Martin Aeschlimann CLA 2006-07-14 12:25:53 EDT
API suggestion:

+ org.eclipse.jdt.core.dom.BindingEnvironment
   boolean containsBinding(String key)
   IBinding getBinding(String key)
   ITypeBinding getArrayBinding(ITypeBinding baseType, int dim)
   ITypeBinding getWildcardBinding(ITypeBinding bound, int isUpperBound)
   ITypeBinding getParameterizedTypeBinding(ITypeBinding[] typeArguments)
   ITypeBinding getWellKnownTypeBinding(String name)

org.eclipse.jdt.core.dom.AST:
  + public BindingEnvironment getBindingEnvironment()

org.eclipse.jdt.core.dom.IBinding
  + public BindingEnvironment getBindingEnvironment()

org.eclipse.jdt.core.dom.ASTParser
  + void setBindingEnvironment(BindingEnvironment environment)
// creates the new AST(s) with the given environments
Comment 2 Philipe Mulet CLA 2006-07-19 11:41:50 EDT
Pipeline should be able to shutdown (explicitly or when finished) and be restarted. When shutting down, all bindings would be flushed, but they would behave as handles, and still be able to be repopulated (preserving identity on DOM AST side) using new compiler bindings under the cover (when the pipeline has been restarted).
Comment 3 Markus Keller CLA 2006-09-11 09:51:43 EDT
Update to BindingEnvironment API from comment 2:

- getParameterizedTypeBinding(..) should be:
    ITypeBinding getParameterizedTypeBinding(
            ITypeBinding genericType, ITypeBinding[] typeArguments

- additional API:
    ITypeBinding getRawTypeBinding(ITypeBinding genericType)
   
- getWellKnownTypeBinding(String name) would better be replaced by
    ITypeBinding getTypeBinding(String name)
  (see bug 156871)
Comment 4 Olivier Thomann CLA 2007-01-29 14:46:05 EST
This binding environment will add an new indirection between the dom bindings and the compiler bindings. This will slow down the accesses to any binding information since I cannot rely anymore on getting the compiler binding inside a dom binding.
Comment 5 Olivier Thomann CLA 2007-01-30 11:56:40 EST
*** Bug 148224 has been marked as a duplicate of this bug. ***
Comment 6 Olivier Thomann CLA 2007-02-01 11:35:12 EST
I would split this request in 3 sub requests:
1) life cycle of a binding environment. Right now the user doesn't control the life cycle since the ASTParser is cleaning up scopes once the tree is returned. This blocks further binding resolution.
2) Adding factory method to create new type bindings
3) Keeping the DOM binding identity.

For 1), this could be fixed without a binding environment. See bug 164660. We could add a new API to prevent cleaning of queued units.
2) could be done right now if we don't shutdown the lookup environment once the tree is returned.
3) requires more work in the dom binding creation.

Any thought?
Comment 7 Olivier Thomann CLA 2007-02-01 11:35:47 EST
Moving to 3.3M6 since Id' like to get Martin's opinion on this.
Comment 8 Olivier Thomann CLA 2007-02-01 11:48:11 EST
*** Bug 40096 has been marked as a duplicate of this bug. ***
Comment 9 Markus Keller CLA 2007-02-01 12:48:20 EST
I agree that we should wait until M6 now.

As I understood, we should not remove API after M5, so you should maybe attach a warning to the method ITypeBinding#createArrayType(..) from bug 148224, telling that this API is experimental and could be removed for 3.3.
Comment 10 Olivier Thomann CLA 2007-02-01 12:53:16 EST
Done.
Comment 11 Dani Megert CLA 2007-02-02 07:14:45 EST
>As I understood, we should not remove API after M5
Neither remove nor change. M5 is the freeze for existing API introduced during 3.3. You will have to deprecated such APIs if you change them after M5. If I understood it right there is one exception to that rule: if you can get PMC approval you can change it.

New additions can still be made.
Comment 12 Olivier Thomann CLA 2007-02-02 08:15:57 EST
(In reply to comment #11)
> >As I understood, we should not remove API after M5
> Neither remove nor change. M5 is the freeze for existing API introduced during
> 3.3. You will have to deprecated such APIs if you change them after M5. If I
> understood it right there is one exception to that rule: if you can get PMC
> approval you can change it.
Then I should remove it completely. Are you using it?

Comment 13 Martin Aeschlimann CLA 2007-02-22 07:05:19 EST
Yes, the suggested API's in comment 1 and comment 3 bring together two separate requests that can be solved seperatly, but where, I believe, the BindingEnvironment API is the right place for it.
- reusing binding environment (AKA: creating multiple AST with the binding identity, simpler story than the AST queue)
  -> solves lots of workarounds and performance issues in our code
- finding and creating bindings (bug 148224, bug 156871, bug 40096)
  -> required to implement functionality releated to bindings

As you say in comment 6, the 'binding life cycle' problem could also be added to the BindingEnvironment, but I think we rather keep that seperate. It should be said that by default, the user should be able to get all bindings as long he has a reference of an AST or a binding. Shooting down the binding environment after creation and without knowing what information the user requires, is a bug that should be fixed again.

It would have been great to get these new powerful API early in the build cycle. Right now, I'm not sure if we will find the time to update our code to make fully use of it. So I'm fine with a partial solution, or moving the request to post 3.3. However, I hope we can agree on the 'vision', and hopefully add the new API early in the next release.
Comment 14 Olivier Thomann CLA 2007-02-23 10:57:09 EST
Postponing post 3.3.
Comment 15 Olivier Thomann CLA 2010-11-17 10:42:03 EST
Satyam, this might be in line with the current changes you are making in DOM bindings ?
Comment 16 Eclipse Genie CLA 2019-10-25 00:04:39 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 17 Alex Boyko CLA 2020-01-21 17:37:37 EST
What are the current JDT classes that BindingEnvironment is build from?

I thought it would be one of:
org.eclipse.jdt.core.dom.DefaultBindingResolver
org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment

Doesn't look like either of them... Not really sure what would be the closest implementation of this interface in the current JDT...
Any help?
Comment 18 Stephan Herrmann CLA 2020-01-21 17:47:24 EST
(In reply to Alex Boyko from bug 559042 comment #5)
> I'll start with implementing IBindingEnvironment outlined here
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=150657#c1
> Hope it is the right way to kick this off in the right direction

Thanks, Alex, for offering to work on this!

To connect some dots, I think you want to add some ASTParser methods also to BindingEnvironment, notably #setEnvironment() (and let ASTParser use that information from the BindingEnvironment, if set).

Then the BindingEnvironment can cache the List<Classpath> created from the above.

Finally, BindingEnvironment will need s.t. like a close() method, which will end a parsing session and clear all data and resources. After that, the binding environment can no longer be used, and an ASTParser configured with a closed BindingEnvironment should refuse to create more ASTs.

I can offer to review early draft implementations. I might also add support for keeping Scopes (compiler internal) as long as the BindingEnvironment is alive, which should fix various NPEs in this area.

Above discussion mentions re-opening a compiler session after close. IMHO this can be left for a future iteration (if needed).

Feel free to ask any questions when they occur.
Comment 19 Stephan Herrmann CLA 2020-01-21 17:55:34 EST
(In reply to Alex Boyko from comment #17)
> What are the current JDT classes that BindingEnvironment is build from?
> 
> I thought it would be one of:
> org.eclipse.jdt.core.dom.DefaultBindingResolver
> org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment
> 
> Doesn't look like either of them... Not really sure what would be the
> closest implementation of this interface in the current JDT...
> Any help?

So far this bug only lists new API, without speaking about how that API is implemented, but I envision that you can pull any data from ASTParser and CompilationUnitResolver to BindingEnvironment as you need, incl.:
- as per your initial request: List<Classpath>
- DefaultBindingResolver is certainly a good idea
- you probably want to manage the INameEnvironment rather than the compiler's
  LookupEnvironment

With that, you probably want to add new methods to CompilationUnitResolver that will accept the stuff managed in BindingEnvironment (or the entire BindingEnvironment instance :) ).
Comment 20 Alex Boyko CLA 2020-01-22 19:59:47 EST
I'm a bit confused about things that BindingEnvironment should have a containment reference to, i.e. things we'd like to share between CUs and ASTParsers with BindingEnvironment.

Very likely:
- INameEnvioronment
- DefaultBindingResolver.BindingTables (currently only shared between CUs parsed/resolved in batch)

Doesn't seem like i can share DefaultBindingResolver between CUs as it takes CU Scope which has a referenceContext field which is CompilationUnitDeclaration. The latter seems to specific for each particular CU and therefore cannot be shared.

Unlikely:
From DefaultBindingResolver:
- astNodesToBlockScope ?
- bindingsToAstNodes ?
- newAstToOldAst ? 
All of the above seem to deal with ASTs for which DefaultBindingResolver is different... Or shall I try to share these too?

Anything else I might be missing that you had in mind?


I'm about to go with just sharing INameEnvironment and BindingTables and add methods to create BindingResolver and possibly other data structures requiring INameEnvironment and BindingTables, i.e LookupEnvironment
Comment 21 Stephan Herrmann CLA 2020-01-23 09:13:05 EST
(In reply to Alex Boyko from comment #20)
> I'm a bit confused about things that BindingEnvironment should have a
> containment reference to, i.e. things we'd like to share between CUs and
> ASTParsers with BindingEnvironment.
>
> Very likely:
> - INameEnvioronment
> - DefaultBindingResolver.BindingTables (currently only shared between CUs
> parsed/resolved in batch)
>
> Doesn't seem like i can share DefaultBindingResolver between CUs as it takes
> CU Scope which has a referenceContext field which is
> CompilationUnitDeclaration. The latter seems to specific for each particular
> CU and therefore cannot be shared.

You raise some good points here. Let's have a quick look at multiplicities:

Each CompilationUnitResolver (which is-a Compiler) holds exactly one
- INameEnvironment
- LookupEnvironment
- BindingTables
The following things are created once per file:
- org.eclipse.jdt.core.dom.CompilationUnit
- org.eclipse.jdt.core.dom.AST
- org.eclipse.jdt.core.dom.DefaultBindingResolver
- org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration
- org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope
There are already many links between these instances, so perhaps it's good if the BindingEnvironment collects exactly one of these things (e.g., Set<CompilationUnit> processedUnits;), which would theoretically provide access to all the rest.
One purpose of remembering: this will be key for stopping to clear all Scopes while a session is still open, and discard them all during close().
 
> Unlikely:
> From DefaultBindingResolver:
> - astNodesToBlockScope ?
> - bindingsToAstNodes ?
> - newAstToOldAst ? 
> All of the above seem to deal with ASTs for which DefaultBindingResolver is
> different... Or shall I try to share these too?

Let's not keep individual parts of the DefaultBindingResolver. They won't be very useful without the binding resolver itself.

> I'm about to go with just sharing INameEnvironment and BindingTables and add
> methods to create BindingResolver and possibly other data structures
> requiring INameEnvironment and BindingTables, i.e LookupEnvironment

This sounds like a viable approach. Yet, looking at the above multiplicities, I wonder if the binding environment could actually just remember one instance of CompilationUnitResolver, which already captures all the other things.

Here's a thought experiment: could we possibly use CompilationUnitResolver as the actual BindingEnvironment? Currently, this class is purely internal, but we could let it implement IBindingEnvironment and thus make a selected part of it API. Currently, this class is instantiated exclusively in its own static resolve(..) methods, and never remembered. If we could remember the CompilationUnitResolver, all the parts mentioned above would be automatically remembered / sharable.

I don't immediately see if it would work to invoke any of the non-static resolve(..) methods more than once, but very likely the accept(..) methods can be invoked any number of times, provided we didn't clean up too much (notably scope references in 
org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration).

So, for an initial experiment, if ASTParser is pre-populated with a CUResolver aka IBindingEnvironment then it could check: during first invocation it can use the regular resolve(..) method which consumes many ICUs at once, and on subsequent invocations it would pass them individually to accept().


Would you like to make that thought experiment a little code experiment?
Comment 22 Alex Boyko CLA 2020-01-23 14:03:39 EST
I have explored the code and believe that keeping CompilationUnitResolver around is worth trying so I'll get started with it. Thanks very much!
Comment 23 Alex Boyko CLA 2020-01-27 14:41:15 EST
I have made CompilationUnitResolver to implement BindingEnvironment... I'd like to test the code before adding support for:
 1. Caching processed CUs to dispose scopes later when environment is disposed
 2. Cleaning up the environment between resolve calls (i.e. fields from Compiler)
(2. needs to be explored and tested still)

JDT has "model" tests which is over 200K tests and  "compiler" tests with a total of over 100K tests. Many of these fail even if i switch to master branch. The most typical exception is:
java.lang.IllegalArgumentException
	at org.eclipse.test.internal.performance.PerformanceMeterFactory.assertUniqueScenario(PerformanceMeterFactory.java:46)
	at org.eclipse.test.internal.performance.PerformanceMeterFactory.createPerformanceMeter(PerformanceMeterFactory.java:30)
	at 

I have tried StandAloneASTParserTest and some others that use ASTParser directly. These tests mainly pass so I have some confidence that the changes i made are working.
Any idea how make the rest of the tests pass in my dev? A link to the doc would be enough.

Also I'd be interested in getting the Maven build for JDT Core working.
I'm running this Maven command: "mvn -Pbuild-individual-bundles clean install"
It always fail for me with this:
[ERROR] Cannot resolve project dependencies:
[ERROR]   Software being installed: org.eclipse.jdt.compiler.tool 1.2.700.qualifier
[ERROR]   Missing requirement: org.eclipse.jdt.core 3.20.100.qualifier requires 'osgi.bundle; org.eclipse.core.resources [3.12.0,4.0.0)' but it could not be found
[ERROR]   Cannot satisfy dependency: org.eclipse.jdt.compiler.tool 1.2.700.qualifier depends on: osgi.bundle; org.eclipse.jdt.core [3.3.0,4.0.0)

(Same error for master branch)

Any idea how to get the maven build going?

Thanks in advance :-)
Comment 24 Stephan Herrmann CLA 2020-01-27 15:25:40 EST
(In reply to Alex Boyko from comment #23)

Congrats for a quick first success :)

> I have made CompilationUnitResolver to implement BindingEnvironment... I'd
> like to test the code before adding support for:
>  1. Caching processed CUs to dispose scopes later when environment is
> disposed
>  2. Cleaning up the environment between resolve calls (i.e. fields from
> Compiler)
> (2. needs to be explored and tested still)
> 
> JDT has "model" tests which is over 200K tests and  "compiler" tests with a
> total of over 100K tests. Many of these fail even if i switch to master
> branch. The most typical exception is:
> java.lang.IllegalArgumentException
> 	at
> org.eclipse.test.internal.performance.PerformanceMeterFactory.
> assertUniqueScenario(PerformanceMeterFactory.java:46)
> 	at
> org.eclipse.test.internal.performance.PerformanceMeterFactory.
> createPerformanceMeter(PerformanceMeterFactory.java:30)
> 	at 

I have never seen this exception, so I wonder how you launched the tests. A simple "Run As ... > JUnit Plug-in Test" should do. You can do this directly on the full suite org.eclipse.jdt.core.tests.RunJDTCoreTests, or on any direct or indirect child of this suite. To speed up test launches, open the run configuration and select as your application "[No Application] - Headless Mode".

If those hints don't solve the riddle, maybe you can add a word of how you set up your development environment. Did you use Oomph?


> Also I'd be interested in getting the Maven build for JDT Core working.
> I'm running this Maven command: "mvn -Pbuild-individual-bundles clean
> install"
> It always fail for me with this:
> [ERROR] Cannot resolve project dependencies:
> [ERROR]   Software being installed: org.eclipse.jdt.compiler.tool
> 1.2.700.qualifier
> [ERROR]   Missing requirement: org.eclipse.jdt.core 3.20.100.qualifier
> requires 'osgi.bundle; org.eclipse.core.resources [3.12.0,4.0.0)' but it
> could not be found
> [ERROR]   Cannot satisfy dependency: org.eclipse.jdt.compiler.tool
> 1.2.700.qualifier depends on: osgi.bundle; org.eclipse.jdt.core [3.3.0,4.0.0)
> 
> (Same error for master branch)

I can't recall, when was the last time I invoked a maven build for JDT :) Frankly, I don't see it adding any value during development. We typically let gerrit/jenkins run maven once we're confident with a change, so feel free to push your changes to gerrit just for testing (LMK if you need pointers regarding gerrit). This also brings the advantage that I could glance over your changes as you go. (If you insist in running maven locally I could dig out the parameters used on jenkins, but I think this road is a waste of effort).
Comment 25 Eclipse Genie CLA 2020-01-27 20:03:41 EST
New Gerrit change created: https://git.eclipse.org/r/156684
Comment 26 Alex Boyko CLA 2020-01-27 20:13:37 EST
Here is the initial version of the solution lacking the 2 points I outlined in comment 23.

I ran the RunJDTCoreTests - there are 72k tests. There is only one failing and I'm sure it has nothing to do with the change (testTestDefaultCompliance - fails with both master and this branch)

I'd need the maven build to test the sharing of BindingEnvironment with real world use case - the spring boot language server. The Spring Boot LS consumes JDT Core as a maven dependency...
I have tried having a dependency on the local workspace maven project and seems to all compile in the workspace. However, when i launch the Spring Boot LS it throws NoSuchMethodFound errors on methods i put in to create BindingEnvironment... This is my current task. Try it with the Spring Boot LS :-)
Of course there should be JUnits for the BindingEnvironment, namely for re-using it for resolving various CUs - I plan to write a few tests.

Please let me know if what you see in the gerrit change aligns with what you had in mind.
Comment 27 Alex Boyko CLA 2020-01-27 20:15:11 EST
Once again, thank you very much for guiding and helping me with this feature!!!
Comment 28 Stephan Herrmann CLA 2020-01-28 14:07:58 EST
(In reply to Alex Boyko from comment #26)
> I ran the RunJDTCoreTests - there are 72k tests. There is only one failing
> and I'm sure it has nothing to do with the change (testTestDefaultCompliance
> - fails with both master and this branch)

This is expected, it reminds us that tests should be invoked with the following vm arg:

  -Djdt.default.test.compliance=1.8

> I'd need the maven build to test the sharing of BindingEnvironment with real
> world use case - the spring boot language server. The Spring Boot LS
> consumes JDT Core as a maven dependency...

granted. Here's the set of relevant maven args used on jenkins:

-DskipTests=false -U clean verify -Pbuild-individual-bundles -Pbree-libs -Ptest-on-javase-13 -Papi-check -Peclipse-sign

The last two you don't need. "-Ptest-on-javase-13" requires you to have a valid toolchains definition for JavaSE-13. Not 100% sure, what happens when you omit this. It arranges for testing on a different JRE than what maven is running on and adjusts the set of compliance levels to test (-Dcompliance=...). There are variants for other recent java versions. Or say -DskipTests=true and hope the test-on profile is irrelevant :)


> Please let me know if what you see in the gerrit change aligns with what you
> had in mind.

At a first glance I found no obvious problems. Will have to pull the gerrit and play with it a little during the next days / week to get a clearer picture.

Once you start integrating with Spring Boot LS let me know which API exactly you will be using, so we can minimize the API surface to what is actually used.
Comment 29 Stephan Herrmann CLA 2020-01-28 14:40:32 EST
Seeing the gerrit build failed with API errors, I suggest you download Eclipse SDK 4.14 and set it as the baseline, so you can enable API tools in your workspace.
Comment 30 Alex Boyko CLA 2020-01-28 21:51:39 EST
Haven't fixed the API baseline yet.
Made a few small corrections and it seems to be working but...

We attempt to parse/resolve the same source more than once in Spring Boot LS. The cache on the Boot LS expires entries in 1 minute. Attempt to parse/resolve the same file again  with the same BindingEnvironment results in an error because LookupEnvironment sees a duplicate type.

This implies that we should probably cache parsed CUs on the CU Resolver (thus satisfying 1 from comment 23) and then attempt to parse/resolve the same source will return cached CU. Does that sound ok?

Now with the Spring Boot LS it is likely that the source file changes slightly and then needs to be parsed/resolved again... Cannot use the same BindingEnvironment that i shared per project now, can I? Don't think i can update this one type in the LookupEnvironment either... Think the best i can do now is reset the BindingEnvironment (i.e. reset the CU Resolver) which clears out various caches but leaves the NameEnvironment untouched (NOT reset) Anything else i can do here?

To be honest i'm tempted a bit adding some API i had in my solution for Bug 559042. The reason for sharing List<Classpath> in the case of Spring Boot LS was its slow creation on Windows where the bottleneck was calls to #isDirectory(). Since majority of classpath entries are for JARs on the classpath that are not changing sharing Classpath entries seemed like a great idea.
Seems that on the Spring Boot LS side every time a new type is created or renamed I'd need to dispose and re-create BindingEnvironment again to keep the up to date NameEnvironment. If I'm to do that it'd be nice if most of the Classpath entries (JARs entries) were initialized already and only entries corresponding to output folders were created and initialized.
Comment 31 Alex Boyko CLA 2020-02-04 13:51:32 EST
Doesn't look like LookupEnvironment is something that can be shared between different batches of CompilationUnitResolver resolve calls...
There is a JUnit in Spring Boot LS that does the following:
 - Create classpath entires from dependencies JARs + project output folder
 - Create BindingEnvironment (CompilationUnitResolver) using the classpath and then the ASTParser with that environment to parse source files.
 - Takes source file A that depends on type from source file B and parses and resolves it.
 - Binding for type from source file B is resolved while resolving types of members s of A. Hence type from B is a "known type" for a package binding in the lookup environment
 - Now source file B is parsed and resolved. Source file B LookupEnvironment#buildTypeBindings(CompilationUnitDeclaration unit, AccessRestriction accessRestriction) reports duplicate type during compilation now because there is a binding (BinaryBinding) in known types already. 

Should probably have this test in JDT Core... I'll try to adopt it to JDT Core and have it there.

Perhaps after each CompilationUnitResolver#resolve(...) call the LookEnvironment field should have a new instance of the LookupEnvironment (inside the #cleanResolver() method i've added)
How does this sound?
Comment 32 Stephan Herrmann CLA 2020-02-04 14:30:02 EST
(In reply to Alex Boyko from comment #31)
> Doesn't look like LookupEnvironment is something that can be shared between
> different batches of CompilationUnitResolver resolve calls...

there be dragons


> There is a JUnit in Spring Boot LS that does the following:
>  - Create classpath entires from dependencies JARs + project output folder
>  - Create BindingEnvironment (CompilationUnitResolver) using the classpath
> and then the ASTParser with that environment to parse source files.
>  - Takes source file A that depends on type from source file B and parses
> and resolves it.
>  - Binding for type from source file B is resolved while resolving types of
> members s of A. Hence type from B is a "known type" for a package binding in
> the lookup environment
>  - Now source file B is parsed and resolved. Source file B
> LookupEnvironment#buildTypeBindings(CompilationUnitDeclaration unit,
> AccessRestriction accessRestriction) reports duplicate type during
> compilation now because there is a binding (BinaryBinding) in known types
> already. 

I see. And the binary type is no good for creating AST :(
 
> Should probably have this test in JDT Core... I'll try to adopt it to JDT
> Core and have it there.

That'd be great so we can collaborate using that test.
 
> Perhaps after each CompilationUnitResolver#resolve(...) call the
> LookEnvironment field should have a new instance of the LookupEnvironment
> (inside the #cleanResolver() method i've added)
> How does this sound?

That sounds problematic. Without sharing the LookupEnvironment, wouldn't sharing the BindingResolver likely cause grief, too? See in particular DefaultBindingResolver.BindingTables.compilerBindingsToASTBindings


The proposal in comment 1 contains
> boolean containsBinding(String key)

so one part of the equation could be to refuse processing any type that is already contained in the BindingEnvironment.

The other part would be avoiding to pull in binary types for types that we want the AST of. In the STS case, would you be able to know, whether or not a requested type is a candidate for requesting AST? Either via callback, or we might check why the binary type is preferred by the compiler.
Comment 33 Alex Boyko CLA 2020-02-25 12:29:26 EST
FYI: I have switched to another task for some time but I now i'm back to this...
Comment 34 Alex Boyko CLA 2020-02-28 12:23:01 EST
I have replicated the STS unit test in JDT Core and will push it to gerrit soon. The latest changes would also have a flag in the BindingEnvironment which makes the test pass or fail. The flag if true lets CompilationUnitScope#buildTypeBindings(...) to resolve types for CUs for which there is already a duplicate in the lookup environment.
(This flag and changes proagating the flag to ompilationUnitScope#buildTypeBindings(...) are for demonstration purposes only)

Yes, not sharing the LookupEnvironment is not a great idea...

STS use case is roughly the following:
There is a Spring Boot project. User opens/closes various Java files to may invoke actions requiring AST and type resolving of the opened Java files, i.e. content asisst or hovers.
Therefore, at some random points in time we may need AST for various source files that can and likely to change over time.

Ideal solution for me that involves a BindingEnvironment would probably be an ability to have a composite BindingEnvironment consisting of 2 sub BindingEnvironments where one i can re-use which is based on JAR classpath entries (these unlikely to change) and one that could be quickly created on demand from project output folders and source files. Obviously the second cannot exist without the first hence not it is unlikely possible.

I don't think having BindingEnvironment helps me in my use case substantially... Every time source file changes the environment needs to be reset which wipes out everything except the NameEnvironment. If source files are added removed then the NameEnvironment needs to be reset as well which is really disposing the BindingEnvironment.

I feel I'm better off with sharing a chunk of NameEnvironment which is unlikely to change. Namely instances of Classpath for JAR entries. Exposing API for creating Classpath from String paths and feed to BindingEnvironment/ASTParser should be enough for me.
Comment 35 Alex Boyko CLA 2020-02-28 16:28:24 EST
Latest gerrit patch build fails due to some api-checks failures... not sure where those come from as i don't see any errors while pointing at 4.14 as the baseline... even after marking api-check problems as errors. Lets just forget about the api-check error since the latest patch is definitely not for merging.
Comment 36 Alex Boyko CLA 2020-03-19 11:28:35 EDT
I propose the following for the final gerrit patch for this issue:
1. Current implementation of the BindingEnvironment
2. Add boolean containsBinding(String key) to the BindingEnvironment and implement it
3. Adjust the unit test added with previous patch to test BindingEnvironment and containsBinding(String key) method
4. Keep API for creating Classpath entries used to create BindingEnvironment

(The API to create Classpath entries would be most useful for us on the Spring tooling side)

How does this sound to you Stephan?
Comment 37 Alex Boyko CLA 2020-05-14 16:27:49 EDT
Hi Stephan,

Could you please provide your feedback for my proposal from the previous comment? Very interested in your opinion and get to some kind of conclusion and continue the work here.

Thanks in advance!
Comment 38 Stephan Herrmann CLA 2023-10-01 17:26:02 EDT
Hi Alex, obviously I dropped the ball here, when I stopped actively working on JDT in 2020. Sorry for not talking to you!

Are you still interested in feedback and to complete the work done?

If so, we should move to where JDT is nowadays: https://github.com/eclipse-jdt/eclipse.jdt.core
Comment 39 Alex Boyko CLA 2023-10-03 21:25:54 EDT
Hi Stephan,

Sure, I'll try to find time to move this over to github and probably rebase this stuff on the latest JDT
Comment 40 Mickael Istria CLA 2024-02-01 03:17:30 EST
Related: https://github.com/eclipse-jdt/eclipse.jdt.core/issues/1915 (and a basic currently incorrect proposal at https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1942 )