"var" type inferrence bug with generic wrapping

[stephan-herrmann]

• Ignore type bound of "? extends Object"
• Preserve null info from erased bound '? extends @nonnull Object'
• Test adjustments

Fixes #5028
Fixes #2857

[stephan-herrmann]

@srikanth-sankaran fyi I organized the changes as 3 commits for easier reviewing, in case you find the time for it.

commit 1 brings the main change in TypeSystem.getWildcard(), where we simply drop "extends Object". In this commit I adjusted tests only where we expected error messages with "? extends Object", which in many cases is no longer shown. Only for one error message I restore that information from the AST.

commit 2 resolves all remaining failures in GenericTypeTest, where changes were indeed significant, but AFAICS all changes are going towards alignment with javac. This is the part that might deserve special scrutiny, but running this test class with -Drun.javac=enabled indeed shows better results than before! Note, that in this test mode you can use the console output to see all error messages from javac, too.

commit 3 finally restores nullness information from ? extends @NonNull Object, which otherwise got dropped when dropping the bound altogether.

[srikanth-sankaran]

@srikanth-sankaran fyi I organized the changes as 3 commits for easier reviewing

I'll wait for the test failures to be cleaned up and then review - OK ? Thanks

[stephan-herrmann]

Test failures revealed that curiously our jclMinXY.jar declare Object.getClass() as returning Class<? extends Object> which is neither useful nor does it reflect the situation in real JDK. Anyway, via find+replace test expectations no longer insist on the useless bound (commit 4).

[stephan-herrmann]

@srikanth-sankaran fyi I organized the changes as 3 commits for easier reviewing

I'll wait for the test failures to be cleaned up and then review - OK ? Thanks

OK, test failures have been resolved. I only need to remove a now-obsolete @SuppressWarnings("unchecked") in our code. Will have a look why that one changed.

@jarthana since this is a fundamental change it would be great to have this change stress tested. If possible with imposing stress on you :) I'd like to merge this for M3.

[stephan-herrmann]

I only need to remove a now-obsolete @SuppressWarnings("unchecked") in our code. Will have a look why that one changed.

Previously, we suppressed this warning:
Type safety: The expression of type List needs unchecked conversion to conform to List<? extends Object>

Inside TypeBinding.needsUncheckedConversion(TypeBinding) we call isBoundParameterizedType() which says: "Returns true if parameterized type AND not of the form List<?>". By erasing the useless extends Object we fall into that exception exactly.

A second warning was
Type safety: Unchecked invocation unmodifiableList(List) of the generic method unmodifiableList(List<? extends T>) of type Collections

but that one is tightly coupled to the former warning via code INVOCATION_ARGUMENT_UNCHECKED.

Interestingly, javac is reporting the second warning but not the first:

warning: [unchecked] unchecked method invocation: method unmodifiableList in class Collections is applied to given types
                                        return Collections.unmodifiableList(list);
                                                                           ^
  required: List<? extends T>
  found:    List
  where T is a type-variable:
    T extends Object declared in method <T>unmodifiableList(List<? extends T>)

I doubt the usefulness of that warning, though, since no part of the code should make any assumptions based on the inferred List<?>, no assumptions which could blow up at runtime.

[jarthana]

@jarthana since this is a fundamental change it would be great to have this change stress tested. If possible with imposing stress on you :) I'd like to merge this for M3.

Looks good.

[srikanth-sankaran]

I have made one pass over the changes. Code changes received close attention while test changes were only glanced through. I will closely scrutinize the test changes tomorrow.

[srikanth-sankaran]

A second warning was Type safety: Unchecked invocation unmodifiableList(List) of the generic method unmodifiableList(List<? extends T>) of type Collections

but that one is tightly coupled to the former warning via code INVOCATION_ARGUMENT_UNCHECKED.

Interestingly, javac is reporting the second warning but not the first:

This worries me somewhat - I haven't fully investigated it. My worry is about any ripple effects around not detecting an unchecked conversion. As I recall there are various clauses in the JLS that stipulate different behavior based on whether an unchecked cast was involved. Don't have a ready reference though. I'll trust your intuition here.

If raw types were not in the picture, the warning would go away even with javac ? That would mitigate my worries a whole lot.

[srikanth-sankaran]

As commented in #5054 (comment) the difference in warning behavior worries me a bit - I will go with your analysis and intuition here.

[srikanth-sankaran]

This is what you allude to in the top half of #5054 (comment) I believe. Looks good.

[srikanth-sankaran]

Diagnostics skip from 1 to 3 ... I have seen this before that such jumps don't cause test failures. If you have other changes, fix this one too along with it.

