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.

Bug 208570 - Utility methods to add attribute values to an annotation
Summary: Utility methods to add attribute values to an annotation
Status: RESOLVED FIXED
Alias: None
Product: java
Classification: Unclassified
Component: Source (show other bugs)
Version: 7.2
Hardware: PC Linux
: P3 normal (vote)
Assignee: Jan Lahoda
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 211037
Blocks:
  Show dependency tree
 
Reported: 2012-02-17 14:25 UTC by Jan Lahoda
Modified: 2012-04-11 17:45 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Proposed API change. (20.98 KB, patch)
2012-02-17 14:25 UTC, Jan Lahoda
Details | Diff
Updated patch. (23.39 KB, patch)
2012-02-22 14:48 UTC, Jan Lahoda
Details | Diff
Updated patch. (36.64 KB, patch)
2012-03-12 08:35 UTC, Jan Lahoda
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Lahoda 2012-02-17 14:25:15 UTC
Created attachment 115878 [details]
Proposed API change.

I propose to add two utility methods into java.source/GeneratorUtilities that would allow to augment an annotation's attribute values with given values, creating the annotation if it does not exist. Useful e.g. for adding suppress warnings keys to @SuppressWarnings. See #207577.JG11 (handles only the @SuppressWarnings/@NbBundle.Messages case).
Comment 1 Jesse Glick 2012-02-21 22:41:38 UTC
(In reply to comment #0)
> See #207577.JG11

I.e. hyperlinked: bug #207577 comment #1
Comment 2 Jesse Glick 2012-02-21 23:05:24 UTC
[JG01] <issue number="999999"/> needs to be fixed.


[JG02] appendToAnnotationValue(CompilationUnitTree, ...) should probably explain that it is expected to be used on a package-info.java.


[JG03] Would be polite to enumerate what the useful kinds of ExpressionTree you might add actually are, since it is hard to guess - and nowhere documented that I know of. LiteralTree obviously for primitives, but also what tree kinds are expected for strings, classes, enums, arrays, nested annotations.

Nicest would be to have another utility method (Object) -> ExpressionTree that would accept as input any primitive wrapper, String, Class, Enum, recursively Object[] (replacing singleton arrays with their element), recursively Map<String,Object> for annotations (where the null key is the FQN), or ExpressionTree itself (for use within recursion).


[JG04] Bogus patch to libs.findbugs.


[JG05] Would be good to try using aTAV from addMessage in UseNbBundleMessages to make sure there is more than one successful use case.

Also Hinter.Context.addAnnotation and CreatedModifiedFiles.PackageInfo could probably use aTAV, though these would also benefit from JG03.
Comment 3 Jan Lahoda 2012-02-22 14:48:22 UTC
Created attachment 116029 [details]
Updated patch.
Comment 4 Jan Lahoda 2012-02-22 14:53:05 UTC
Thanks for the comments - I have attached an updated patch.

(In reply to comment #2)
> [JG01] <issue number="999999"/> needs to be fixed.

Done.

> 
> 
> [JG02] appendToAnnotationValue(CompilationUnitTree, ...) should probably
> explain that it is expected to be used on a package-info.java.

Done.

> 
> 
> [JG03] Would be polite to enumerate what the useful kinds of ExpressionTree you
> might add actually are, since it is hard to guess - and nowhere documented that
> I know of. LiteralTree obviously for primitives, but also what tree kinds are
> expected for strings, classes, enums, arrays, nested annotations.

Done.

> 
> Nicest would be to have another utility method (Object) -> ExpressionTree that
> would accept as input any primitive wrapper, String, Class, Enum, recursively
> Object[] (replacing singleton arrays with their element), recursively
> Map<String,Object> for annotations (where the null key is the FQN), or
> ExpressionTree itself (for use within recursion).

I disagree with that - it is complex to explain, provides no type checking and (in case of Class and Enum attributes at least) mixes runtime with "design" time, which is not something java.source should encourage.

> 
> 
> [JG04] Bogus patch to libs.findbugs.

Fixed.

> 
> 
> [JG05] Would be good to try using aTAV from addMessage in UseNbBundleMessages
> to make sure there is more than one successful use case.

Actually, I started with that, but then I realized there are not tests for UNBM, which means there is no real way to evaluate whether the rewrite was successful or not, except of manual testing of unknown corner cases.
Comment 5 Jesse Glick 2012-02-22 15:27:04 UTC
(In reply to comment #4)
> I have attached an updated patch

BTW remember to mark previous patches "obsolete" in BZ.

> provides no type checking

Nor does the current API, really - you can pass in all sorts of ExpressionTree's and not find until you try it that they are syntactically illegal in that position.

Using separate methods would solve this intuitively enough:

public final class AnnotationMaker {
  public AnnotationMaker(TreeMaker make) {...}
  public ExpressionTree forInt(int value) {...}
  public ExpressionTree forString(String value) {...}
  public ExpressionTree forClass(Class<?> value) {...}
  public ExpressionTree forClass(TypeElement value) {...}
  public ExpressionTree forArray(ExpressionTree... arrayValues) {...}
  public ExpressionTree forAnnotation(Class<?> anno, Map<String,? extends ExpressionTree> values) {...}
  // etc.
}

though it is unusable for cases like CMF.PI where you want to pass in the structure of the annotation before you even have a WorkingCopy.

> mixes runtime with "design" time

Of course, but this is often convenient, which is why even 269 does so in a few places. But OK, can keep such helper methods elsewhere private I guess.

JG03 nit - probably you meant <th> rather than <td> in the first row? Also typo "and and".

No need to repeat extensive Javadoc for appendToAnnotationValue(CompilationUnitTree, ...); just @see #appendToAnnotationValue(ModifiersTree, ...) for all details. Or just use org.openide.util.Union2 (pity this is not in java.util!) so you only need to expose one method.

> there are not tests for UNBM

Right; I had no idea what infrastructure to use. Bug #207577 may help with that.
Comment 6 Jan Lahoda 2012-03-12 08:35:12 UTC
Created attachment 116575 [details]
Updated patch.

The last version of the patch, I would like to integrate tomorrow, unless there are strong objections. It also updated UseNbBundleMessages, verified by its tests.
Comment 7 Jesse Glick 2012-03-14 23:08:18 UTC
Looks OK.
Comment 9 Quality Engineering 2012-03-17 10:41:25 UTC
Integrated into 'main-golden', will be available in build *201203170400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/672210bc7863
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #208570: Utility methods to add attribute values to an annotation