[compiler] Unnecessary cast warning is not detected when casting a bound generic type to its super type

[stephan-herrmann]

fixes #2470

[stephan-herrmann]

@srikanth-sankaran at your leisure (not for 3.32) could you please check if you see any risk in the test changes in reaction to more warnings? I.e., when people react to the warning and remove a cast, could that have unintended effects?

[stephan-herrmann]

The new analysis suggested to remove the cast in this statement (in org.eclipse.jdt.internal.core.util.Util):

return Signature.createUnionTypeSignature(((List<Type>)union.types()).stream()
				.map(Util::getSignature)
				.toArray(String[]::new));

After removing it no longer compiles.

Ergo: even if the method (stream()) can be invoked without a cast, it would have the wrong signature (returning Stream<Object>), so let's generally consider as useful when

• casting a method receiver
• the cast converts a raw type into a parameterized type

to check: parameterized with nested raw type?

[stephan-herrmann]

to check: parameterized with nested raw type?

the cast needed to trick the compiler is illegal to begin with. See new test in 5903df9

[stephan-herrmann]

This change lets ecj flag more casts as unnecessary. Just in jdt.core this triggered more than 100 new warnings (were we that sloppy with casting? 😄).

@srikanth-sankaran do you want to double check that we don't flag necessary casts as unnecessary? In fact the build already agreed to the removal of those 100+ casts.

@iloveeclipse should we coordinate with other projects / announce the fact that they may need to apply the quickfix at large (which is what I did, it worked smoothly with not a single hiccup)?

Implementation note: this is not about the general analysis in CastExpression but concerns casted receivers of method calls. This is now analysed after the MethodBinding has been found: if that method would be available also via the uncasted receiver, then I assume the cast was unnecessary (with special consideration of parameterized types). Could you think of any situation, where still the cast could have an influence on overload resolution?

[stephan-herrmann]

Could you think of any situation, where still the cast could have an influence on overload resolution?

I found a witness. Overloading once more succeeds to invalidate common sense reasoning. Working on an improved patch...

[stephan-herrmann]

Could you think of any situation, where still the cast could have an influence on overload resolution?

I found a witness. Overloading once more succeeds to invalidate common sense reasoning. Working on an improved patch...

Actually some of the test changes from 4221238 were already bogus, signaling casts as unnecessary that indeed changed overload resolution.

With this there doesn't seem to be a shortcut, and hence with the latest change we test exactly this:

• if we would use the un-casted receiver type, which method would we find?
• if that method is valid, is it override-equivalent with the actual resolved method?

[stephan-herrmann]

This change lets ecj flag more casts as unnecessary. Just in jdt.core this triggered more than 100 new warnings ...
@iloveeclipse should we coordinate with other projects / announce the fact that they may need to apply the quickfix at large (which is what I did, it worked smoothly with not a single hiccup)?

Any suggestion, or should I just merge and see what happens? :)

[iloveeclipse]

suggestion, or should I just merge and see what happens? :)

First of all, build is currently failing. So please don't merge it!

To the original question: sorry, I've overlooked this, too many mails.

I guess what would happen is that for most platform projects that enabled "quality gate" github PR validation may fail once new SDK build is there (that would happen on M1 usually, once new compiler is deployed and configured). That could be forced to happen at any time.

However, if I recall it right, we build SDK itself as a whole (not the various platform github projects) with the latest compiler snapshot from JDT master. I wonder if that could cause SDK build failure because some projects may have configured "warning as error".

So ideally one should try to fix all the unnecessary casts first - but that would be flagged as errors by old compiler version? So can the existing code be fixed already (upfront) or not?

Do you have any idea how many new warnings we could have? 1, 10, 100 per project? JDT is probably not a good measure because of old/specific API style used.

The easiest workaround would be to first disable this warning in compiler options for affected projects and after that using new SDK build with new compiler, enable option and fix new warnings.

For that would be really nice to have the according .settings change here as documentation and know who in the platform/UI/equinox/PDE is affected.

[stephan-herrmann]

suggestion, or should I just merge and see what happens? :)

First of all, build is currently failing. So please don't merge it!

sure, I just triggered it manually, to find out.

To the original question: sorry, I've overlooked this, too many mails.

n.p.

So ideally one should try to fix all the unnecessary casts first - but that would be flagged as errors by old compiler version? So can the existing code be fixed already (upfront) or not?

OK, I'll try to wipe off the dust from my full SDK workspace and see how it behaves, and whether I can easily provided fixes upfront.

Old compiler should not require those casts, it just doesn't recognize they're redundant.

[iloveeclipse]

Stephan, it looks like you made a merge commit, but you probably wanted rebase?

[stephan-herrmann]

Stephan, it looks like you made a merge commit, but you probably wanted rebase?

no, but don't worry, that merge commit will never appear in master. Promised.

[stephan-herrmann]

to check: parameterized with nested raw type?

the cast needed to trick the compiler is illegal to begin with. See new test in 5903df9

Interested what new test this refers to? Simply click the link.
Oh no!

[stephan-herrmann]

Do you have any idea how many new warnings we could have? 1, 10, 100 per project? JDT is probably not a good measure because of old/specific API style used.

@iloveeclipse In my SDK workspace I now typically see a handful of such warnings/errors per group of projects (workingset as defined by the oomph setup). JDT Debug has 23, PDE 36, Platform UI 36, JDT UI 52. Total: 308.

I'll prepare PRs where necessary. Where the workspace shows these are warnings, not errors, would those fail a build, too? Or could I leave warning-only projects without such PR?

[iloveeclipse]

JDT Debug has 23, PDE 36, Platform UI 36, JDT UI 52. Total: 308.

Platform & SWT are missing or are they warning free?

I'll prepare PRs where necessary.

Thanks.

Where the workspace shows these are warnings, not errors, would those fail a build, too?

As I've mentioned, that might not fail SDK build, but that would fail the github quality gate, which is, if I understand it right, configured to fail on new warnings for platform. I think @laeubi knows more about details of the quality gate.
So it would be very good to fix most warnings upfront...

[laeubi]

Actually we tried a while ago to unify the builds to show the warnings of the workspace, also they will not fail directly it will just mark the build as introducing new warnings, so either one has to reset the quality gate or better fix the warnings with a followup PR if it can't be done beforehands (similar to version bumps due to compiler changed).

[stephan-herrmann]

JDT Debug has 23, PDE 36, Platform UI 36, JDT UI 52. Total: 308.

Platform & SWT are missing or are they warning free?

Not warning free, but ranging in the field of "a handful of such warnings/errors per group of projects"