Skip to content

Improve error messages for annotation methods on synthetic TypeVariables #7974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Improve error messages for annotation methods on synthetic TypeVariables #7974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aaf2c8e
Add support for annotations in `TypeVariable` handling.
lmcrean Sep 7, 2025
065f25e
Merge branch 'master' of https://github.com/google/guava into lmcrean-2
lmcrean Sep 14, 2025
6c1fe9b
Clarify annotation handling in `TypeResolver` and `Types` classes. Up…
lmcrean Sep 14, 2025
28e3793
Refactor `TypeResolver` to remove unnecessary parameter from `newArti…
lmcrean Sep 14, 2025
0c0d483
Remove unused `newArtificialTypeVariable` method in `Types` class to …
lmcrean Sep 14, 2025
3c4ea15
Enhance exception handling for annotation methods in `Types` class. A…
lmcrean Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions guava/src/com/google/common/reflect/TypeResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,12 @@ Type resolveInternal(TypeVariable<?> var, TypeTable forDependants) {
* by us. And that equality is guaranteed to hold because it doesn't involve the JDK
* TypeVariable implementation at all.
*
* TODO: b/147144588 - But what about when the TypeVariable has annotations? Our
* implementation currently doesn't support annotations _at all_. It could at least be made
* to respond to queries about annotations by returning null/empty, but are there situations
* in which it should return something else?
* NOTE: b/147144588 - Custom TypeVariables created by Guava do not preserve annotations.
* This is intentional. The semantics of annotation handling during type resolution are
* unclear and have changed across Java versions. Until there's a clear specification for
* what annotations should mean on resolved TypeVariables with modified bounds, annotation
* methods will throw UnsupportedOperationException. Frameworks requiring annotation
* preservation should use the original TypeVariable when bounds haven't changed.
*/
if (Types.NativeTypeVariableEquals.NATIVE_TYPE_VARIABLE_ONLY
&& Arrays.equals(bounds, resolvedBounds)) {
Expand Down
14 changes: 13 additions & 1 deletion guava/src/com/google/common/reflect/Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,19 @@ private static final class TypeVariableInvocationHandler implements InvocationHa
String methodName = method.getName();
Method typeVariableMethod = typeVariableMethods.get(methodName);
if (typeVariableMethod == null) {
throw new UnsupportedOperationException(methodName);
// Provide helpful error message for annotation-related methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some others at https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/reflect/AnnotatedElement.html. Some are default methods, whose behavior I'd have to check under Proxy, but I'd imagine we'd want to cover at least getAnnotations() and possibly the default ones, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. While the existing startsWith checks technically already cover getAnnotations() and getDeclaredAnnotations(), I've added them explicitly for clarity. This mainly makes it crystal clear that we're handling these specific methods, and serves as defensive programming if one of the other startsWith approaches ever changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks, clearly I missed the detail of startsWith entirely. Maybe we should be doing methodName.contains("Annotat") or something :) I'll see if an internal reviewer has an opinion when I import this now.

if (methodName.equals("getAnnotatedBounds")
|| methodName.startsWith("getAnnotation")
|| methodName.startsWith("getDeclaredAnnotation")
|| methodName.equals("isAnnotationPresent")
|| methodName.equals("getAnnotations")
|| methodName.equals("getDeclaredAnnotations")) {
throw new UnsupportedOperationException(
"Annotation methods are not supported on synthetic TypeVariables created during type "
+ "resolution. The semantics of annotations on resolved types with modified bounds are "
+ "undefined. Use the original TypeVariable for annotation access. See b/147144588.");
}
throw new UnsupportedOperationException(methodName); // Keep original behavior for other methods
} else {
try {
return typeVariableMethod.invoke(typeVariableImpl, args);
Expand Down