[stephan-herrmann]

I found it kind of cute, that this is tolerated, allowing us to minimize the diff. :)
OTOH, I agree that this looks confusing now.

[srikanth-sankaran]

Do you plan to raise this with the OpenJDK team ?

[stephan-herrmann]

Yea, I probably should. Reduced test:

public abstract class X<T extends Runnable> implements Runnable {
	void foo1(X<X<? extends Runnable>> lhs, X<X<? extends Object>> rhs) {
		lhs = rhs;
	}
	void foo2(X<X<? extends Runnable>> lhs, X<X<?>> rhs) {
		lhs = rhs;
	}
}

Should in any way report an even number or errors, but:

• ecj: 0
• javac: 1

[srikanth-sankaran]

Nit: Diagnostics number is not sequential

[srikanth-sankaran]

Overall looks good.

1. A couple of minor comments about diagnostics not being sequential. Since these seem tolerated, you may ignore them unless you are also considering other changes.
2. A question about follow up with javac folk on behavior.
3. Please double check once again to satisfy yourself that what is documented in #5054 (comment) is all well (I have no definite problem to report here)

[srikanth-sankaran]

I ran a query to pull up all open github tickets that contain the string "extends Object" somewhere in the body.

Here is the result. At leisure you may want to see if any of them change behavior due to the fix here, hopefully in the right direction.

[stephan-herrmann]

A second warning was Type safety: Unchecked invocation unmodifiableList(List) of the generic method unmodifiableList(List<? extends T>) of type Collections
but that one is tightly coupled to the former warning via code INVOCATION_ARGUMENT_UNCHECKED.
Interestingly, javac is reporting the second warning but not the first:

This worries me somewhat - I haven't fully investigated it. My worry is about any ripple effects around not detecting an unchecked conversion.

Please note that javac does not report unchecked conversion but unchecked invocation. I would expect this warning to be specified somewhere in JLS 15.12 but so far I found nothing in this direction.

While I was deep into arguing that there is no reason for any warning I found something worth further investigation -> #5062.

Back to the issue at hand, however, we have

• a declaration <T> List<T> unmodifiableList(List<? extends T>
• inference instantiating T as Object
• a resulting signature of List<Object> unmodifiableList(List<? extends Object>
• which is equivalent to List<Object> unmodifiableList(List<?>)
• we provide an argument of type List#RAW to a parameter List<?>
  • due to "All of the type arguments of T are unbounded wildcards" this is exempted from unchecked warnings

I see no reason for any warning, pending the result of discussing #5062.

As I recall there are various clauses in the JLS that stipulate different behavior based on whether an unchecked cast was involved. Don't have a ready reference though. I'll trust your intuition here.

If raw types were not in the picture, the warning would go away even with javac ? That would mitigate my worries a whole lot.

Right, replacing both instances of List with List<?> silences javac.

[stephan-herrmann]

I ran a query to pull up all open github tickets that contain the string "extends Object" somewhere in the body.

Here is the result. At leisure you may want to see if any of them change behavior due to the fix here, hopefully in the right direction.

Thanks, this is very helpful.

For some anecdotal reference I found in an email as early as Feb 2014:

Does it make a difference whether we have S<?> or S<? extends Object>?
I don't have a lot of answers here, maybe the JLS has some?

:-/

[stephan-herrmann]

Overall looks good.

Thanks for your diligence!

1. A couple of minor comments about diagnostics not being sequential. Since these seem tolerated, you may ignore them unless you are also considering other changes.

Since no further changes are planned I'll leave it at this. I noted, though, that in the future I will prefer contiguous numbering over minimal test changes.

2. A question about follow up with javac folk on behavior.

Noted, will do. Perhaps after scanning your list of "extends Object" issues.

3. Please double check once again to satisfy yourself that what is documented in #5054 (comment) is all well (I have no definite problem to report here)

For the case at hand I argue that no harm is done, while a more general question has been out sourced to #5062

Based on the approval, the only thing left to do is coordination regarding eclipse-platform/eclipse.platform.ui#3992

[stephan-herrmann]

Here is the result. At leisure you may want to see if any of them change behavior due to the fix here, hopefully in the right direction.

No changes found in any of those examples. Actually, there was hardly any mention of ? extends Object, but I also found no way to further restrict the search.

[iloveeclipse]

@stephan-herrmann : FYI: one jdt ui test needs expectation update:
https://download.eclipse.org/eclipse/downloads/drops4/I20260512-0925/testresults/html/org.eclipse.jdt.ui.tests.refactoring_ep440I-unit-linux-x86_64-java21_linux.gtk.x86_64_21.html