Skip to content
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.

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

@SuppressWarnings("removal") does not work #3402

Open
laeubi opened this issue Dec 4, 2024 · 14 comments · May be fixed by #3532
Open

@SuppressWarnings("removal") does not work #3402

laeubi opened this issue Dec 4, 2024 · 14 comments · May be fixed by #3532
Assignees
Labels
bug Something isn't working

Comments

@laeubi
Copy link
Contributor

laeubi commented Dec 4, 2024

If I look at

org.eclipse.pde.internal.core.product.ArgumentsInfo

it has

	@SuppressWarnings("deprecation")
	@Override
	public String getCompleteProgramArguments(String os, String arch) {

but eclipse claims

Unnecessary @SuppressWarnings("deprecation")

so far so good, then we can remove it, but now I don't get any warning at all even though a forRemoval should be even more serve

grafik

this was recently changed here:

@jukzi
Copy link
Contributor

jukzi commented Dec 4, 2024

After deprecation for removal that method would now need a @SuppressWarnings("removal") instead. However it does not work in this case -> bug
see also #469 @subyssurendran666

@jukzi jukzi added the bug Something isn't working label Dec 4, 2024
@jukzi jukzi changed the title Deprecate for removal does not give a warning anymore @SuppressWarnings("removal") does not work Dec 4, 2024
@laeubi
Copy link
Contributor Author

laeubi commented Dec 4, 2024

After deprecation for removal that method would now need a @SuppressWarnings("removal") instead.

That's what I would expect that it still complains (but need a different suppress.

@HannesWell
Copy link
Contributor

Could anyone from JDT please have a look at this? The case described above is causing build-failures in PDE due to new warnings and actually I don't want to remove the suppression now just to add it back later when this is fixed.

If you can point me to the place where this can be fixed, I can also have a look by myself.

@srikanth-sankaran
Copy link
Contributor

Could anyone from JDT please have a look at this? The case described above is causing build-failures in PDE due to new warnings and actually I don't want to remove the suppression now just to add it back later when this is fixed.

If you can point me to the place where this can be fixed, I can also have a look by myself.

See call sites of org.eclipse.jdt.internal.compiler.problem.ProblemReporter.unusedWarningToken(Expression) for the logic that emits the Unnecessary @SuppressWarnings(\<Token\>)

org.eclipse.jdt.internal.compiler.ast.Annotation.recordSuppressWarnings(Scope, int, int, boolean) and org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.finalizeProblems() are places of interest, in the latter the block of code that carries the comment // flag SuppressWarnings which had no effect (only if no (mandatory) error got detected within unit

In general a good way to find out where different things happen in ECJ is to plant a breakpoint in org.eclipse.jdt.internal.compiler.problem.ProblemHandler.handle(int, String[], int, String[], int, int, int, ReferenceContext, CompilationResult)

EVERY Error/Warning/Info diagnostics go through this method.

So plant a breakpoint there and give the compiler some workload that would trigger a "relevant" message.

Call stack points to the smoking gun 😄

(Sorry that I am personally not able to jump on this task - I am trying to wrap up Enhanced switches redesign/reimplementation before the year end holidays)

Good luck. I am available to review PRs though

@jarthana
Copy link
Member

jarthana commented Jan 7, 2025

Reproduced. Will take a look.

@jarthana
Copy link
Member

jarthana commented Jan 7, 2025

Hmm... The compiler doesn't report or try to report deprecated warning at all. And the reason for that is, JDT only considers JDK versions for the Deprecated annotation. The relevant code is in ASTNode#sinceValueUnreached().

I don't even know how we can compare the given version since = "2025-03" with the project's compliance, which is what we are doing now.

@jukzi
Copy link
Contributor

jukzi commented Jan 7, 2025

impossible to compare by design: The text has no special internal structure
https://docs.oracle.com/javase/7/docs/technotes/tools/windows/javadoc.html#since

@laeubi
Copy link
Contributor Author

laeubi commented Jan 7, 2025

Looking at the javadoc it says:

Returns the version in which the annotated element became deprecated

It does not indicate that this has any relation to the java version at all.

In the general section it says:

Compilers issue warnings when a deprecated program element is used or overridden in non-deprecated code.
This annotation type has a string-valued element since. The value of this element indicates the version in which the annotated program element was first deprecated.

it does not tell anything that compiler should further analyze the parameters

@jarthana
Copy link
Member

jarthana commented Jan 7, 2025

For the record, this change was introduced as part #2873 and commit 54fbfc4

@iloveeclipse
Copy link
Member

JDT only considers JDK versions for the Deprecated annotation

@jjohnstn , @stephan-herrmann : from the conversation on #2874 (comment) I see that you both assumed that "since" version in the "Deprecated" annotation will represent JDK version - why ?

Isn't the annotation fully generic and can be used to annotate any code and in any version format: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/lang/Deprecated.html

This is the related diff:

https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2874/files#diff-81f035749c916e40ed2032d500dc139c0400468dd65b122421a34c82b1602e18R585

Do I miss something?

@stephan-herrmann
Copy link
Contributor

It seems I was misled by lots of text in https://openjdk.org/jeps/277 speaking about the use of enhanced deprecation in the JDK, where indeed "since" refers to the JDK version. I should have re-checked the specification before commenting in #2874. Sorry.

At face value this seems to imply that the compiler can only make sense of the since value when it appears in a JDK class, since JDK versions are the only versions the compiler knows about. But then we'd need to check, if the compiler can tell which classes are from the JDK. If it can't then #2874 should probably reverted in entirety. In that case we should put our money on #3168 instead.

I haven't read the full thread above. Does my assessment make sense in this context?

@iloveeclipse
Copy link
Member

yes

@stephan-herrmann
Copy link
Contributor

If we'd want to keep #2874 for JDK classes then NameEnvironmentAnswer and BinaryTypeBinding both would need a new flag (to be set from clients like ClasspathJrt). I'm not sure if that's worth it.

Feel free to simply revert #2874 then (sorry @jjohnstn).

@jukzi
Copy link
Contributor

jukzi commented Jan 8, 2025

Note that JDK also documented that they did not use pure numeric values in the past but also Strings like jdk5 or with minor versions. I would favor to not have special assumptions for JDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
7 participants