Community
Participate
Working Groups
20060120 We the recent change of a new option to ICompilationUnit.reconcile (recovery option) we could use the opportunity to solve a different problem we have with reconcile. Reconcile allows to specify if an AST is wanted and what level it should get (and if it should contain recovery). Many users use reconcile and don't really care for an AST. The Java editor however listens for the delta to get the AST from it (new API addeed for 3.2). But we can only use JLS3 with recovery. For all other ASTs we can use the deltas but have the request an AST again, which is of course a performance problems. So what if the Java editor could specify its preferred options? The API would be changed to reconcile(WorkingCopyOwner owner, IProgressMonitor monitor) (no options) and the preferred opetions can be attached e.g. to the WorkingCopyOwner (WorkingCopyOwner.getReconcileASTLevel...) or on the IProblemRequestor that is passed on 'becomeWorkingCopy'.
I was thinking that when reconciling, the AST will have statement recovery enabled by default. Why wouldn't it be the case ?
Yes, for the editor we what to have it 'on' by default. But not only the editor is using 'reconcile'... The other issue is that we need to turn the switch to 'on' in a controlled way, making sure all our features can handle the recovered nodes.
>. The Java editor however listens for the delta to get the AST >from it (new API addeed for 3.2). This is not true. We only use the AST that we get back during reconcile.
Just to clarify my last comment: we tried to use the AST from the delta but went away from this for two reasons: - it is not guaranteed that - there is an AST i.e. it might be null because it was not requested in reconcile(...) - the AST has the AST level as needed by the editor - when using the AST requested by the editor's reconciler we can benefit from the life cycle that the reconciler offers i.e. as soon as you start typing we can let AST requestors wait until we get a valid AST through the reconciler
JDT/Summit feedback: ------------------- JDT Core will change the code so that the primary working copy owner can specify the default AST level (or none) and reconcile(...) without parameters will provide the AST with the default level and also send out the delta with that AST. Open questions: what happens when a client specifies the AST level?
attachment 52107 [details] for bug 124662 contains a suggested fix
it's attachment 52107 [details] for bug 125504
Created attachment 52317 [details] Proposed patch This patch comes from the split of patch provided in bug 125504 comment 8 + changes discussed yesterday on call... Jerome, Martin, Please let me know if it's OK for you. I'm currently looking at tests to add on JDT/Core to verify that this new API works well...
Created attachment 52335 [details] Proposed patch after Jerome's review I forgot to add ICompilationUnit parameter on WorkingCopyOwner #getASTLevel, #enableStatementsRecovery and #resolveBindings methods, this is done now... I've also create a new flag #F_OWNER_AST_AFFECTED on IJavaElementDelta to distinguish change of AST from compilation and working copy owner... I finally run all JDT/Core Java Model tests and fix 3 failures, 2 due to the fact to toString() method has changed on JavaElementDelta and 1 due to an error on ReconcileContext#getAST3() method... There are still 2 errors and we also have 170 deprecated warnings in org.eclipse.jdt.core.tests.model now! I have to fix all these problems and also add specific tests. I'll do it next week (I'll be out of office tomorrow and Monday...)
The new patch looks good from the API side, with two additions: - WorkingCopyOwner.getProblemRequestor should also pass a ICompilationUnit. Same reason as for the AST methods: Problem reporting might not necessary for each working copy. - deprecate IJavaElementDelta.getCompilationUnitAST: It will only be set if one of the deprecated reconcile methods is called.
I looked at the patch and see one problem: we - and other clients - currently have code that reconciles the CU in the UI thread just to update the structure i.e. it must be fast. The call was originally IWorkingCopy.reconcile() and is now: ICompilationUnit.reconcile( ICompilationUnit.NO_AST, false /* don't force problem detection */, null /* use primary owner */, null /* no progress monitor */); When I understand your proposal correctly this will no longer be possible without either living with deprecated code or pay performance penalty in the UI thread. Both seems a no go from my side.
That's exactly the situation that Martin wanted to avoid, ie. having someone calling reconcile without creating the AST. This would create an inconsistency between the state of the working copy and the last AST created. However, if you know what you're doing, and since you own the WorkingCopyOwner, you can still temporarily return NO_AST to WorkingCopyOwner#getASTLevel(ICompilationUnit) while you're calling reconcile(...).
We do not know - that's my point. Example: a client (not JDT UI) who writes an action has to call reconcile before it checks certain (structure) state of the CU because otherwise the answer might be outdated. JDT UI does exactly the same and hence this pattern is copied by many clients. If this reconcile() - which runs in the UI - now takes longer then lots of blocking will happen in the UI. Sorry but that's really not the direction we want to dive into. If we deprecate the lightweight reconcile then we're in trouble because we sort of force clients to use the expensive reconcile. However, I don't think the patch is so far away from the solution: it already handles the case where clients call a (future) deprecated method. What I suggest is to provide clients with a non-deprecated method that simply updates the structure. We must simply ensure that the next reconcile call that wants the (working copy) AST fires the delta even if CU is in sync.
Downgrading. Need more input from Dani and Martin.
We should at least solve the problem requestor issue which seems to be simple to solve with a good migration path.
In reply to comment #15) > We should at least solve the problem requestor issue which seems to be simple > to solve with a good migration path. > Dani, what do you mean by 'problem requestor issue'?
(In reply to comment #16) > In reply to comment #15) > > We should at least solve the problem requestor issue which seems to be simple > > to solve with a good migration path. > > > Dani, what do you mean by 'problem requestor issue'? > I guess this is the fact that WorkingCopyOwner#getProblemRequestor() method should have an ICompilationUnit parameter. Memories begin to come back to me :-)
Here's what I meant and discussed with Martin: - we add WorkingCopyOwner.createProblemRequestor(ICompilationUnit) - we add ICompilationUnit.becomeWorkingCopy(IProgressMonitor) which takes the problem requestor from the WorkingCopyOwner. This also reduces misunderstandings that can happen with the current API, see bug 81063.
Created attachment 57537 [details] New proposed patch This patch adds last required method and does not deprecate current existing ICompilationUnit.reconcile(...) methods. Just warn client that using these API methods may result with performance degradation in case of parameter values different from WokringCopyOwner ones. This patch passes all JDT/Core Model tests. Dani, please let me know your feedback about it, thanks
Frédéric, given that Martin was the driving force behind this and he's away until M5 is done I suggest to move this to M6, especially because M5 is the first stage of the API freeze i.e. API can be added after M5 but new 3.3 API that's in M5 can't be broken anymore. I will provide my feedback today or on Monday.
Move to M6 for final agreement...
Patch looks good.
After talking about the patch with Martin, here are our conclusion on this bug. This new functionality may have performance impact if user still uses the current existing ICompilationUnit#reconcile(...) method. So, it seems risky to introduce such an API change a this late stage of 3.3 dvpt. We agreed to postpone this requirement to the next version but we want to keep the new API method ICompilationUnit#becomeWorksingCopy(IProgressMonitor). So, I'll reset the target for this bug and put it as LATER but I will open a new bug for the new becomeWorkingCopy method.
As we won't address this for 3.3, I defer it...
Reopen to see if we'll put this in 3.4...
Created attachment 71167 [details] New patch on top of 3.3 RC4 This patch includes new tests using the new reconcile API methods. It also includes changes in org.eclipse.jdt.ui.tests to fix 2 compiler errors (IJavaElementDelta has now one additional method implementors must implement...) It passes all JDT/Core and JDT/UI tests :-)
