Community
Participate
Working Groups
Currently it's too much difficult to provide additional inference engine.
Would like to do this, but API changes are not allowed anymore for 3.0.* releases. WebTools PMC has denied previous request to make similar changes. Will have to target for 3.1 (next year's release)
Here are some fields that I've run into when subclassing InferEngine, that should have protected access scope: compUnit currentContext
If access protection for anything needs to be changed, the reasons for doing so need to be explained. Michael, please start with specific members. Marcus, please explain why those fields need to be visible, i.e. what you're trying to do that can't be accomplished with the current visibility.
(In reply to comment #3) > please explain why those fields need to be visible, I just discovered the findDefinedType method which removes the need for me to access compUnit. As for currentContext I got by without it in the end, so I retract my earlier comment. I'll fill in more information if I bump into any other issue with access.
Thanks, Marcus.
We use the following methods/fields: 1. org.eclipse.wst.jsdt.core.infer.InferEngine.populateType(InferredType, ObjectLiteral, boolean) 2. org.eclipse.wst.jsdt.core.infer.InferEngine.currentContext 3. org.eclipse.wst.jsdt.core.infer.InferEngine.Context.currentType But, I think we should think about possible implementors of this class; may be creating an interface for this class (and for Context) is better even. (In reply to comment #3) > If access protection for anything needs to be changed, the reasons for doing so > need to be explained. Michael, please start with specific members.
(In reply to comment #6) > We use the following methods/fields: > > 1. org.eclipse.wst.jsdt.core.infer.InferEngine.populateType(InferredType, > ObjectLiteral, boolean) > 2. org.eclipse.wst.jsdt.core.infer.InferEngine.currentContext > 3. org.eclipse.wst.jsdt.core.infer.InferEngine.Context.currentType > > But, I think we should think about possible implementors of this class; may be > creating an interface for this class (and for Context) is better even. You're right, although I think Context could stay as a concrete class while InferEngine becomes an interface. For now, though, InferEngine is at least a public class and M6 should support multiple InferrenceProviders properly. Submitting to PMC review for M7 to create an IInferEngine interface, deprecate the current class in favor of a "DefaultInferEngine" replacement, and turn the internal static class, "Context", into a public top-level class of its own.
Can you explain a bit more what this new API is for? What is trying to be accomplished? Is it something like different library formats? Some new functionality?
It can be used for different libraries and their documentation formats to determine additional object types for use later during resolution. Largely it's a refactoring since the current InferrenceProvider interface returns the concrete InferEngine class. Interaction with that class is done as an ASTVisitor with a few initializers, so it's possible that we should create an abstract class above it instead.
Thanks Nitin. This sounds like some very important function and some very good changes. But, it does sound like "new" function, and post M6 is not the time to get started on new function -- finish it up maybe, but not get started. The odds are great it would take 2 or 3 milestones to "get right". I think your intentions are good in trying to help out others in the community, but how much would we use this API in WTP itself this release? ... so that we'd know if it was right and met several use-cases? You even say "so it's possible that we should create an abstract class above it instead." and similar remarks in other comments make it sound like this API is still being designed. Again, post M6 is not the time to start designing an API for the release. Post M6 exceptions are more for finishing touches on a new API that has been there a milestone or two, and a just discovered changes found to be required to finish it up, or handle corner cases. If you wanted to get started on it now, I think that's great, but it should be in a side branch, where it can iterate a few times before being finalized in the next release, presumably the 3.2 release we are considering for Dec. Thanks for bringing it forward for consideration. It does sound important, just too risky in knowing you had the API right in such a short time.
Categorizing JSDT bugzillas for planning purposes.
hi, can we get back to this topic? For start it would be good to separate InferEngine from ASTVisitor, as not every inference engine has to be ASTVisitor. From current JSDT code, my understanding is that inference engine works following way: 1. InferEngine.initialize(); is called from Parser -> InferEngine uses this for some internal initialization 2. InferEngine.setCompilationUnit(CompilationUnitDeclaration parsedUnit); is called from Parser -> InferEngine uses this to attach itself to compilation unit 3. InferEngine.doInfer(); is called from Parser -> InferEngine does it's main work here 4. InferEngine adds types to CompilationUnitDeclaration passed ealier by changing following public fields: - compUnit.numberInferredTypes - number of inferred types - compUnit.inferredTypes - array with inferred types - compUnit.inferredTypesHash - map with inferred types 5. InferEngine also sometimes uses following method to query for already found types: compUnit.findInferredType(key) - alias for compUnit.inferredTypesHash.get(key) More than one InferEngine can be executed on compUnit, so engines should be careful about it. There are also some fields (appliesTo and inferenceProvider) in InferEngine, that can be changed by InferenceManager. And there are two static fields accessed by others. is this all that one should know when implementing his own inference engine?
Created attachment 154494 [details] patch first take on extracting InferEngine to interface. Based on previous comment. Patch was created by searching for all references to InterEngine and replacing them with IInferEngine. I intentionally left some references to InferEngine in tests plug-in, because that tests probably test only this one particular InferEngine class. Comments are welcome. (patch is based on HEAD)
I like it, but why the IInferengine#setAppliesTo(boolean)? org.eclipse.wst.jsdt.core.infer.InferrenceProvider#applysTo(IInferenceFile) is responsible for telling InferenceManager that a particular engine should be run; I'm not sure why you'd need to tell the engine anything.
I added IInferEngine.setAppliesTo(int), to correctly refactor two assignments to inferEngine.appliesTo field in InferenceManager in lines 61 and 71. But indeed, the InferEngine.appliesTo field is never read, so it can probably be removed together with IInferEngine.setAppliesTo
Created attachment 154546 [details] patch v2 updated patch with removed InferEngine.appliesTo, IInferEngine.setAppliesTo(int) and related assignments in InferenceManager
Nitin, assuming there's no other concerns, would it be possible to commit last patch? A possible next step could be to separate out ASTVisitor from InferEngine. This would make InferEngine more a utility class that helps manage context (push/pop) and types (findType, populateType, etc.). The real action would still stay in ASTVisitor but it'd be easier to switch between different visitors.
Looks good, but IInferEngine#setInferenceProvider(InferrenceProvider) isn't actually needed, it's more of an implementation detail that's unrelated to how that interface is meant to used. Since the engines are always and only created by their inferrence provider, the provider can take care of that using whatever contract it wants. If nothing else I want to actively discourage anyone *else* from being able to tamper with that value.
Created attachment 155488 [details] patch v3 Updated the patch to remove setInferenceProvider() as pointed out by Nitin in comment 18. Indeed DefaultInferenceProvider takes care of setting inferenceProvider field in InferEngine (default impl). So InferenceManager or anyone else should not re-set this. I also added a little javadoc to IInferEngine.
Thank you for your work on this patch Jacek and for your quick response to our feedback. v3 patch has been checked into 3.2m5.
