Community
Participate
Working Groups
In an API type or interface, add a method or field with a package default scope. The javadoc quickfixes will allow you to add a @noreference restriction. The TagScanner will inspect the tags and they will be added to the .api_description file. This isn't correct as a package default level API could only be accessed by another API class in the same package. Since the restrictions are intended for clients, not for the bundle's internal use, there is not value in allowing the restrictions.
Mike, what are your thoughts on this? It looks like package default methods are generally treated as valid API in the API tools code. The fix for the tag scanner is to modify org.eclipse.pde.api.tools.internal.provisional.scanner.TagScanner.Visitor.isPrivate(int) The quick fixes and bad tag markers will require additional work.
(In reply to comment #1) > Mike, what are your thoughts on this? It looks like package default methods > are generally treated as valid API in the API tools code. > This is wrong, a default scope member cannot be accessed by another bundle - the case that would cause an API error to be created. It looks like the fix to bug 253242 was only half complete - we should add an additional check in like Flags.isPackageDefault(..) to the isPrivate method you mentioned. > The quick fixes and bad tag markers will require additional work. These are also wrong and should be fixed - since they would no longer be scanned + added to the API description (with the fix above) allowing them to have tags at all would be misleading. We should still allow the 'bad tag scanner' to find tags on them to report that they are not allowed.
Pushed fix to: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=d4d7ad36cdea388cee28d5a456cd21680f1578d2 There are still a couple of test failures to sort out.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=02e544def114a3f9529e865f1f2be6c7fe1c26f2 Fixed the failing tests Added a ton more tag tests covering all the valid builder messages Fixed a couple minor problems Added quotes to the test launch config as my workspace path has a space in it I had a couple intermittent failures while running the tests. I couldn't reproduce them and they weren't related to any of the changes here.
*** Bug 404329 has been marked as a duplicate of this bug. ***
TagScannerTests still fail in N20130326-2000.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=d38693f9e1afc34e351f94df2d11357cde21f7ba Fixes the TagScannerTests Leaving this open to discuss with Mike the behaviour of IApiDescription.resolverAnnotations(). The API says it will return the parent package annotation if the element isn't part of the description. However, it ends up returning the restrictions on the parent element in the following case: public class A { @noreference public int x; } class B { } If I request annotations on class B I get the class A visibility (API). If I remove the @noreference, requesting annotations on class B returns null. The inconsistent behaviour in the tests is because of pruning in the TagScanner. If there are no restrictions the description's package map is never created.
Tests are green now (N20130327-2100).
(In reply to comment #7) > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > ?id=d38693f9e1afc34e351f94df2d11357cde21f7ba > Fixes the TagScannerTests > > Leaving this open to discuss with Mike the behaviour of > IApiDescription.resolverAnnotations(). The API says it will return the > parent package annotation if the element isn't part of the description. > However, it ends up returning the restrictions on the parent element in the > following case: > The doc could use some clean-up. It will only return the parent description if the parent has one. Using the example: /** * */ public class A { /** * @noinstantiate */ class B { } } null is returned (and expected) when asking about B - (1) because B does not appear in the description now, and (2) A is unrestricted and we have never added unrestricted parent nodes to the description if none of their children are restricted. > The inconsistent behaviour in the tests is because of pruning in the > TagScanner. If there are no restrictions the description's package map is > never created. This is sort of expected, both cases mean unrestricted (null and otherwise), but in the null case the parent is not even added to the description as mentioned above. I will fix the commented out tests and update the doc on IApiDescripton.
(In reply to comment #9) > I will fix the commented out tests and update the doc on IApiDescripton. http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=ee96ffb50dbd979f69079f3be4ccca4cf4a1ddc8 I found an issue with our invalid tag scanner, it appears it does not thoroughly check the visibility of the a member. Consider the following example: public class PublicStaticInner { static class Inner { /** * @noinstantiate This class is not intended to be instantiated by clients. * @noextend This class is not intended to be subclassed by clients. */ public static class OpenInner { /** * @noreference This method is not intended to be referenced by clients. */ public static void one() { System.out.println("one"); } } } } Those tags are not flagged as invalid when they should be.
I've created a separate bug (Bug 404744) for the problem Mike found as it is independent of this change and needs to be fixed for private types as well as package default. Marking this bug as FIXED.
This causes two issues: 1. I now get an unexpected "missing @since tag" API error on a field which has such an unsupported warning. 2. The warning I get on IJavaColorConstants.PREFIX (jdt.ui) is gibberish: Tag '@noreference' is unsupported on a package default field The buildermessages.properties file has several similar entries, which also need work. Marking as 'major', since I now have errors in my workspace.
The new warning message "Tag '@noreference' is unsupported on a package default field" is wrong for interface fields. Example: public interface IJavaColorConstants { /** * @noreference This field is not intended to be referenced by clients. */ String PREFIX= "java_"; //$NON-NLS-1$ The field PREFIX is *not* package default. It is implicitly public, static, and final, see JLS7 9.3 or the Eclipse Outline view. The message should be: "Tag '@noreference' is unsupported on a final field". (In reply to comment #12) I think we have to live with these problems, since a classfile-based API Tools implementation cannot support @noreference on constant fields. Workaround is to remove @noreference and add an API filter for the missing @since tag for now. The filter can be removed as soon as the API baseline profile has been regenerated and contains the previously-excluded field.
> (In reply to comment #12) > I think we have to live with these problems, since a classfile-based API > Tools implementation cannot support @noreference on constant fields. > Workaround is to remove @noreference and add an API filter for the missing > @since tag for now. The filter can be removed as soon as the API baseline > profile has been regenerated and contains the previously-excluded field. I recompiled my big source workspace, and the reported issue is the only one. Hence, I'm fine with adding a filter (did that for JDT). BUT: we need to add this to the migration guide and README, so that people know, what's behind this unexpected error.
*** Bug 405172 has been marked as a duplicate of this bug. ***
Created attachment 229516 [details] fix fix that still has some test failures due to changed messages.
Created attachment 229517 [details] TagScanner fix The other patch has a patch for our completion proposal computer and this patch is for the tag scanner
pushed fix + test updates to: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=391927e196dd319d6ba19497af094dde9d4804d3 I am going to leave this open until I have a build to test to make sure all is well.
(In reply to comment #18) > pushed fix + test updates to: > > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > ?id=391927e196dd319d6ba19497af094dde9d4804d3 > > I am going to leave this open until I have a build to test to make sure all > is well. 7 failures in N20130411-2000. http://download.eclipse.org/eclipse/downloads/drops4/N20130411-2000/testresults/html/org.eclipse.pde.api.tools.tests_macosx.cocoa.x86_5.0.html
(In reply to comment #19) > (In reply to comment #18) > > pushed fix + test updates to: > > > > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > > ?id=391927e196dd319d6ba19497af094dde9d4804d3 > > > > I am going to leave this open until I have a build to test to make sure all > > is well. > > 7 failures in N20130411-2000. > > http://download.eclipse.org/eclipse/downloads/drops4/N20130411-2000/ > testresults/html/org.eclipse.pde.api.tools.tests_macosx.cocoa.x86_5.0.html Fixed in: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=d7fd9fd18ad35c0c2044cfe8756758724e2af3ee The commit also fixes the case for the nooverride tag on static methods
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > pushed fix + test updates to: > > > > > > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > > > ?id=391927e196dd319d6ba19497af094dde9d4804d3 > > > > > > I am going to leave this open until I have a build to test to make sure all > > > is well. > > > > 7 failures in N20130411-2000. > > > > http://download.eclipse.org/eclipse/downloads/drops4/N20130411-2000/ > > testresults/html/org.eclipse.pde.api.tools.tests_macosx.cocoa.x86_5.0.html > > Fixed in: > > http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/ > ?id=d7fd9fd18ad35c0c2044cfe8756758724e2af3ee > > The commit also fixes the case for the nooverride tag on static methods org.eclipse.wst.jsdt.core.dom ... seriously? You know that's not "one of ours", right? :) Not sure if you committed that by accident, or really want to use it ... but, causes build to fail pretty quick (since it "can't be found").
(In reply to comment #21) > Not sure if you committed that by accident, or really want to use it ... > but, causes build to fail pretty quick (since it "can't be found"). I'm not even sure how that got in there. But it has been removed.
Tests pass now.
Marking fixed, all the tests pass and smoke testing has not revealed anything new.
Filed bug 409271 for the readme entry. Looks like PDE doc does not have a migration guide section.
