Community
Participate
Working Groups
Created attachment 242336 [details] Project to recreate this Import the attachment. In class ClassTest1 public class ClassTest1 extends ClassTest2{ public void m3(){ m1(); } } call to m1() should be reported as illegal reference. Currently it is not. This is a problem for both javadoc tag and annotations.
Created attachment 242704 [details] Proposed patch All the api tools junits run fine with this fix. Curtis, can you please review this for 4.4 RC1
Fixes to both Bug 433528 & Bug 433527 has refresh issues. Currently the APItype's methods are not refreshed since they are cached. Using resolveType solves this issue but I'm looking at whether this issue has bigger impact of PDE API error reporting or not and whether there is some other cleaner alternative to solve this issue.
Basically IncrementalApiBuilder's public boolean visit(IResourceDelta delta) throws CoreException { should have ensure that APIType's are updated with the latest changes. But seeing APIType's @Override public IApiType getSuperclass() throws CoreException { String name = getSuperclassName(); if (name == null) { return null; } if (fSuperclass == null) { ....resolveType return fSuperclass; } and resolveType(name); I am not sure if there is a flow that could update the structure of APITypeRoot from updated classfile. So according to me there are 2 options. a) somehow ensure that IncrementalApiBuilder updates the APIType b) during API error analysis force a resolveType of APIType. But for doing this this function has to be changed from private to public.
Minor issues: - Should either explicitly do nothing when CoreException found or we should log it - Reference is the only implementation of IReference and we don't check instanceof elsewhere. The check is ok by me though. Major issues: - Performance, this change would now resolve every possible reference. We do this for all references flagged as DEFAULT_METHOD which is already a performance hit, doing it every time is a mistake. The issue here is that without resolving the reference, we have the wrong referenced type name (it defaults to the enclosing type of the reference member). A better first check would be IApiType referenceType = reference.getMember().getEnclosingType(); if (reference.getReferencedTypeName().equals(referenceType.getName())) { boolean mustResolve = referenceType.getMethod(reference.getReferencedMemberName(), reference.getReferencedSignature()) == null; } Once we know that the referenceType is incorrect, we could choose to resolve the reference or do some further lookups in the illegal method list (not currently possible without the proper typename as it does a hash lookup).
Created attachment 243073 [details] Updated patch 1) exception logged 2) reference cast kept in case in future we have more kind of references 3) performance check ( double check in fact) as stated by you added All API tool tests PASS with this change !!! 2 other issues -------------- a) May be we may need to revisit this once "refresh" issue is solved. b) Will add test cases for @NoOverride for static method as well as this one - once I document and file all kinds of issues in and around this area. I expect around 6 to 12 test cases that would be added.
The fact that we have many method usage tests, yet this common case isn't covered suggests it was done on purpose. Resolving all virtual method invocations will have a significant impact on performance. I can't find a bug comment referencing this choice however. I did some crude performance measurements and when building jdt.ui, ~1100ms is spend resolving references with the patch than would be otherwise. Bumping to 4.5. If we can find a way to cut down on the instances where we resolve the reference, great. If not we should have tests that explicitly test these cases with an explanation on why we chose not to cover them.
This is probably work as designed. Need to investigate more !
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.
