Bug 526335 - [9][hovering] Deprecation warning should show the new 'since' deprecation value
Summary: [9][hovering] Deprecation warning should show the new 'since' deprecation value
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.7.1a   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.7.3   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 530600
  Show dependency tree
 
Reported: 2017-10-21 09:03 EDT by Dani Megert CLA
Modified: 2018-03-06 10:34 EST (History)
6 users (show)

See Also:


Attachments
Attachment showing the since value on usage of deprecated function upon hover (9.88 KB, image/png)
2017-11-06 04:03 EST, Kalyan Prasad Tatavarthi CLA
no flags Details
Attachment showing the since value on deprecated function upon hover (7.17 KB, image/png)
2017-11-06 04:10 EST, Kalyan Prasad Tatavarthi CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2017-10-21 09:03:03 EDT
[hovering] Hover on client code should show the new 'since' deprecation value.
Comment 1 Dani Megert CLA 2017-10-21 09:03:27 EDT
Example:

	@Deprecated(since="2.0")
	public void dontUseMe() {
		
	}
Comment 2 Kalyan Prasad Tatavarthi CLA 2017-11-06 04:03:10 EST
Created attachment 271335 [details]
Attachment showing the since value on usage of deprecated function upon hover
Comment 3 Kalyan Prasad Tatavarthi CLA 2017-11-06 04:04:21 EST
I have not been able to reproduce this issue. Attached a screenshot showing the since value on a deprecated function. If you still find the issue, please provide steps to reproduce this issue
Comment 4 Kalyan Prasad Tatavarthi CLA 2017-11-06 04:10:14 EST
Created attachment 271336 [details]
Attachment showing the since value on deprecated function upon hover
Comment 5 Noopur Gupta CLA 2017-11-07 02:04:59 EST
I cannot reproduce the issue with latest master and R4_7.
Comment 6 Dani Megert CLA 2017-11-07 06:55:36 EST
Don't suppress the warning and you will see the problem.

So, most likely something that JDT Core needs to add to the warning message.
Comment 7 Noopur Gupta CLA 2017-11-07 07:44:28 EST
The bug is about the message from compiler problem (not the Javadoc hover).
Comment 8 Stephan Herrmann CLA 2017-11-07 09:43:51 EST
Changed the bug title.

