Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 353472

Summary: [override method] generating a method override should copy annotations
Product: [Eclipse Project] JDT Reporter: Stephan Herrmann <stephan.herrmann>
Component: UIAssignee: Markus Keller <markus.kell.r>
Status: VERIFIED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: daniel_megert, markus.kell.r
Version: 3.7Keywords: polish
Target Milestone: 3.8 M7   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 353474    
Bug Blocks:    

Description Stephan Herrmann CLA 2011-07-31 15:21:36 EDT 
While working on null annotations in method signatures (bug 186342)
I came to wish that completion in a case like this:

class A {
	public @NonNull Object foo(@Nullable Object input) { return this; }
}
class B extends A {
	| // Ctrl-space here
}

when selecting proposal "override foo" should create s.t. like:
	
	@Override
	public @NonNull Object foo(@Nullable Object input) {
		// TODO Auto-generated method stub
		return super.foo(input);
	}

All these annotations should be seen as part of the method specification
and not copying them will probably render the override incompatible with
the inherited method.
Comment 1 Stephan Herrmann CLA 2011-07-31 15:39:16 EDT 
I also played with implementing this and found:

The change could be made either in OverrideCompletionProposal or generally
in StubUtility2.createImplementationStub(..)
Should this feature actually also work for other clients of
createImplementationStub?

Implementing this for marker annotations is straight forward, I haven't
tried to include any member value pairs.

Currently parameter annotations may get lost on the way because they are not
converted by {Source,Binary}TypeConverter, but this can be added trivially.
Comment 2 Dani Megert CLA 2011-08-02 03:29:39 EDT 
Copy all annotations is not always desired, e.g. I would not want @SuppressWarnings("all") to be copied.
Comment 3 Stephan Herrmann CLA 2011-08-02 04:21:57 EDT 
(In reply to comment #2)
> Copy all annotations is not always desired, e.g. I would not want
> @SuppressWarnings("all") to be copied.

Mh, saying this should be done for all possible annotation types is
indeed an over-generalization.

For your example I'd probably say manually removing the annotation is
easier than adding all annotations where copying is the right thing.

The distinction I see: some annotations are part of the method specification
(like null annotations) whereas other annotations affect (also) the
implementation (like @SuppressWarnings).

In trying to find a way to distinguish SPEC annotations from IMPL
annotations I wonder if @Retention(SOURCE) would be a good criterion
to recognize IMPL annotations?

Would it make more sense to keep a (configurable) positive list of
annotations we want to copy?

What do you think?
Comment 4 Markus Keller CLA 2011-08-02 16:12:59 EDT 
> Would it make more sense to keep a (configurable) positive list of
> annotations we want to copy?

That's the conclusion I also reached in bug 350396 comment 6.

But @Retention(SOURCE) sounds even better. I think we should try that first (without any configurable options) and only resort to the white or black list if we have a real need.
Comment 5 Dani Megert CLA 2011-08-03 02:18:19 EDT 
> That's the conclusion I also reached in bug 350396 comment 6.
> 
> But @Retention(SOURCE) sounds even better. I think we should try that first
> (without any configurable options) and only resort to the white or black list
> if we have a real need.
+1.
Comment 6 Markus Keller CLA 2011-08-10 03:08:48 EDT 
Raksha, could you please have a look? We should use the same implementation also for Extract Interface (bug 350396) and probably more places where we produce overriding methods (Pull Up / Push Down).
Comment 7 Stephan Herrmann CLA 2012-01-18 07:34:18 EST 
To clarify (my initial statement may have been a bit dubious), do we agree that @Retention(SOURCE) describes the weaker kind of annotations that should *not* be copied, whereas all others *will* be copied?
Comment 8 Markus Keller CLA 2012-01-18 08:33:02 EST 
(In reply to comment #7)
> To clarify (my initial statement may have been a bit dubious), do we agree that
> @Retention(SOURCE) describes the weaker kind of annotations that should *not*
> be copied, whereas all others *will* be copied?

Yes.

With the justification that SOURCE annotations are usually only interesting for the compiler, but CLASS and RUNTIME annotations form a contract (e.g. modify the type of an element).
Comment 10 Dani Megert CLA 2012-05-01 08:22:27 EDT 
Verified in I20120430-2000.
Comment 11 Markus Keller CLA 2012-10-25 14:17:31 EDT 
The automatic copying has been reverted for most cases, see bug 386410.