Bugzilla – Full Text Bug Listing
| Summary: | Type use annotations should not be attached to constructor elements | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Jay Arthanareeswaran <jarthana> |
| Component: | APT | Assignee: | Jay Arthanareeswaran <jarthana> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | stephan.herrmann |
| Version: | 4.5.1 | ||
| Target Milestone: | 4.6 M6 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| See Also: |
https://git.eclipse.org/r/67022 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=c5316735ee17d65960d7fcf7b16e0e8ec7cd9b5d |
||
| Whiteboard: | |||
Here's some more input from Stephan from en email exchange:
... looking at type annotations mainly from the perspective of null annotations:
For null annotations, annotating a constructor is useless, because by construction any constructor C() has the type '@NonNull C'.
For other type annotations the situation may be different, so, e.g., '@Raw C() {}' could say that this constructor produces an instance that is not yet fully initialized (not to confuse this use of @Raw with raw types ...).
In this light, I believe we should indeed apply copySE8AnnotationsToType
to constructors, too.
Additionally, I should add a warning for
@Nullable C() { }
mentioning that the annotation has no effect in this position, as we'd never want to consider a newly created instances as can-be-null.
...
BTW: another spec quote (from 9.7.4):
"If an annotation appears before a constructor declaration and is deemed to apply to the type which is closest to the annotation, that type is the type of the newly constructed object. The type of the newly constructed object is the fully qualified name of the type immediately enclosing the constructor declaration. Within that fully qualified name, the annotation applies to the simple type name indicated by the constructor declaration."
IMHO this is even clearer than what you quoted, as it says "*the type* of the newly constructed object".
IOW: with
@Raw C() {}
the type of
new C()
is
@Raw C
Okay, just to confirm I got the requirement right, here's my understanding of what needs to be done: 1. The type-use only annotations should be removed from the binding of the constructor. 2. The constructors are moved to the *AllocationExpression#type or (and eventually to) #resolvedType. Is that it or is there more to it? And I don't yet know where the annotations can be held for future uses if they are removed from method binding! The problem is: we don't really have a field in MethodBinding that cleanly captures "the type of this method" for constructors, but we could try to move the type-use annotation on the constructor to the MethodBinding#declaringClass, so that this annotated type will then automatically be transported into *AllocationExpression#type#binding and #resolvedType.
This would be a litmus test for our use of TypeBinding.equalsEquals():
ReferenceBinding "C"
- contains a MethodBinding "<init>()"
- has a declaringClass "@Raw C"
and still TypeBinding.equals("C", "@Raw C") holds true.
An alternative would be to start using MethodBinding#returnType even for constructors, but that sounds like a much deeper ripple.
(plus a declaration AND type-use annotation must be present in both locations)
(In reply to Stephan Herrmann from comment #4) > The problem is: we don't really have a field in MethodBinding that cleanly > captures "the type of this method" for constructors, but we could try to > move the type-use annotation on the constructor to the > MethodBinding#declaringClass, so that this annotated type will then > automatically be transported into *AllocationExpression#type#binding and > #resolvedType. I thought that would work except that MethodBinding#declaringClass is ReferenceBinding. Although, I can see the annotations being applied on the allocation's type auomatically, but we are breaking lot of tests that don't expect these annotations on the type. (In reply to Jay Arthanareeswaran from comment #5) > I thought that would work except that MethodBinding#declaringClass is > ReferenceBinding. Although, I can see the annotations being applied on the > allocation's type auomatically, but we are breaking lot of tests that don't > expect these annotations on the type. Just thinking aloud, how about just creating a new field typeAnnotations in MethodBinding just for constructors? And this can later be used to apply to the AllocationExpression#resolvedType. (In reply to Jay Arthanareeswaran from comment #5) > (In reply to Stephan Herrmann from comment #4) > > The problem is: we don't really have a field in MethodBinding that cleanly > > captures "the type of this method" for constructors, but we could try to > > move the type-use annotation on the constructor to the > > MethodBinding#declaringClass, so that this annotated type will then > > automatically be transported into *AllocationExpression#type#binding and > > #resolvedType. > > I thought that would work except that MethodBinding#declaringClass is > ReferenceBinding. I'm not sure why you are surprised :) Did you add the type annotations into the existing ReferenceBinding or did you create an annotated copy and replace #declaringClass? > Although, I can see the annotations being applied on the > allocation's type auomatically, but we are breaking lot of tests that don't > expect these annotations on the type. Can you give an example? If these comments don't help to make this approach fly, then, yes, a new field typeAnnotations would certainly work. (In reply to Stephan Herrmann from comment #7) > I'm not sure why you are surprised :) > Did you add the type annotations into the existing ReferenceBinding or did > you create an annotated copy and replace #declaringClass? I created a copy, like this: method.declaringClass = (ReferenceBinding) scope.environment().createAnnotatedType(method.declaringClass, se8Annotations); > > Although, I can see the annotations being applied on the > > allocation's type auomatically, but we are breaking lot of tests that don't > > expect these annotations on the type. > > Can you give an example? I admit I didn't investigate these in depth, but there are a few failing with this exception: Caused by: java.lang.IllegalStateException at org.eclipse.jdt.internal.compiler.lookup.NestedTypeBinding.syntheticEnclosingInstanceTypes(NestedTypeBinding.java:239) at org.eclipse.jdt.internal.compiler.lookup.MethodBinding.signature(MethodBinding.java:1128) at org.eclipse.jdt.internal.compiler.ClassFile.generateMethodInfoHeader(ClassFile.java:3446) at org.eclipse.jdt.internal.compiler.ClassFile.generateMethodInfoHeader(ClassFile.java:3412) at org.eclipse.jdt.internal.compiler.ast.ConstructorDeclaration.internalGenerateCode(ConstructorDeclaration.java:361) at org.eclipse.jdt.internal.compiler.ast.ConstructorDeclaration.generateCode(ConstructorDeclaration.java:293) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:567) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:629) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:560) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:629) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:560) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:636) at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:371) at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:1029) at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:197) at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:194) at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:259) at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:579) at org.eclipse.jdt.internal.core.CompilationUnit.makeConsistent(CompilationUnit.java:1085) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:171) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:90) at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:724) at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:790) at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1250) at org.eclipse.jdt.core.tests.dom.AbstractASTTests.buildASTs(AbstractASTTests.java:454) at org.eclipse.jdt.core.tests.dom.AbstractASTTests.buildASTs(AbstractASTTests.java:491) at org.eclipse.jdt.core.tests.dom.AbstractASTTests.buildAST(AbstractASTTests.java:368) at org.eclipse.jdt.core.tests.dom.AbstractASTTests.buildAST(AbstractASTTests.java:359) at org.eclipse.jdt.core.tests.dom.AbstractASTTests.buildAST(AbstractASTTests.java:346) at org.eclipse.jdt.core.tests.dom.TypeBindingTests308.test425216a(TypeBindingTests308.java:2376) New Gerrit change created: https://git.eclipse.org/r/67022 Gerrit change https://git.eclipse.org/r/67022 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=c5316735ee17d65960d7fcf7b16e0e8ec7cd9b5d (In reply to Eclipse Genie from comment #10) > Gerrit change https://git.eclipse.org/r/67022 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=c5316735ee17d65960d7fcf7b16e0e8ec7cd9b5d I went with the setTypeAnnotation on MethodBinding simply because it didn't produce any failures in the existing tests. I have applied the fix to QAE as well and released. . Verified for 4.6 M6 using I20160315-0800 build |
Here's a test code that annotates TYPE_USE annotations on a constructor declaration: @Target(ElementType.TYPE_USE) @interface Type { } public class X { @Type public X(){} X _x_ = new X(); } ExecutableElement constr = ...; // Represents the constructor element. annotationMirrors = constr.getAnnotationMirrors(); The result contains all the type use annotations, which is wrong. The specification says: "A type annotation is permitted in front of a constructor declaration, where declaration annotations are already permitted. In that location, a type annotation is treated as applying to the constructed object (which is different than the receiver, if any, of the constructor). " Looking at the code, I don't see any effort is being made to move the SE8 annotations to the type of the node (ASTNode#copySE8AnnotationsToType()).