This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
I am working on a public (or more precisely under development) API for writing Java Hints. Quite a few things are not finished yet (see the linked page), but any early feedback would still be very welcome. A concise description of the state of the API, the intents behind it and links to continuous build, Javadoc and patch are here (also linked from the URL field): http://wiki.netbeans.org/JavaHintsAPIReview I don't think this API could or should go through the fast track review. Jesse, Petr Hejl, Dusan and Ralph agreed to be reviewers of the API. Thanks in advance for any comments.
I was about to try to play with the branch, e.g. rewriting UseNbBundleMessages to use the new API rather than AbstractHint, until I realized that it uses MQ which is not friendly for collaboration. Consider using a plain named branch, or pbranch if you want to maintain multiple related patches. It is hard to evaluate the API in much detail when so much Javadoc is missing. I do not mean that every single throws clause must be documented in official style, but the meaning of e.g. the JavaFix.rewriteFix or Pattern.createPatternWithRemappableVariables overloads is hard to guess at just from the signature. [JG01] Avoid the subclassing antipattern TestBase extends NbTestCase; provide static utility methods, or if necessary builders, for use from tests with any structure. Also avoid explicit references to JUnit 3 APIs (other than Assert) such as junit.framework.TestSuite if possible, to make it easier to use the test utility with various frameworks (JUnit 4, TestNG). [JG02] The idiom of @Hint applied to a class of no particular type, then nested static methods with a verbally described signature and one of two trigger annotations, plus some (ordered!) @BooleanOption's or @UseOptions's, seems needlessly haphazard. Would suggest the more standard pattern of an interface that must be implemented and an annotation to register it with the minimum metadata needed for lazy loading: public interface /* or abstract class */ Hint { List<? extends ErrorDescription> run(HintContext context); String[] suppressWarningsKeys(); JComponent createCustomizer(); @Target({TYPE, METHOD}) // assignable to Hint @Retention(SOURCE) @interface Registration { String category(); String id() default ""; boolean enabled() default true; HintSeverity severity() default WARNING; Kind kind() default HINT; BooleanOption[] booleanOptions() default {}; Options[] options() default {}; // maybe rename! String[] triggerPattern() default {}; ConstraintVariableType[] triggerConstraints() default {}; Tree.Kind[] triggerKinds() default {}; } @Target() // only used from @Registration @interface BooleanOption { String key(); boolean defaultValue(); String displayName(); String toolTip(); } } If many "new trigger [type]s may be added", it would be possible to invert the above so that each trigger type is a top-level annotation, and the hint metadata is defined in a shared place: @Target() @interface HintInfo { String category(); // ... } @Target({TYPE, METHOD}) @interface PatternTrigger { HintInfo info(); String[] pattern() default {}; ConstraintVariableType[] triggerConstraints() default {}; } [JG03] Requiring the special bundle key names DN_id and DESC_id is nonstandard. Rather add String displayName() and String description() attributes to the registering annotation, using LayerBuilder.File.bundlevalue(String, String, Annotation, String) in the normal way so localizable values can be supplied for those who care. Same for @BooleanOption (see example in JG02). [JG04] Javadoc of ConstraintVariableType.type is too vague. Should this be the canonical or binary name of a nested type? Is java.lang effectively imported? How are primitives and arrays represented? Are generics permitted? If so, what about wildcards and raw types? And does the variable need to be declared to be of this exact type, or may it be declared to be of a subtype, or may it merely be assignable to this type? [JG05] Javadoc of TriggerPattern.value must describe the syntax of trigger patterns, which I guess is that used by Jackpot (but this is nowhere documented very clearly that I know of). [JG06] What is HintContext.getPreferences() for? If for BooleanOption's, I would rather expect to be given a Map<String,Boolean> of declared option keys. [JG07] It seems that hint customization may involve creating a JComponent in the general case. If so, it would be more straightforward to drop declarative BooleanOption completely, and just offer a support SPI factory method which creates a panel with a list of checkboxes corresponding to boolean keys you specify. It is not obvious even this is desirable; writing a small form with trivial data binding is not much of a burden, and is far more flexible since you control aspects of the layout and offer other Swing controls. In general, nothing should be forced into an annotation unless that is required in order to maintain acceptable startup time and/or memory usage when many providers are being handled at once; plain old Java programming is always preferable. [JG08] Matcher.OccurrenceDescription should be a top-level class (probably just "Occurrence" or "Match"). [JG09] setCancel(AtomicBoolean) should probably be replaced by implementing org.openide.util.Cancellable. [JG10] FixFactory class Javadoc is obsolete (no longer just suppress warnings fixes). [JG11] Would really appreciate some convenience API to add or update an annotation to an element (including a CompilationUnitTree or a package-info.java). org.netbeans.modules.apisupport.hints.Hinter.Context.addAnnotation and org.nbheaven.sqe.tools.findbugs.codedefects.hints.FindBugsHint.Task.SuppressWarningsFix.addSuppressWarnings show how tricky this can be. But I think this would be better placed in java.source for reusability, perhaps with a method in FixFactory to call it as the sole effect of a fix. [JG12] "always produce the Fix as a subclass of JavaFix" cannot be right since JavaFix itself is not assignable to anything. Do you mean that you should try to use one of the Fix-returning methods in JavaFix? By the way static Fix toEditorFix(JavaFix javaFix) inside JavaFix is silly; prefer (nonstatic) Fix toEditorFix(). [JG13] Would be nice for JavaFix to either document that you may perform non-Java modifications in performRewrite, or (better) offer some other method in which you can prepare such work so that it can be previewed in the Refactoring window and either applied or canceled. UseNbBundleMessages would need this, as would anything else that somehow manipulates resource files as part of the fix. (And is there any way to modify other source files as part of a fix?) [JG14] org.netbeans.spi.java.hints.matching does seem like it would better belong in java.source. Various tools might wish to scan the source base for particular patterns without necessarily using the hint UI. [JG15] Might be useful to have a standard way for a hint to express that it is only applicable when certain libraries are in the classpath. Or that it should always be displayed, but that certain fixes are only available in such a case. I see this is in your "Future Evolution Paths" section. Not sure if there is any need for this to be expressed declaratively, or indeed if any special API is needed for this at all; without any particular support you can check whether compilationInfo.getClasspathInfo().getClassPath(COMPILE).findResource("pkg/Type.class") != null, it just seems a little clumsy and misses the opportunity to cache this check when rerunning hints. Of course this is mainly useful when the target of a rewrite would use some API which the origin does not; otherwise the trigger pattern would suffice to exclude the hint from consideration on inappropriate projects.
Thank you very much for your comments. (In reply to comment #1) > I was about to try to play with the branch, e.g. rewriting UseNbBundleMessages > to use the new API rather than AbstractHint, until I realized that it uses MQ > which is not friendly for collaboration. Consider using a plain named branch, > or pbranch if you want to maintain multiple related patches. Ok, I will move into a separate branch - what I like better on MQs is that it is much easier to review the patch (and even the parts of the patch may be removed manually, making it easier to do patch cleanups). It is even possible to add some temporary hacks into the queue without a risk of forgetting about them and pushing them into the production build. > > It is hard to evaluate the API in much detail when so much Javadoc is missing. > I do not mean that every single throws clause must be documented in official > style, but the meaning of e.g. the JavaFix.rewriteFix or > Pattern.createPatternWithRemappableVariables overloads is hard to guess at just > from the signature. Yes - the API is pretty big and I was not able to fully document it yet. > > > [JG01] Avoid the subclassing antipattern TestBase extends NbTestCase; provide > static utility methods, or if necessary builders, for use from tests with any > structure. > > Also avoid explicit references to JUnit 3 APIs (other than Assert) such as > junit.framework.TestSuite if possible, to make it easier to use the test > utility with various frameworks (JUnit 4, TestNG). Ok - will look into this. I would like to make the testing as convenient as possible, so some experimentation with static utilities (or more probably a builder-like pattern) will be needed. Getting a reasonable work dir will be a bit tricky without references to JUnit 3/NbJUnit. An alternative is to have a fully-declarative tests, as are the current declarative tests for Jackpot 3.0 rules. > > > [JG02] The idiom of @Hint applied to a class of no particular type, then nested > static methods with a verbally described signature and one of two trigger > annotations, plus some (ordered!) @BooleanOption's or @UseOptions's, seems > needlessly haphazard. Would suggest the more standard pattern of an interface > that must be implemented and an annotation to register it with the minimum > metadata needed for lazy loading: > > public interface /* or abstract class */ Hint { > List<? extends ErrorDescription> run(HintContext context); There are a few problems with this, for which I have left this path: -a hint must be a class. With the proposed API, its possible to create hint consisting of a single method, having more than one hint in one class. As methods are cheaper than classes, which can save loading of some classes (by a quick search, there are 20+ hints written as methods in the current java.hints, which would explode to new classes). -the signature is inflexible. Almost all hints need to return only one ErrorDescription per invocation. But, some relatively rare hints need to return more than one. As the signature of the interface method must specify the most generic case, all hint implementors are forced to wrap their sole ErrorDescription with a list, which is pretty inconvenient. (This API does not have a dozen or a few dozens of uses - it has about a hundred now, and may have much more in the future, so having it more convenient rather than less convenient seems good to me.) -a hint may be only one method, triggered by one trigger. Although not very common, there are hints that (for various reasons) use more than one triggered method. In some cases, it could be rewritten with some inconvenience, like: http://hg.netbeans.org/main-silver/file/tip/java.hints/src/org/netbeans/modules/java/hints/jdk/ThrowableInitCause.java But in some cases, the actual semantics of the hint may require more than one triggered method to efficiently implement the hint at all. Unbalanced: http://hg.netbeans.org/main-silver/file/tip/java.hints/src/org/netbeans/modules/java/hints/bugs/Unbalanced.java is in this direction, although it is not an example of a particularly nice hint (and some further API enhancements would make it much nicer). -having an instance of a hint requires specification of its lifecycle: does the hint instance exist only trasiently while processing one HintContext (one triggered location?). Or while processing one file? Or "forever"? In the latest case, the trouble is that users tend to store something in fields, and forget about them. This is of course problem even in the current proposal, but people are generally aware that storing something in a static field may not be a good idea, unlike storing something in an instance field. And keeping a single instance of a javac object may easily lead to a huge memory leak. There is of course certain level of undiscoverability in the current proposal, but I would assume that can be mitigated by having an APISupport wizard. > String[] suppressWarningsKeys(); Suppress warnings keys need to be in the "declarative" part, because they may prevent the hint from instantiating. > JComponent createCustomizer(); The customizer is tricky: having it as a non-declarative method on the hint would mean that the hint classes would be loaded while browsing through the options. Having "customizer" attribute in the annotation would require a separate customizer class for each hint. (A note on missing Javadoc entry&missing check in the annotation processor: the customizer needs to get an instance of Preferences, into which it writes the modified values and from which it reads the original values.) > @Target({TYPE, METHOD}) // assignable to Hint > @Retention(SOURCE) > @interface Registration { > String category(); > String id() default ""; > boolean enabled() default true; > HintSeverity severity() default WARNING; > Kind kind() default HINT; > BooleanOption[] booleanOptions() default {}; > Options[] options() default {}; // maybe rename! > String[] triggerPattern() default {}; > ConstraintVariableType[] triggerConstraints() default {}; > Tree.Kind[] triggerKinds() default {}; > } > @Target() // only used from @Registration > @interface BooleanOption { > String key(); > boolean defaultValue(); > String displayName(); > String toolTip(); > } > } > > If many "new trigger [type]s may be added", it would be possible to invert the > above so that each trigger type is a top-level annotation, and the hint > metadata is defined in a shared place: > > @Target() @interface HintInfo { > String category(); > // ... > } > @Target({TYPE, METHOD}) @interface PatternTrigger { > HintInfo info(); > String[] pattern() default {}; > ConstraintVariableType[] triggerConstraints() default {}; > } > > > [JG03] Requiring the special bundle key names DN_id and DESC_id is nonstandard. > Rather add String displayName() and String description() attributes to the > registering annotation, using LayerBuilder.File.bundlevalue(String, String, > Annotation, String) in the normal way so localizable values can be supplied for > those who care. > > Same for @BooleanOption (see example in JG02). Great idea. Ideally, the annotation would contain directly the text (display name and description/tooltip), and would generate the key into the bundle automatically. That would make it much more convenient. Would require cooperation with NbBundleProcessor. > > > [JG04] Javadoc of ConstraintVariableType.type is too vague. Should this be the > canonical or binary name of a nested type? Is java.lang effectively imported? > How are primitives and arrays represented? > > Are generics permitted? If so, what about wildcards and raw types? And does the > variable need to be declared to be of this exact type, or may it be declared to > be of a subtype, or may it merely be assignable to this type? Will update the javadoc. > > > [JG05] Javadoc of TriggerPattern.value must describe the syntax of trigger > patterns, which I guess is that used by Jackpot (but this is nowhere documented > very clearly that I know of). Will do my best. > > > [JG06] What is HintContext.getPreferences() for? If for BooleanOption's, I > would rather expect to be given a Map<String,Boolean> of declared option keys. For any settings the hint may want to use. A custom panel may set a value of any type supported by Preferences to them. > > > [JG07] It seems that hint customization may involve creating a JComponent in > the general case. If so, it would be more straightforward to drop declarative > BooleanOption completely, and just offer a support SPI factory method which The reason why I introduced BooleanOption is that having (one) boolean setting is very common (among hints that actually require a customization). Extending that to more than one (boolean) setting seemed reasonable, with possible future enhancement to other options (IntegerOption?, TextOption?), although there is not enough usecases for that right now. The problem with a factory method is that it requires (almost) one new class per hint (if to be registered in the annotation), which needs to be loaded once the user navigates to that hint in options. > creates a panel with a list of checkboxes corresponding to boolean keys you > specify. It is not obvious even this is desirable; writing a small form with > trivial data binding is not much of a burden, and is far more flexible since > you control aspects of the layout and offer other Swing controls. Most hints do not need the flexibility (although some done, and the API allows that). For many hints, the burden of creating the form is at the same level as creating the hint itself, not speaking about the extra class for the hint (which would be practically same as many other customizers to other hints). > > In general, nothing should be forced into an annotation unless that is required > in order to maintain acceptable startup time and/or memory usage when many > providers are being handled at once; plain old Java programming is always > preferable. > > > [JG08] Matcher.OccurrenceDescription should be a top-level class (probably just > "Occurrence" or "Match"). Will do. > > > [JG09] setCancel(AtomicBoolean) should probably be replaced by implementing > org.openide.util.Cancellable. Will make the Matcher cancelable. But the setCancel method is still needed, there are quite a few places that need to use Matcher which cannot call the cancel method on it. > > > [JG10] FixFactory class Javadoc is obsolete (no longer just suppress warnings > fixes). > > > [JG11] Would really appreciate some convenience API to add or update an > annotation to an element (including a CompilationUnitTree or a > package-info.java). > org.netbeans.modules.apisupport.hints.Hinter.Context.addAnnotation and > org.nbheaven.sqe.tools.findbugs.codedefects.hints.FindBugsHint.Task.SuppressWarningsFix.addSuppressWarnings > show how tricky this can be. But I think this would be better placed in > java.source for reusability, perhaps with a method in FixFactory to call it as > the sole effect of a fix. I have nothing against such convenience methods (although I the usecases covered by the two listed methods are probably different). But these do not really belong into the Java hints. Moreover, a Java hint should no normally generate @SuppressWarnings itself - the infrastructure handles it automatically. > > > [JG12] "always produce the Fix as a subclass of JavaFix" cannot be right since > JavaFix itself is not assignable to anything. Do you mean that you should try > to use one of the Fix-returning methods in JavaFix? Yes. Good catch, will write that correctly. Its actually not "try to use". The Fix must be the outcome of toEditorFix, backed by a JavaFix, otherwise it cannot be applied in Inspect&Refactor. As a recent change, I have moved most of the JavaFix method into JavaFixUtilities, to prevent possible problems when clients are extending JavaFix. An alternative would be that the java.hints' ErrorDescriptionFactory would take JavaFix rather than a Fix - would make the matter much clearer (based on an offline suggestion by dbalek). > > By the way static Fix toEditorFix(JavaFix javaFix) inside JavaFix is silly; > prefer (nonstatic) Fix toEditorFix(). Ok. > > > [JG13] Would be nice for JavaFix to either document that you may perform > non-Java modifications in performRewrite, or (better) offer some other method > in which you can prepare such work so that it can be previewed in the > Refactoring window and either applied or canceled. UseNbBundleMessages would > need this, as would anything else that somehow manipulates resource files as > part of the fix. > > (And is there any way to modify other source files as part of a fix?) Not currently (aside from actually doing the change in the file on the disk, which is clearly wrong). I've changed the signature of the performRewrite method in JavaFix to take TransformationContext, which should be extended so that it would allow recording of non-Java changes. > > > [JG14] org.netbeans.spi.java.hints.matching does seem like it would better > belong in java.source. Various tools might wish to scan the source base for > particular patterns without necessarily using the hint UI. I am not completely sure if java.source is the correct module - a new module might be more appropriate (this API may be a bit too high level for java.source). > > > [JG15] Might be useful to have a standard way for a hint to express that it is > only applicable when certain libraries are in the classpath. Or that it should > always be displayed, but that certain fixes are only available in such a case. > I see this is in your "Future Evolution Paths" section. Not sure about the exact solution (esp. for the fixes part), but having @ConstraintTypeAvailable, or alike, is definitelly a plan. I just did not want to pollute the already pretty big API change with new non-essential changes. > > Not sure if there is any need for this to be expressed declaratively, or indeed > if any special API is needed for this at all; without any particular support > you can check whether > compilationInfo.getClasspathInfo().getClassPath(COMPILE).findResource("pkg/Type.class") > != null, it just seems a little clumsy and misses the opportunity to cache this > check when rerunning hints. > > Of course this is mainly useful when the target of a rewrite would use some API > which the origin does not; otherwise the trigger pattern would suffice to > exclude the hint from consideration on inappropriate projects.
(In reply to comment #2) > what I like better on MQs is that it > is much easier to review the patch (and even the parts of the patch may be > removed manually, making it easier to do patch cleanups). It is even possible > to add some temporary hacks into the queue without a risk of forgetting about > them and pushing them into the production build. The pbranch extension has most of the same advantages, but is much easier to collaborate with. Of course it has some drawbacks too, most significantly that it is not trivial to reorder patches, but I would recommend it over MQ for long-lived changes especially when multiple people and/or a Hudson job may be involved. If you are only working on a single patch, i.e. do not need the dependent branch feature of pbranch, a simple named branch works fine. Reviewing the running diff is easy enough: [alias] branchdiff = diff -r ancestor(default,.) Regarding manual patch editing - this is risky even in MQ (offset lines may be messed up by your edit, for example, and you need to be sure the patch is not currently applied). Generally better to use revert or import to selectively back out parts of the change, then refresh the patch.
From the hint implementor point of view: [PH01] Missing javadoc for Hint.Kind and HintSeverity categories. BTW perhaps it would be better to have Hint.Kind and Hint.Severity or HintKind and HintSeverity. [PH02] TriggerPattern: Perhaps for users not really familiar with the editor the sentence "Invoke the method for TreePaths that match the given pattern." does not help much. What is the pattern and how is the matching done? [PH03] JavaFix is missing javadoc. [PH04] Not sure if FixFactory.isSuppressWarningsFix() has any usage in the api.
The changes are currently in the spi.java.hints branch of the prototypes repository. I was still not able to modify the continuous build to produce the diff automatically ("ancestor(default,.)" works since 1.7, while the server has only 1.4, and debugancestor does not seem to work correctly on the server as well). JG01: I have introduced HintTest that allows to create tests using a builder-like pattern: http://bertram2.netbeans.org:8080/job/prototypes-spi.java.hints/javadoc/org-netbeans-modules-java-hints-test/org/netbeans/modules/java/hints/test/api/HintTest.html I have rewritten a test to show how this can be used: http://hg.netbeans.org/prototypes/file/spi.java.hints/java.hints/test/unit/src/org/netbeans/modules/java/hints/perf/SizeEqualsZeroTest.java If this seems like a good direction, I will change all the tests and delete TestBase. JG03: If we could introduce AbstractBundleProcessor, similar to LayerGeneratingProcessor, that would help a lot. I will attach a patch that is in this direction - is that something that could be considered? JG04: done JG10: done Thank you very much Petr for your comments. PH01: I moved HintSeverity->Hint.Severity and added a documentation PH02 (similar to JG05): not done yet. PH03: Added a javadoc to JavaFix, not yet for JavaFixUtilities. PH04: I have removed the suppress warnings methods from the FixFactory and updated its javadoc (JG10).
Created attachment 115419 [details] A sketch of AbstractBundleProcessor. Separates creation of Bundle.properties into a separate class, AbstractBundleProcessor, makes NbBundleProcessor a subclass of the AbstractBudleProcessor, and introduces another processor in java.hints that produces keys to bundles.
A few comments after trying the spi for some i18n hints. [R01] I couldn't understand the Hint.Kind's values (HINT, SUGGESTION) without reading the javadoc. I think the values (INSPECTION, ACTION) would be better. [R02] I think the number of Hint.Severity levels is too limited. Most used seems to be five levels. (1=most severe, 5=least severe) [R03] It looks like the Hint.Severity is used to specify when the hint is displayed (CURRENT_LINE_WARNING). It should be enough to look at the Hint.Kind to see when/how to display.
Another comment from the hint implementor point of view: [DB01] Provide a simple way to get the caret position from the hint method (perhaps by adding HintContext.getCaretPosition()?) that would prevent the current practice of calling 'CaretAwareJavaSourceTaskFactory.getLastPosition(context.getInfo().getFileObject())'
(In reply to comment #2) > Getting a reasonable work dir will be a bit tricky without references to JUnit > 3/NbJUnit. The typical idiom is to just to pass a File workDir parameter to the test utility, e.g. HintTest.create. >> [JG02] The idiom of @Hint applied to a class of no particular type > > methods are cheaper than classes, which can save loading of some classes True, but by this argument we should never use anonymous inner classes at all, and strive to implement as many interfaces as possible on every concrete class we do have. In the longer run I would expect lambdas to make it cheap to express closures: @Hint.Registration(...) public static Hint myhint = (context) -> {...}; > the signature is inflexible Just means you introduce MultiHint returning a List, and accept that as an alternate target type. > in some cases, the actual semantics of the hint may require more than one > triggered method to efficiently implement the hint at all. Unbalanced already has two separate @Hint registrations so I am confused by this example. I guess you mean the occasional case where two or more triggers should have exactly the same display name and description and other metadata? It is true that the @Hint.Registration params would need to be copied in such cases. > the trouble is that users tend to store something in fields, and forget > about them. This is of course problem even in the current proposal, but people > are generally aware that storing something in a static field may not be a good > idea, unlike storing something in an instance field. This is quite true, though it applies to all registration annotations applied to classes, e.g. @ServiceProvider; I have considered making their processors issue warnings if you attempt to add an instance field to such classes, since such fields could introduce memory leaks and should either be removed or made explicitly static. > There is of course certain level of undiscoverability in the current proposal, > but I would assume that can be mitigated by having an APISupport wizard. Yes, to some extent. I was more concerned about stylistic inconsistency, though we are already in a bit of mess most notably with context-sensitive @ActionRegistration, and alignment with the Java type system (e.g. finding all subtypes of Hint). Not a blocking issue by any means, just something I wanted to bring up as an alternative. > Suppress warnings keys need to be in the "declarative" part, because they may > prevent the hint from instantiating. Makes sense, I was just guessing here. > The customizer is tricky: having it as a non-declarative method on the hint > would mean that the hint classes would be loaded while browsing through the > options. True. Not as important as avoid loading of hints while using the editor. Possibly solvable by having an extension or mixin interface CustomizableHint, and recording in the processor whether that is implemented or not. > missing Javadoc > entry&missing check in the annotation processor: the customizer needs to get an > instance of Preferences Sounds to me like you should define an interface expressing this contract, and define the annotation attribute to be assignable to that interface. > Ideally, the annotation would contain directly the text (display > name and description/tooltip), and would generate the key into the bundle > automatically. That would make it much more convenient. Would require > cooperation with NbBundleProcessor. -1 on such special treatment in just this annotation. If you want to introduce an API change in LayerBuilder.File such as a bundlevalue variant which accepts e.g. displayName="##Actual English Label", cooperating with NbBundleProcessor and picking a key based on the annotated element, annotation (incl. nested!), and annotation method, and use this new variant in all existing annotations, and extend UseNbBundleMessages to suggest it, that would be a good API review to file separately. In the meantime the standard idiom works well enough with hints. > I have nothing against such convenience methods (although I the usecases > covered by the two listed methods are probably different). But these do not > really belong into the Java hints. Agreed, can be a separate API change. > Moreover, a Java hint should no normally > generate @SuppressWarnings itself - the infrastructure handles it > automatically. In the FindBugs case, it cannot use java.lang.SuppressWarnings. > [alternately] the java.hints' ErrorDescriptionFactory would take > JavaFix rather than a Fix - would make the matter much clearer Yes, that would seem clearer. But then for true safety you should not use the (generic editor's) ErrorDescription at all; introduce JavaErrorDescription and require a @Hint to return it. > Not currently (aside from actually doing the change in the file on the disk, > which is clearly wrong) That is what I will do in UseNbBundleMessages for now, pending a better API, e.g. another method in JavaFix such as protected void performAdditionalChanges() throws IOException {} Which reminds me of [JG16] Need trigger replacement for ErrorRule, e.g. for SearchClassDependencyInRepo. (In reply to comment #5) > the server has only 1.4 File a bug in www/builds & repositories please. > I have rewritten a test to show how this can be used: > [...]/SizeEqualsZeroTest.java > > If this seems like a good direction Looks nice to me.
BTW JG13 also needed by EmbeddableEJBContainerHint. Probably also by JavadocHintProvider, which would also be hard to rewrite depending on JG12 is resolved.
Thanks for the comments. (In reply to comment #7) > A few comments after trying the spi for some i18n hints. > > [R01] I couldn't understand the Hint.Kind's values (HINT, SUGGESTION) without > reading the javadoc. I think the values (INSPECTION, ACTION) would be better. I am fine with almost any names. I am thinking: what about introducing @Hint(/@Inspection) and @Suggestion(/@Action - or probably some better name). Some of the @Hint(/@Inspection) attributes make little sense for "suggestions" (category, severity, suppressWarnings), so this would make it easier to understand. Any objections to that? > > [R02] I think the number of Hint.Severity levels is too limited. Most used > seems to be five levels. (1=most severe, 5=least severe) What would be the names? I wonder if the Hint.Severity shouldn't be changed the spi.editor.hints' Severity. Does not provide 5 levels, but we may need to extend that anyway in future, and reusing a standard would be better future-proof. > > [R03] It looks like the Hint.Severity is used to specify when the hint is > displayed (CURRENT_LINE_WARNING). It should be enough to look at the Hint.Kind > to see when/how to display. Actually, there are a few (not many) "hints" that use "CURRENT_LINE_WARNING" (e.g. Javadoc/Create Javadoc). I'm not sure if this can be dropped. I Hint.Severity.CURRENT_LINE_WARNING is undesirable, I can see two possibilities: -adding a separate attribute to the annotation saying that the warning should be on the current line only -if spi.editor.hints' Severity is used, one of them could be used to the current line warning (traditionally, Severity.HINT was used for similar behavior). (In reply to comment #8) > Another comment from the hint implementor point of view: > > [DB01] Provide a simple way to get the caret position from the hint method > (perhaps by adding HintContext.getCaretPosition()?) that would prevent the > current practice of calling > 'CaretAwareJavaSourceTaskFactory.getLastPosition(context.getInfo().getFileObject())' I have added HintContext.getCaretLocation() for that.
(In reply to comment #9) Thanks for the comments. > (In reply to comment #2) > > Getting a reasonable work dir will be a bit tricky without references to JUnit > > 3/NbJUnit. > > The typical idiom is to just to pass a File workDir parameter to the test > utility, e.g. HintTest.create. I would rather not do that - would make it unnecessary difficult to write junit4 tests. Moreover, I am still hoping the tests could run fully in memory sometime. I managed to resolve this somehow. > >> [JG02] The idiom of @Hint applied to a class of no particular type > > > > methods are cheaper than classes, which can save loading of some classes > > True, but by this argument we should never use anonymous inner classes at all, > and strive to implement as many interfaces as possible on every concrete class > we do have. In the longer run I would expect lambdas to make it cheap to > express closures: > > @Hint.Registration(...) public static Hint myhint = (context) -> {...}; > > > the signature is inflexible > > Just means you introduce MultiHint returning a List, and accept that as an > alternate target type. > > > in some cases, the actual semantics of the hint may require more than one > > triggered method to efficiently implement the hint at all. > > Unbalanced already has two separate @Hint registrations so I am confused by > this example. I guess you mean the occasional case where two or more triggers > should have exactly the same display name and description and other metadata? Yes. Both the Unbalanced hints use two triggers. > It is true that the @Hint.Registration params would need to be copied in such > cases. > > > the trouble is that users tend to store something in fields, and forget > > about them. This is of course problem even in the current proposal, but people > > are generally aware that storing something in a static field may not be a good > > idea, unlike storing something in an instance field. > > This is quite true, though it applies to all registration annotations applied > to classes, e.g. @ServiceProvider; I have considered making their processors > issue warnings if you attempt to add an instance field to such classes, since > such fields could introduce memory leaks and should either be removed or made > explicitly static. Great idea. > > > There is of course certain level of undiscoverability in the current proposal, > > but I would assume that can be mitigated by having an APISupport wizard. > > Yes, to some extent. I was more concerned about stylistic inconsistency, though > we are already in a bit of mess most notably with context-sensitive > @ActionRegistration, and alignment with the Java type system (e.g. finding all > subtypes of Hint). Not a blocking issue by any means, just something I wanted > to bring up as an alternative. > > > Suppress warnings keys need to be in the "declarative" part, because they may > > prevent the hint from instantiating. > > Makes sense, I was just guessing here. > > > The customizer is tricky: having it as a non-declarative method on the hint > > would mean that the hint classes would be loaded while browsing through the > > options. > > True. Not as important as avoid loading of hints while using the editor. > Possibly solvable by having an extension or mixin interface CustomizableHint, > and recording in the processor whether that is implemented or not. > > > missing Javadoc > > entry&missing check in the annotation processor: the customizer needs to get an > > instance of Preferences > > Sounds to me like you should define an interface expressing this contract, and > define the annotation attribute to be assignable to that interface. Ok. > > > Ideally, the annotation would contain directly the text (display > > name and description/tooltip), and would generate the key into the bundle > > automatically. That would make it much more convenient. Would require > > cooperation with NbBundleProcessor. > > -1 on such special treatment in just this annotation. If you want to introduce > an API change in LayerBuilder.File such as a bundlevalue variant which accepts > e.g. displayName="##Actual English Label", cooperating with NbBundleProcessor > and picking a key based on the annotated element, annotation (incl. nested!), > and annotation method, and use this new variant in all existing annotations, > and extend UseNbBundleMessages to suggest it, that would be a good API review > to file separately. In the meantime the standard idiom works well enough with > hints. I have adding displayName and description attributes to @Hint and displayName and tooltip to @BooleanOption, with the usual semantics. > > > I have nothing against such convenience methods (although I the usecases > > covered by the two listed methods are probably different). But these do not > > really belong into the Java hints. > > Agreed, can be a separate API change. > > > Moreover, a Java hint should no normally > > generate @SuppressWarnings itself - the infrastructure handles it > > automatically. > > In the FindBugs case, it cannot use java.lang.SuppressWarnings. FindBugs case is not covered by spi.java.hints. > > > [alternately] the java.hints' ErrorDescriptionFactory would take > > JavaFix rather than a Fix - would make the matter much clearer > > Yes, that would seem clearer. But then for true safety you should not use the > (generic editor's) ErrorDescription at all; introduce JavaErrorDescription and > require a @Hint to return it. Hm, may not be possible after all - plain editor fix may be OK in some cases. JavaFix is needed only for Inspect&Transform - if the hint is not NO_BATCH, any Fix is OK, and the Fix may do whatever it wants. This may be the case of EmbeddableEJBContainerHint. > > > Not currently (aside from actually doing the change in the file on the disk, > > which is clearly wrong) > > That is what I will do in UseNbBundleMessages for now, pending a better API, > e.g. another method in JavaFix such as > > protected void performAdditionalChanges() throws IOException {} I would rather add TransformationContext.recordChange(...), as that would allow to show the diff inside the refactoring window. Will prototype that for UseNbBundleMessages. > > Which reminds me of > > [JG16] Need trigger replacement for ErrorRule, e.g. for > SearchClassDependencyInRepo. Sorry, but unlikely to happen (for 7.2). There are too many tricky usecases in this, and the benefits are relatively limited. There seem to be only two ErrorRules outside java.hints and contrib/javahints, both of them appear to be bound to "cannot resolve symbol"-like errors, which may call for a specialized API anyway. > > (In reply to comment #5) > > the server has only 1.4 > > File a bug in www/builds & repositories please. > > > I have rewritten a test to show how this can be used: > > [...]/SizeEqualsZeroTest.java > > > > If this seems like a good direction > > Looks nice to me. I have changed the java.hints tests to the new style, will remove TestBase soon.
> I am fine with almost any names. I am thinking: what about introducing > @Hint(/@Inspection) and @Suggestion(/@Action - or probably some better name). > Some of the @Hint(/@Inspection) attributes make little sense for "suggestions" > (category, severity, suppressWarnings), so this would make it easier to > understand. Any objections to that? Personally I do not think having separate annotations would be a good choice, but this is just personal preference/feeling. > > [R02] I think the number of Hint.Severity levels is too limited. Most used > > seems to be five levels. (1=most severe, 5=least severe) > > What would be the names? I wonder if the Hint.Severity shouldn't be changed the > spi.editor.hints' Severity. Does not provide 5 levels, but we may need to > extend that anyway in future, and reusing a standard would be better > future-proof. I like the naming used by sonar; Blocker, Critical, Major, Minor, Info. PMD also has a severity of five, and they are mapped to three markers: PMD priority 1 -> severity error, priority high PMD priority 2 -> severity error, priority normal PMD priority 3 -> severity warning, priority high PMD priority 4 -> severity warning, priority normal PMD priority 5 -> severity information, priority normal > > [R03] It looks like the Hint.Severity is used to specify when the hint is > > displayed (CURRENT_LINE_WARNING). It should be enough to look at the Hint.Kind > > to see when/how to display. > > Actually, there are a few (not many) "hints" that use "CURRENT_LINE_WARNING" > (e.g. Javadoc/Create Javadoc). I'm not sure if this can be dropped. I > Hint.Severity.CURRENT_LINE_WARNING is undesirable, I can see two possibilities: > -adding a separate attribute to the annotation saying that the warning should > be on the current line only > -if spi.editor.hints' Severity is used, one of them could be used to the > current line warning (traditionally, Severity.HINT was used for similar > behavior). How about making this a configuration choice for the user. Give the option to show from a certain level only on the current line and do not show from another level. Like: Severity 5-3: Show always 2: Show on the current line 1: Do not show in the editor If the user can somehow influence the severity of hints, I think this would be the best option.
Thanks for the comments so far. I would like to merge the branch into trunk sometime soon - it would be perfect if I could get any further comments (or reminders about anything important I forgot) in a week. Comments: JG01: cleaned the test API, TestBase does not exist anymore JG12: tried to rephrase the requirement in arch.xml JG13: I have added TransformationContext.getResourceContent/getResourceOutput to handle non-Java changes. I changed the UseNbBundleMessages to use that, even though it still won't work in Inspect&Transform correctly, because when two independent modifications are being done to one ModifiersTree, doing WorkingCopy.rewrite(original, new) twice won't produce the correct result (the last mapping from original to new will overwrite any previous mappings). I am more inclined to fix that on java.source level rather than on java.hints level, even though correct solution is pending (can be workarounded in the fixes using Map<Weak, Weak>). JG14: I plan to move the org.netbeans.spi.java.hints.matching into org.netbeans.api.java.source.matching/java.source module, if there are no objections R01: converted Hint.Kind.HINT->INSPECTION, and Hint.Kind.SUGGESTION->ACTION R02&R03: I dropped Hint.Severity, the defaults can be specified using the spi.editor.hints.Severity now, Severity.HINT is documented to typically produce only current-line warning (the API does not (IMO) currently prevent addition of an GUI option for that, although it seems like too big complication of the user interface for the gains). If we need more severities, we should add them to the base API.
JG13 - sounds OK; prototypes #f5e977b5c0e1 looks straightforward enough. Will this be supported in the test framework as well?
JG13 cont'd - unclear how to handle any IOException resulting from getResourceContent, getResourceOutput, or operations on these streams. performRewrite does not permit the IOE to be passed on. Also there is no longer a way to save modifications to the [Java]DataObject after the hint is applied, since it the calling code which actually commits the transaction. This is a problem for UseNbBundleMessages, since the Java parser does not recompute the list of methods in Bundle.java until the modified source is saved, so the newly introduced usage will generally be marked as an error - confusing to the user. Assuming that underlying bug is not easily fixed, would rather just keep the pre-branch workaround of saving the file after the hint is applied. This would require some kind of overridable method like cleanup(TransformationContext) called if and when changes are committed. [JG17] Inspect & Transform > Use @NbBundle.Messages > Preview shows "Missing Description" in the left pane for each line changed. [JG18] I&T on UNBM on PluginIndexManager.java proposes to modify getHtmlDetails, including replacing several calls to NbBundle.getMessage with distinct keys, yet only the last key appears in the generated annotation. I guess this is what you were mentioning earlier in response to JG13, but it seems like a distinct issue and I wanted to make sure it was recorded and tracked; this would be a blocker for the UNBM rewrite, I think, and could well break other hints too. [JG19] The trunk version of UNBM suffers from a random bug that sometimes applying the fix will remove the key from the Bundle.properties but not modify the source file at all. (Leaving the recalculated hint to just issue a warning that the key is missing; workaround is to revert the line removal from the bundle and try again.) Seems to happen mostly when you are applying the hint a number of times in rapid succession - perhaps the index not getting refreshed in time for something to happen, etc. I had thought this was just a symptom of bug #201871. But I just observed the same bug in the branch, in which that bug should as I understand it be obsolete, since the hint is no longer managing any state between calculation of the hint and its application. Does this mean that the hint infrastructure is suffering from a similar race condition, or am I missing something wrong with this hint's implementation?
Still wondering about test support for changes to non-Java resources (JG13). I would imagine this would be a fairly common requirement, e.g. a hint to "inline" a call to This.class.getResourceAsStream("something.txt") with the contents of the resource. But writing these without a test framework is a bit painful. [JG20] Buglets for the Java Hint wizard: - fails to disable Finish when various fields are not filled in - does not define package for new files (hotfix in prototypes #f16882c820ec)
[JG21] HintTest does not allow you to test code which refers to library classes not in the JRE. (It looks like the extraClassPath is there for this purpose, but it is never written.) That would seem to make it useless for domain-specific hints. You could I guess set compilable=false but your parse tree will not be properly attributed in general. Recommend something like public HintTest classpath(URL... entries); (with the suggestion to use FileUtil.urlForArchiveOrDir), or File... (doing it for you), or String (as in ClassPathSupport.createClassPath(String)). Note that using ${java.class.path} would be convenient for the common case that the library in question is a static dependency of the hint code, but using this actual system property is a bad idea as it assumes that tests are being run in a certain way (will fail e.g. under Surefire in unforked mode). A potentially more useful signature for this case is public HintTest classpath(Class<?>... libraryClasses); where the entries would then be libraryClass.getProtectionDomain().getCodeSource().getLocation(), processed with FileUtil.getArchiveRoot if necessary.
(In reply to comment #16) > JG13 cont'd - unclear how to handle any IOException resulting from > getResourceContent, getResourceOutput, or operations on these streams. > performRewrite does not permit the IOE to be passed on. Hm, lets add "throws Exception" to JavFix.performRewrite. > > Also there is no longer a way to save modifications to the [Java]DataObject > after the hint is applied, since it the calling code which actually commits the > transaction. This is a problem for UseNbBundleMessages, since the Java parser > does not recompute the list of methods in Bundle.java until the modified source > is saved, so the newly introduced usage will generally be marked as an error - > confusing to the user. Assuming that underlying bug is not easily fixed, would > rather just keep the pre-branch workaround of saving the file after the hint is > applied. This would require some kind of overridable method like > cleanup(TransformationContext) called if and when changes are committed. Generally, the fixes should not save files when invoked from the editor. As the UNBM is currently the only hint that needs to do that, I would rather look whether there is some other solution than to extend the API. > > > [JG17] Inspect & Transform > Use @NbBundle.Messages > Preview shows "Missing > Description" in the left pane for each line changed. The same in trunk, as far as I can tell - unlikely to be fixed before feature freeze because of time constraints, but should be fixed for 7.2. > > > [JG18] I&T on UNBM on PluginIndexManager.java proposes to modify > getHtmlDetails, including replacing several calls to NbBundle.getMessage with > distinct keys, yet only the last key appears in the generated annotation. > > I guess this is what you were mentioning earlier in response to JG13, but it > seems like a distinct issue and I wanted to make sure it was recorded and > tracked; this would be a blocker for the UNBM rewrite, I think, and could well > break other hints too. Yes thats it. It actually affects some other hints and is being workarounded there. We may need something like WorkingCopy.targetTree(Tree original) to resolve this properly. > > > [JG19] The trunk version of UNBM suffers from a random bug that sometimes > applying the fix will remove the key from the Bundle.properties but not modify > the source file at all. (Leaving the recalculated hint to just issue a warning > that the key is missing; workaround is to revert the line removal from the > bundle and try again.) Seems to happen mostly when you are applying the hint a > number of times in rapid succession - perhaps the index not getting refreshed > in time for something to happen, etc. > > I had thought this was just a symptom of bug #201871. But I just observed the > same bug in the branch, in which that bug should as I understand it be > obsolete, since the hint is no longer managing any state between calculation of > the hint and its application. Does this mean that the hint infrastructure is > suffering from a similar race condition, or am I missing something wrong with > this hint's implementation? Sorry, but hard to guess without actually trying and repeatedly adding and tuning logs. (In reply to comment #17) > Still wondering about test support for changes to non-Java resources (JG13). I > would imagine this would be a fairly common requirement, e.g. a hint to > "inline" a call to This.class.getResourceAsStream("something.txt") with the > contents of the resource. But writing these without a test framework is a bit > painful. Sorry, I forgot to write about that - this is supported by the test infra, I have added a test that shows that changes to non-Java resources can be tested: http://hg.netbeans.org/prototypes/file/590032453528/java.hints.test/test/unit/src/org/netbeans/modules/java/hints/test/api/HintTestTest.java > > > [JG20] Buglets for the Java Hint wizard: > > - fails to disable Finish when various fields are not filled in > - does not define package for new files (hotfix in prototypes #f16882c820ec) Thanks!
(In reply to comment #18) > [JG21] HintTest does not allow you to test code which refers to library classes > not in the JRE. (It looks like the extraClassPath is there for this purpose, > but it is never written.) > > That would seem to make it useless for domain-specific hints. You could I guess > set compilable=false but your parse tree will not be properly attributed in > general. > > Recommend something like > > public HintTest classpath(URL... entries); Yes, I will add it to the API. We can add more variants in the future. > > (with the suggestion to use FileUtil.urlForArchiveOrDir), or File... (doing it > for you), or String (as in ClassPathSupport.createClassPath(String)). > > Note that using ${java.class.path} would be convenient for the common case that > the library in question is a static dependency of the hint code, but using this > actual system property is a bad idea as it assumes that tests are being run in > a certain way (will fail e.g. under Surefire in unforked mode). A potentially > more useful signature for this case is > > public HintTest classpath(Class<?>... libraryClasses); > > where the entries would then be > libraryClass.getProtectionDomain().getCodeSource().getLocation(), processed > with FileUtil.getArchiveRoot if necessary. The problem is that the code of the hints generally does not need to depend on the library for which they are written. So classpath(Class...) may be a convenience method, but a general variant taking URL, File or FileObject is required. Thanks.
[JG22] (continuing JG05) TriggerPattern.value claims that it may represent "a Java field, method or class" without giving any concrete examples. After some trial and error I got it to match "$modifiers$ class $c extends some.SuperType {$body$;}" but this is less than obvious, especially the odd ';' after the body (which would not in fact appear directly inside a class body!).
(In reply to comment #19) > lets add "throws Exception" to JavaFix.performRewrite. Sounds fine. Do not forget to update HintTestTest accordingly. > fixes should not save files when invoked from the editor. As the > UNBM is currently the only hint that needs to do that, I would rather look > whether there is some other solution than to extend the API. OK. >> Still wondering about test support for changes to non-Java resources (JG13). > > this is supported by the test infra Great, now as soon as JG21 is fixed it ought to be possible to test UNBM. (In reply to comment #20) > The problem is that the code of the hints generally does not need to depend on > the library for which they are written. So classpath(Class...) may be a > convenience method, but a general variant taking URL, File or FileObject is > required. Yes of course. I would consider URL... to the definitive signature, and File... and/or Class... to be optional conveniences only. I do not see any purpose for a FileObject... overload.
Added "throws Exception" to JavaFix.performRewrite (JG13): http://hg.netbeans.org/prototypes/rev/29733aa714b2 and added classpath method to HintTest (JG21): http://hg.netbeans.org/prototypes/rev/fccc65341589
Nice, I finally managed to write UseNbBundleMessagesTest. [JG23] Regarding HintTest's use of NbBundle.setBranding("test") and the template's use of that: this seems unnecessary, and in fact I wrote UNBMT comfortably without it: assertWarnings("3:41-3:51:warning:" + UseNbBundleMessages_error_text()) is not really less readable than assertWarnings("3:41-3:51:warning:UseNbBundleMessages.error_text") but permits code completion when writing the test, does not require a separate Bundle_test.properties, and adds an extra measure of compile-time safety to the combination of hint + test: if you remove a message from the hint which is still checked by the test, the project will be marked with an error badge; you cannot forget to clean up an entry from Bundle_test.properties that is no longer used by the test; etc. In particular, for a user running the Java Hint wizard, this would reduce the number of files they need to pay attention to from three to two.
Re JG23: that's doable, of course: http://hg.netbeans.org/prototypes/rev/170c11002a18 But I find it somewhat less convenient (the typical workflow it to copy the actual test output into the test, not to use the code completion), and somewhat more dangerous (if the user does not convert the actual test output to use Bundle.ERR_whatever, the test will not be stable). Moreover, the test branding must stay, as not using it would practically force use of @NbBundle.Messages, which is undesirable.
(In reply to comment #25) > the typical workflow it to copy the > actual test output into the test, not to use the code completion I see your point. I assumed that you would write the test as you expect the hint to be displayed, putting in dummy line and column numbers and fixing them after the first failure. At least that is what I did. There are probably different workflows depending on whether you are primarily testing 1. A single warning message with complex conditions for when it appears; and perhaps also a single fix, with complex conditions for what it produces. or 2. A hint which can display several warnings depending on various conditions, and perhaps one or more fixes, all having their own messages. > if the user does not convert the actual test output to use > Bundle.ERR_whatever, the test will not be stable Assuming by "stable" you mean "would not fail when run in an unexpected locale", I think this could be guaranteed merely by setting the default locale in the test rather than branding: either they are using localized messages and use Bundle.* methods in test output (following the example given in the template); or they use localized messages but hardcode the English strings in the test, which will be guaranteed to be used; or they are not localizing at all, in which case the test will just follow the hint. (Of course setting global state of any kind - Locale.default, NbBundle.branding - in a unit test is poor practice since it prevents parallel test execution, e.g. in Surefire. But NB tests are already chronic violators of this rule since the platform does not use dependency injection.) > the test branding must stay No problem with that. > not using it would practically force use of @NbBundle.Messages No, but it would force use of NbBundle.getMessage(TheHint.class, "the_key") which is ugly.
Merged into jet-main: http://hg.netbeans.org/jet-main/rev/9edee0315cb6 Will close after all pending tasks are finished.
BTW: build of contrib/javahints (part of nbms-and-javadoc) will fail after this is merged into main-silver, will fix that when that happens.
Integrated into 'main-golden', will be available in build *201203100400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/951acc290dd1 User: Jan Lahoda <jlahoda@netbeans.org> Log: #207577.JG03: adding @Hint.displayName/description and BooleanOption.displayName/tooltip
Surely this issue can be closed as FIXED.