Community
Participate
Working Groups
20060127 I know this is a classic, but this would help really a lot: introduce a common supertype for ICompilationUnit and IClassFile We currently often pass around IJavaElement, that can only be an IClassFile or ICompilationUnit. This can be swallowed for internal code, but is bad for API. Example: Input for the ASTParser when creating an AST Name suggestion: ITypeContainerUnit Alternative name suggestions: IJavaFile, ISourceUnit, ISourceRoot public interface ITypeContainerUnit extends IJavaElement, IParent, IOpenable, ISourceReference, ICodeAssist { IJavaElement getElementAt(int position) throws JavaModelException; IType findPrimaryType(); } -> ICompilationUnit extends ITypeContainerUnit -> IClassFile extends ITypeContainerUnit ASTParser.setSource(ITypeContainerUnit) CompilationUnit.getSourceUnit() : ITypeContainerUnit IMember.getTypeContainerUnit : ITypeContainerUnit
If we were now designing this API, I agree this would be a good idea. Now adding this now, after years of existence feels just creating more confusion.
> adding this now, after years of existence feels just creating more confusion. I disagree. I currently try to open up the inline method refactoring to enable inlining of binary methods with source attachments. Now, what type should I choose for the variables that hold the unit for AST creation, etc.? It can be either of the proposed supertypes of ITypeContainerUnit, but whatever I choose, I have to cast it wherever I need the other type. It's probably too late for 3.2, but I would really like to get this interface in 3.3.
Created attachment 46389 [details] Patch Here's a patch with the proposed API changes.
reconsidering for 3.3
Frederic - pls check the patch, especially in the light of source and binary compatibility.
decision to be made by M3
I do not see any problem with source or binary compatibility. I'm not sure if it is really necessary to add getWorkingCopy(WorkingCopyOwner, IProblemRequestor, IProgressMonitor) on IClassFile interface? I'd rather prefer to remove it from ICompilationUnit and only have one on IJavaFile. I would also take advantage of this change to depreciate becomeWorkingCopy(...) on IClassFile and getWorkingCopy(IProgressMonitor)+becomeWorkingCopy(...) on ICompilationUnit. All these methods would be redirected to getWorkingCopy(...) of IJavaFile.
Created attachment 52107 [details] Updated patch Updated patch that also addes the API for bug 124662 as the two intersect on IJavaFile.getWorkingCopy(WorkingCopyOwner, IProgressMonitor) With the patch, it is the WorkingCopyOwner who specifies the IProblemRequestor, ast level and options (statement recovery, bindings). This is important as callers of reconcile can't know what the editor wants and so reconcile calles are lost to the editor that needs to build an AST again. See bug 124662 and discussions at the JDT summit.
Created attachment 52318 [details] Splitted patch I've splitted previous patch to keep only changes corresponding to this bug. I'll do the merge and create a new patch later when fix for bug 124662 will be released (we focus for M3 on bug 124662 first)...
Could agree on a 2 days max effort to integrate, with no deprecation induced. Idea would be that API is becoming more convenient, not a substitute. Martin would fix the doc. UI would partially convert to it (not everywhere for 3.3)
Created attachment 53826 [details] minimal patch patch that does not fix the get/becomeWorkingCopy issues with the IProblemRequestor. No deprecations from this path.
(In reply to comment #11) > Created an attachment (id=53826) [edit] > minimal patch > > patch that does not fix the get/becomeWorkingCopy issues with the > IProblemRequestor. > No deprecations from this path. > Here some remarks on this patch: 1) I do not think we'll change ASTParser class. The problem is that introducing this new interface would imply to change null tests for compilationUnitSource and classFileSource with instanceof which was a performance regression. 2) Same for org.eclipse.jdt.core.dom.CompilationUnit. There's no real benefice to change the IJavaElement element to an IJavaFile as all getJavaElement() senders would still need to perform a cast to ICompilationUnit as they already do. There's also no sender of setJavaElement(IJavaElement) method. 3) Reading doc and patch, I definitely feel that IJavaFile is really confusing. I'm 99.99% sure that clients will automatically think that this interface represents ".java" files like IClassFile represents ".class" files! I'm still trying to think to a better name but haven't been able to find any... 4) IJavaFile.getWorkingCopy(WorkingCopyOwner, IProgressMonitor) javadoc comment, contains following lines: * <p> * The problems of this working copy are reported to the {@link IProblemRequestor} configured by the * {@link WorkingCopyOwner}. * </p> However, the implementation in org.eclipse.jdt..internal.core.CompilationUnit calls getWorkingCopy(WorkingCopyOwner, IProblemRequestor, IProgressMonitor) with null for problemRequestor => sounds to be inconsistent. I guess this method should be pulled up on IJavaFile interface only when WorkingCopyOwner will have a problem requestor (ie. when bug 124662 will be implemented)...
1.) ASTParser.setJavaFile is essential and is solving half of the pain we have. Maybe I don't understand exactly but I find it hard to believe that replacing the null check with an instanceof at this place can be a performance regression. It wouldn't be called in a loop or so, right? Maybe you have more information. And if it really is, you can decide to keep the two fields. Important to us is just the API to create an AST from a IJavaFile. As the patch even reduces the number of fields in the code, I think it is a good example that IJavaFile is helpful and the right thing to do. 2.) The benefit of a CompilationUnit.getJavaFile is that you can use the protocol on IJavaFile like getBuffer, getWorkingCopy... and you can use it as an input to feed an ASTParser. All without any casts. setJavaElement is a package visible method, not API, that's right. It's not from us. 3.) I'm fine with any better name, but I think IJavaFile is quite good. 4.) Yes, that spec is wrong in the patch, I forgot to eliminate that sentence. It's describing what I think should be done in the future. In our last call with Philippe and Dani there was the agreement that we will move the IProblemRequestor to WorkingCopyOwner, but will not touch reconcile.
Created attachment 54332 [details] Modified minimal patch 1) OK, I put it back if you really need it. About performance, test to null (or a constant) is just two VM instructions (1 to put in registry + 1 to test) although instanceof implies verification in type hierarchy which obviously costs much more time. So, I've changed all instanceof with getElementType() == IJavaElement.CLASS_FILE or IJavaElement.COMPILATION_UNIT 2) OK, I also put it back but removed setJavaElement(IJavaElement) and so replaced all senders with setJavaFile(IJavaFile). I've also deprecated getJavaElement() as we know the type of the stored IJavaElement: it's an IJavaFile 3) Do you really think IJavaFile won't be misleading with ".java" file? Here are some other names I was thinking of: IJavaModelFile, IJavaUnit, IJavaModelUnit... 4) I changed it to: * </p><p> * Note that working copy's problems, if any, will not be reported using this method. * </p> 5) I've canceled the change in javadoc comment of IWorkingCopy.getWorkingCopy method. This method has a IProblemRequestor and so, we cannot advise clients to use a method which does not accept any (even when WorkingCopyOwner will have its own IProblemRequestor).
1.) Such performance considerations only make sense if this code would be located at a performance trace hot spot, for example in an inner loop. And then you would have to use performance traces to really find out whats faster: extra field and null check or instanceof check. A virtual method call (element.getElementType) doesn't have to be faster than a instanceof check . 2.) The plan is to implement the feature without introducing any deprecations. getJavaElement is still valid and does not hurt anybody. I recommend to leave it as is. 3.) 'IJavaUnit' is also a good name. 4.) As said, the idea is to later use the working copy owners problem requestor. So I would not be too specific in the comment at the moment. 5.) Calling getWorkingCopy with an own IProblemRequestor is a problem and will be discourraged in the future (you can sabotage the editor's error updating if you pass in an own problem requestor. So the recommendation to use the method without problem requestor is in fact a good one...
(In reply to comment #15) 1.) You're perhaps right for virtual method calls... However, for ASTParser, I reduced 2 instanceof tests to only one getElementType() call => it should be faster. And even if this is not a hot spot, why can't we apply it? If this is faster even only once then I think we have to use it... 2.) OK, I remove the @deprecated tag. I guess it was OK for setJavaElement removal? 3.) I also like 'IJavaUnit'... Jerome, what's your mind about this name? 4.) OK, I remove the paragraph 5.) If there's a better usage then it should be documented more than that. Perhaps something like: * @deprecated Use {@link IJavaFile#getWorkingCopy(WorkingCopyOwner, IProgressMonitor)} instead as given problem requestor may interfere with editor's error updating...
(In reply to comment #16) > 3.) I also like 'IJavaUnit'... Jerome, what's your mind about this name? Yes, IJavaUnit is better than IJavaFile. However it still contains the word 'Java'. ISourceUnit would not contain the word 'Java' and would reflect that IClassFile are considered source elements in the Java model (even if their content is binary or even if they have no source attached).
1.) It seems I was finally definitely wrong on this point! :-( => I'll put back instanceof tests...
>1.) It seems I was finally definitely wrong on this point! :-( > => I'll put back instanceof tests... What made you convert you to this?
I think a good rule is to always choose the most readable/maintainable solution and only do performance tuning if the location has been identified as a performace hot spot. So, I think instanceof and getElementType are equally nice. Defintly nicer than two fields. But it's really all up to you, I only protested that this change would be a reason to reject the new API. ISourceUnit sounds good to me.
(In reply to comment #19) > >1.) It seems I was finally definitely wrong on this point! :-( > > => I'll put back instanceof tests... > What made you convert you to this? > Jerome demonstrated that virtual method call costs twice: type hierarchy scan + search to find method in the map => this was definitely killing my initial argument. (In reply to comment #20) > I think a good rule is to always choose the most readable/maintainable > solution and only do performance tuning if the location has been identified > as a performace hot spot. > I agree on this point. > So, I think instanceof and getElementType are equally nice. Definitely nicer > than two fields. But it's really all up to you, I only protested that this > change would be a reason to reject the new API. > This is no longer the case :-) > ISourceUnit sounds good to me. > It seems we have an agreement on this name => I'll change it
Created attachment 54351 [details] New minimal patch This new patch includes all previous comments remarks. Please review while I'm running all tests on it, thanks
patch looks good to me, but remove 2 unused imports on ASTRewrite and ImportRewrite!
(In reply to comment #23) > patch looks good to me, but remove 2 unused imports on ASTRewrite and > ImportRewrite! > Thanks for the review, I've removed these unused imports and all JDT/Core tests pass. However, I've talked with Philippe about the interface name and he does not like neither ISourceUnit nor IJavaUnit... After a long discussion here, we got a new agreement: 1) new name would be IBufferedElement 2) we would add getBuffer(), save(...), isConsistent() and makeConsistent(...) methods on this new interface 3) we would depreciate corresponding methods on IOpenable... Martin, If you agree on this new proposal, I'll prepare a new patch. Otherwise discussion will continue...
Can you give some reasons why Philippe didn't like ISourceUnit? It makes more sense that IBufferedElement. The new type is the holder of types (getChildren, findPrimaryType), it's parent is defined to be a package fragment ect. IBufferedElement seems to describe what IOpenable does. Note that we don't want to deprecate API.
(In reply to comment #25) > Can you give some reasons why Philippe didn't like ISourceUnit? > He said that it may be also confusing for user to think that a class file is considered as a source unit... He also said that neither 'File' nor 'Unit' should be used in this name as these words already have an implicit signification. > It makes more sense that IBufferedElement. The new type is the holder of types > (getChildren, findPrimaryType), it's parent is defined to be a package > fragment ect. IBufferedElement seems to describe what IOpenable does. > In fact package fragments do not have a buffer, only class files and compilation units may have one... So, for us, it sounds like the main functionality of this new interface would be to distinguish an IOpenable which may have a buffer. Then, the new name IBufferedElement really makes sense! [I didn't see IJavaFile.getChildren() method in any of your patches, but if you think that the main interest for this interface is to find primary type, then Philippe was also thinking about ITypeContainer...] > Note that we don't want to deprecate API. > I know, but in this case we would take advantage of this change to fix something not really accurate in existing API...
-1 for IBufferedElement. This sounds strange. What's buffered on an IClassFile? I like ISourceUnit but if you cannot live with that what about ITopLevelTypeContainer?
ISourceUnit has a connotation with source vs. binary, which I think is confusing in the context of (binary) classfile.
(In reply to comment #26) See the type declaration: "public interface ISourceUnit extends IJavaElement, IParent, IOpenable, ISourceReference, ICodeAssist". The new interface extends all common superinterfaces of IClassFile and ICompilationUnit, so it already supports all the inherited methods like getChildren(), getBuffer(), save(...), etc. . That's also why the original name suggestion was ITypeContainerUnit. We could talk about deprecating the buffer-related methods in IOpenable (overriding them with a non-deprecated version in the new interface), but that can also be done later, in another bug.
re: ISourceUnit. Take a classfile, which would be a Source unit. Ask its pkg fragment root what kind it is ---> Binary !?!
ITypeUnit ?
>ITypeUnit ? +1
As Jerome said in comment 17, in our model we treat IClassFile as a source element, IClassFile is implementing IOpenable, ISourceReference, ICodeAssist... I think we are consistent here. Other suggestion: 'ITypeRoot' ISourceUnit is still my favourite.
As we need to close this discussion _today_, let's chose between: a) ITypeUnit b) ITypeRoot Please vote...
(In reply to comment #34) > b) ITypeRoot +1
> b) ITypeRoot +1
It will be ITypeRoot! (as it was Martin who suggested it, I guessed he agrees on it. Philippe directly told me it was OK)
Created attachment 54476 [details] Last proposed patch This patch includes the change to new name ITypeRoot (refactoring was really helpful to to this... great job!) + several other changes: 1) ClassFile#findPrimaryType() is now implemented similarly to CompilationUnit corresponding method: get the type, verify that it exists and return null in case not. 2) Create new element type in IJavaElement: TYPE_ROOT 3) Remove getTypeRoot() method from JavaElement and SourceRefElement. I just implemented it on Member as: public ITypeRoot getTypeRoot() { return (ITypeRoot) getAncestor(TYPE_ROOT); }
Released for 3.3 M4 in HEAD stream.
(In reply to comment #41) > 2) Create new element type in IJavaElement: TYPE_ROOT > 3) Remove getTypeRoot() method from JavaElement and SourceRefElement. I just > implemented it on Member as: > public ITypeRoot getTypeRoot() { > return (ITypeRoot) getAncestor(TYPE_ROOT); > } > IJavaElement#TYPE_ROOT has been removed as these constants are only for leaf types (thanks Markus for the feedback). So, point 2) has been reverted and point 3) is now written: public ITypeRoot getTypeRoot() { IJavaElement element = this; while (element != null) { if (element instanceof ITypeRoot) return (ITypeRoot) element; element= element.getParent(); } return null; }
I've also tuned Member.getTypeRoot(), Member.getClassFile(), SourceRefElement.getCompilationUnit() methods and make public ASTParser.setTypeRoot(...) method after Markus review... For verifiers, no specific test case has been added. Verification should be done looking at code.
2 further comments: (1) ASTParser#setTypeRoot(ITypeRoot) => shouldn't this be called setSource(ITypeRoot)? That would avoid compile errors when clients change the type of their variables to ITypeRoot. (2) For API consumers, it now looks like the methods from ITypeRoot have only been added in 3.3. However: - IClassFile/ICompilationUnit#getElementAt(int) has been there since day 1 => I would add @since 1.0 to ITypeRoot#getElementAt(int) - ICompilationUnit#findPrimaryType() is available since 3.0 (only new for IClassFile) - IClassFile#getWorkingCopy(WorkingCopyOwner, IProgressMonitor) is available since 3.0 (only new for iCompilationUnit) => This should either be mentioned in the Javadoc of the current declarations, or the old declarations should be restored with /** * {@inheritDoc} * * @since 3.0 */
One after-thought, when looking at end result. I think ITypeRoot sounds bad. It feels like the object is a true type, which is toplevel. All we want to express is that it contains at least one toplevel type. I thus think ITypeUnit was superior.
As an IPackageFragment is a child of an IPackageFragmentRoot, an IType is a child of an ITypeRoot.
Verified for 3.3M4 with I20061212-0010.