Note that this requires new API (IProblem).
Comment 9 Dani Megert CLA 2017-11-07 09:50:22 EST
(In reply to Stephan Herrmann from comment #8)
> Changed the bug title.
> 
> Note that this requires new API (IProblem).

Why is that? The compiler already recognizes @Deprecated(forRemoval=true) and issues a different message. Why can't it do it for the @since tag without adding new API?
Comment 10 Stephan Herrmann CLA 2017-11-07 15:23:12 EST
(In reply to Dani Megert from comment #9)
> (In reply to Stephan Herrmann from comment #8)
> > Changed the bug title.
> > 
> > Note that this requires new API (IProblem).
> 
> Why is that? The compiler already recognizes @Deprecated(forRemoval=true)
> and issues a different message. Why can't it do it for the @since tag
> without adding new API?

It requires adding new API constants to IProblem.

The messages relating to "forRemoval" are IProblem.UsingTerminallyDeprecatedType ff.

@Deprecated#since (not @since :) ) is independent from "forRemoval", for each location (Type, Method, Constructor, Field) we'd effectively need 4 IProblem constants (i.e., 2 in addition to what we already have), for each combination of forRemoval and since.

Corresponding messages for illustration:

5 = The type {0} is deprecated
1400 = The type {0} has been deprecated and marked for removal
xxxx = The type {0} has been deprecated since {1} and marked for removal
yyyy = The type {0} is deprecated since {1}


-------------
TL;DR: I'm requesting approval for adding new API in 4.7.2 :)
-------------

If clients like JDT/UI reference any of the old constants relating to deprecation, they should recognize that the new constants are refinements of existing problems, otherwise this could cause loss of functionality.
(First quick search didn't yield any occurrences in JDT/UI, though).
Comment 11 Dani Megert CLA 2017-11-08 12:23:16 EST
(In reply to Stephan Herrmann from comment #10)
> TL;DR: I'm requesting approval for adding new API in 4.7.2 :)

I'm fine to approve that, but maybe we can just move it to 4.7.3?
Comment 12 Stephan Herrmann CLA 2017-11-08 12:58:22 EST
(In reply to Dani Megert from comment #11)
> (In reply to Stephan Herrmann from comment #10)
> > TL;DR: I'm requesting approval for adding new API in 4.7.2 :)
> 
> I'm fine to approve that, but maybe we can just move it to 4.7.3?

perfect
Comment 13 Eclipse Genie CLA 2018-01-21 15:00:29 EST
New Gerrit change created: https://git.eclipse.org/r/115765
Comment 14 Stephan Herrmann CLA 2018-01-21 15:03:39 EST
(In reply to Eclipse Genie from comment #13)
> New Gerrit change created: https://git.eclipse.org/r/115765

The fix grew a bit larger than expected, because some more infra structure ground work was needed.

The compiler was not storing any @Deprecated annotation if !CompilerOptions.storeAnnotations, which also affects the previously implemented addition regarding forRemoval.
To resolve this several locations now check:
- are we at 9 or greater?
- do we have a @Deprecated annotation?
- are we configured to drop annotations?
If yes to all, then force storing this @Deprecated annotation.
This affects both source Annotations and their binary versions in ClassFileReader, MethodInfo and FieldInfo. The latter also remember the class file version, now, for minimal impact of this change.
The additional flag "forceStore" in setAnnotations() et al spreads into lots of locations.
Comment 16 Stephan Herrmann CLA 2018-01-22 08:30:25 EST
(In reply to Eclipse Genie from comment #15)
> Gerrit change https://git.eclipse.org/r/115765 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=67d6195b2e1eca85cb02928c65c7f9577967fe35

Released for 4.8 M5.

Ticket remains open for:

- backport to 4.7.3 after more testing / review

- investigating strange setting of AnnotationTerminallyDeprecated in BTB, introduced by bug 517326
Comment 17 Dani Megert CLA 2018-01-23 10:57:40 EST
This change causes unexpected compile errors in 1.4 projects.

1. Start new workspace
2. Install 1.4 JRE
3. Import org.eclipse.jdt.ui.examples.javafamily from the jdt.ui repo

==>

The type java.lang.Deprecated cannot be resolved. It is indirectly referenced from required .class files
ExampleProjectCreationOperation.java
/org.eclipse.jdt.ui.examples.projects/examples/org/eclipse/jdt/internal/ui/exampleprojects

Reverting the "fix" fixes the problem.

This must be fixed for M5.
Comment 18 Stephan Herrmann CLA 2018-01-23 11:46:52 EST
(In reply to Dani Megert from comment #17)
> This change causes unexpected compile errors in 1.4 projects.
> 
> 1. Start new workspace
> 2. Install 1.4 JRE
> 3. Import org.eclipse.jdt.ui.examples.javafamily from the jdt.ui repo
> 
> ==>
> 
> The type java.lang.Deprecated cannot be resolved. It is indirectly
> referenced from required .class files
> ExampleProjectCreationOperation.java
> /org.eclipse.jdt.ui.examples.projects/examples/org/eclipse/jdt/internal/ui/
> exampleprojects
> 
> Reverting the "fix" fixes the problem.
> 
> This must be fixed for M5.

Looking into this now.
Comment 19 Stephan Herrmann CLA 2018-01-23 11:57:48 EST
Reproduced.

BTW: I had problems configured my 1.4 JRE, because I only have a linux 32 bit version (since that's what I used when 1.4 was current), which is no longer recognized on my 64 bit box (can't execute bin/java).
Any hints on where I can download 64 bit versions of 1.4? If any?
Comment 20 Eclipse Genie CLA 2018-01-23 12:08:56 EST
New Gerrit change created: https://git.eclipse.org/r/115899
Comment 21 Stephan Herrmann CLA 2018-01-23 12:13:34 EST
(In reply to Eclipse Genie from comment #20)
> New Gerrit change created: https://git.eclipse.org/r/115899

This fixes the issue for me, test will follow soon.

In one place I was asking getAnnotations() outside a compliance check, not expecting that a javadoc @deprecated tag would set TagBits.AnnotationDeprecated.
Comment 22 Dani Megert CLA 2018-01-23 12:17:20 EST
(In reply to Stephan Herrmann from comment #19)
> Reproduced.
> 
> BTW: I had problems configured my 1.4 JRE, because I only have a linux 32
> bit version (since that's what I used when 1.4 was current), which is no
> longer recognized on my 64 bit box (can't execute bin/java).
> Any hints on where I can download 64 bit versions of 1.4? If any?

http://www.oracle.com/technetwork/java/javasebusiness/downloads/java-archive-downloads-javase14-419411.html
Comment 23 Stephan Herrmann CLA 2018-01-23 12:47:00 EST
(In reply to Dani Megert from comment #22)
> (In reply to Stephan Herrmann from comment #19)
> > Reproduced.
> > 
> > BTW: I had problems configured my 1.4 JRE, because I only have a linux 32
> > bit version (since that's what I used when 1.4 was current), which is no
> > longer recognized on my 64 bit box (can't execute bin/java).
> > Any hints on where I can download 64 bit versions of 1.4? If any?
> 
> http://www.oracle.com/technetwork/java/javasebusiness/downloads/java-archive-
> downloads-javase14-419411.html

Thanks!

Creating a test requires more work, because the bug can only be triggered if you compile agains JRE 1.4 but reference a class file that references java.lang.Deprecated (which implies the reported error is "correct" - still I agree it is not nice).
Comment 24 Stephan Herrmann CLA 2018-01-23 13:16:17 EST
(In reply to Dani Megert from comment #22)
> (In reply to Stephan Herrmann from comment #19)
> > Reproduced.
> > 
> > BTW: I had problems configured my 1.4 JRE, because I only have a linux 32
> > bit version (since that's what I used when 1.4 was current), which is no
> > longer recognized on my 64 bit box (can't execute bin/java).
> > Any hints on where I can download 64 bit versions of 1.4? If any?
> 
> http://www.oracle.com/technetwork/java/javasebusiness/downloads/java-archive-
> downloads-javase14-419411.html

Unfortunately, neither .bin nor .rpm.bin can be installed on a modern system, aborts with:
Extracting...
./install.sfx.10039: 1: ./install.sfx.10039: ��: not found
./install.sfx.10039: 1: ./install.sfx.10039: ELF2�▒@@H�@8@@@@@���@�@▒▒@@��������P: not found
./install.sfx.10039: 2: ./install.sfx.10039: Syntax error: "(" unexpected
Done.
Comment 26 Stephan Herrmann CLA 2018-01-23 13:53:08 EST
Test released via https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ff655504c93c01dd0b3ca79d794942dabc7dc0ef

This test is a bit crude: I wanted to place this in the compiler.regression suite, but there we don't have the means to compile against a specific JRE (only at specific compliance). I quickly invented a mechanism for hiding a type from the compiler.

Test & fix re comment 17 have been released for 4.8 M5. Resetting status to previous.
Comment 27 Eclipse Genie CLA 2018-01-23 14:26:50 EST
New Gerrit change created: https://git.eclipse.org/r/115909
Comment 28 Stephan Herrmann CLA 2018-01-23 14:28:09 EST
(In reply to Eclipse Genie from comment #27)
> New Gerrit change created: https://git.eclipse.org/r/115909

I found and fixed another omission: when overriding deprecated methods (warning disabled by default) the warning did not show the since value.
Comment 29 Stephan Herrmann CLA 2018-01-23 14:46:08 EST
(In reply to Stephan Herrmann from comment #16)
> - investigating strange setting of AnnotationTerminallyDeprecated in BTB,
> introduced by bug 517326

Moved to new bug 530206
Comment 30 Stephan Herrmann CLA 2018-01-23 18:56:30 EST
(In reply to Stephan Herrmann from comment #28)
> (In reply to Eclipse Genie from comment #27)
> > New Gerrit change created: https://git.eclipse.org/r/115909
> 
> I found and fixed another omission: when overriding deprecated methods
> (warning disabled by default) the warning did not show the since value.

On Jenkins test now consistently fail with 

Unexpected standard output for invocation with arguments ["/tmp/comptest/run.1516743234051/regression/src2/Y.java" -extdirs "/tmp/comptest/run.1516743234051/lib:/tmp/comptest/run.1516743234051/regression/src1" -sourcepath "/tmp/comptest/run.1516743234051/regression/src1" -1.5 -g -preserveAllLocals -verbose -proceedOnError -referenceInfo -d "/tmp/comptest/run.1516743234051/regression" ].
----------- Expected ------------
[parsing    ---OUTPUT_DIR_PLACEHOLDER---/src2/Y.java - #1/1]\n
[parsing    ---OUTPUT_DIR_PLACEHOLDER---/src1/X.java - #2/2]\n
[reading    java/lang/Object.class]\n
[analyzing  ---OUTPUT_DIR_PLACEHOLDER---/src2/Y.java - #1/2]\n
[writing    Y.class - #1]\n
[completed  ---OUTPUT_DIR_PLACEHOLDER---/src2/Y.java - #1/2]\n
[analyzing  ---OUTPUT_DIR_PLACEHOLDER---/src1/X.java - #2/2]\n
[reading    my/pkg/Zork.class]\n
[writing    X.class - #2]\n
[completed  ---OUTPUT_DIR_PLACEHOLDER---/src1/X.java - #2/2]\n
[2 units compiled]\n
[2 .class files generated]\n

------------ but was ------------
[parsing    /tmp/comptest/run.1516743234051/regression/src2/Y.java - #1/1]\n
[reading    X.class]\n
[reading    java/lang/Object.class]\n
[analyzing  /tmp/comptest/run.1516743234051/regression/src2/Y.java - #1/1]\n
[writing    Y.class - #1]\n
[completed  /tmp/comptest/run.1516743234051/regression/src2/Y.java - #1/1]\n
[1 unit compiled]\n
[1 .class file generated]\n

Running locally I cannot see the error, nor do I have an idea what that failure has to do with my change.
Comment 31 Stephan Herrmann CLA 2018-01-23 18:57:29 EST
(In reply to Stephan Herrmann from comment #30)
> (In reply to Stephan Herrmann from comment #28)
> > (In reply to Eclipse Genie from comment #27)
> > > New Gerrit change created: https://git.eclipse.org/r/115909
> > 
> > I found and fixed another omission: when overriding deprecated methods
> > (warning disabled by default) the warning did not show the since value.
> 
> On Jenkins test now consistently fail with 

should mention the test: BatchCompilerTest.test025()
Comment 32 Dani Megert CLA 2018-01-24 04:45:39 EST
Verified in eclipse-SDK-I20180123-2000-win32-x86_64 that the problem from comment 17 is fixed. Thanks!
Comment 33 Stephan Herrmann CLA 2018-01-25 05:56:26 EST
(In reply to Stephan Herrmann from comment #31)
> (In reply to Stephan Herrmann from comment #30)
> > (In reply to Stephan Herrmann from comment #28)
> > > (In reply to Eclipse Genie from comment #27)
> > > > New Gerrit change created: https://git.eclipse.org/r/115909
> > > 
> > > I found and fixed another omission: when overriding deprecated methods
> > > (warning disabled by default) the warning did not show the since value.
> > 
> > On Jenkins test now consistently fail with 
> 
> should mention the test: BatchCompilerTest.test025()

Seeing that this also affects the M5 candidate build [1] I investigated further and found that this only happens when running DeprecatedTests & BatchCompilerTest within the same execution => lack of isolation between tests, not a problem in main code.

I'm on it.

[1] http://download.eclipse.org/eclipse/downloads/drops4/I20180124-2000/testResults.php
Comment 34 Eclipse Genie CLA 2018-01-25 07:00:56 EST
New Gerrit change created: https://git.eclipse.org/r/116029
Comment 35 Stephan Herrmann CLA 2018-01-25 07:09:16 EST
(In reply to Eclipse Genie from comment #34)
> New Gerrit change created: https://git.eclipse.org/r/116029

Several tests place their jar files into LIB_DIR, BatchCompilerTest.test025() puts all of LIB_DIR into -extdir => any of the former tests may influence test025(). To add insult to missing isolation, DeprecatedTest.test008a and BatchCompilerTest.test025 both reference class X from the default package => boom.

Solution attempt: *always* wipe LIB_DIR during tearDown().
Comment 36 Stephan Herrmann CLA 2018-01-25 08:19:04 EST
(In reply to Stephan Herrmann from comment #35)
> (In reply to Eclipse Genie from comment #34)
> > New Gerrit change created: https://git.eclipse.org/r/116029
> 
> Several tests place their jar files into LIB_DIR,
> BatchCompilerTest.test025() puts all of LIB_DIR into -extdir => any of the
> former tests may influence test025(). To add insult to missing isolation,
> DeprecatedTest.test008a and BatchCompilerTest.test025 both reference class X
> from the default package => boom.
> 
> Solution attempt: *always* wipe LIB_DIR during tearDown().

Tests are green. Waiting for master to be re-opened after M5 is declared.
Comment 39 Stephan Herrmann CLA 2018-01-26 18:19:36 EST
Released for 4.8 M6:

(In reply to Eclipse Genie from comment #37)
> Gerrit change https://git.eclipse.org/r/115909 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=6c5635722c92429933514f7117de3f83f298ef2d

Fixed the case of overriding a @Deprecated/since method.

(In reply to Eclipse Genie from comment #38)
> Gerrit change https://git.eclipse.org/r/116029 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=77243bbcfe3ea935bddfbf047e794a3d6a6483b2

Test isolation to prevent failures in BatchCompilerTest.
Comment 40 Stephan Herrmann CLA 2018-02-01 05:49:43 EST
A few comments / questions while I'm preparing a squashed back port candidate:

API tools gives errors on the new constants in IProblem, I guess I should add API filters, but am not sure which way to go:
(1) change @since from 3.14 to 3.13 and add filter against minor version update in Manifest, or
(2) keep @since at 3.14 and add filters for all new constants


Due to bug 528108 about 50% of the files had conflicts during cherry pick, which then triggered EGit bug 441149. This implies that a significant amount of error-prone manual work was involved. Just saying.


If we back port we should probably include a (trivial) fix for bug 530600 to avoid loss of functionality.
Comment 41 Eclipse Genie CLA 2018-02-01 06:51:58 EST
New Gerrit change created: https://git.eclipse.org/r/116526
Comment 42 Stephan Herrmann CLA 2018-02-01 07:35:48 EST
Mmhh, RunAllJava9Tests locally give 4-5 failures, and this happens on head of R4_7_maintenance even before the change from this bug:

ModuleBuilderTests.testAutoModule4
GenericRegressionTest_9.testBug488663_005
ModuleCompilationTests.testBug500170b
ASTConverter9Test.testBug514417
ModuleCompilationTests.testBug519922

Some of this are known test issues, plus bug 500170.
Comment 43 Stephan Herrmann CLA 2018-02-01 09:13:42 EST
Status:

On Jenkins all tests pass (because R4_7_maintenance doesn't yet test on Java 9)

A patch-under-test exists in bug 530600.

Patch here is larger than may have been expected, because the compiler did not have the necessary details of standard annotation @Deprecated in all situations.

In that light the fix here could potentially contribute to bug 529282 (NPE in binary annotation details), perhaps by triggering a buggy situation more frequently than before.

I have not yet added any API filters (see comment 40).


If anyone wants to comment/review the back-port, now would be a good time.
Comment 44 Eclipse Genie CLA 2018-02-06 11:53:25 EST
Gerrit change https://git.eclipse.org/r/116526 was merged to [R4_7_maintenance].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=bf3881160a8c2cad0003a22cceb7074496967f3b
Comment 45 Stephan Herrmann CLA 2018-02-06 11:56:10 EST
At a closer look I could find no theory how bug 529282 could possibly be caused by changes in this bug, so ...

(In reply to Eclipse Genie from comment #44)
> Gerrit change https://git.eclipse.org/r/116526 was merged to
> [R4_7_maintenance].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=bf3881160a8c2cad0003a22cceb7074496967f3b

... I released the fix for 4.7.3.
Comment 46 Manoj N Palat CLA 2018-02-15 03:06:48 EST
Verified for Eclipse Oxygen.3 (4.7.3) with Build id: M20180214-1700
Comment 47 Jay Arthanareeswaran CLA 2018-03-06 10:34:59 EST
Verified for 4.8 M6 with build I20180305-2000