Revert https://github.com/eclipse-jdt/eclipse.jdt.core/pull/630

[srikanth-sankaran]

What it does

Revert #630 for the time being as it results in a cascade of issues

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[stephan-herrmann]

We have two distinct failures in NullTypeAnnotationTests (multiplied by compliance levels):

• testGH854
  I'm not much surprised see here, but will have a second look shortly
• testBug522142_bogusError
  This might indeed indicate that #683 needs #630 - I'm looking into this now

[stephan-herrmann]

Analysis for testBug522142_bogusError, with this example code:

@NonNullByDefault({PARAMETER, RETURN_TYPE, FIELD, TYPE_PARAMETER, TYPE_BOUND, TYPE_ARGUMENT})
	interface Foo<A> {
		interface Bar<A> extends Iterable<Foo<A>> {
	}
}

After Foo has been correctly translated into Foo<@NonNull A> (due to @NNBD), it's the member type Bar<A> where the reference "Foo<A>" is resolved before the type parameter <A> (declared by Bar) has applied the nullness default. Note that @NNBD should apply only to the declaration of A, not to its application.

Yet, the bound check for the PSTR Foo<A> is deferred, for now its resolved type has no nullness information.

"Belatedly", resolveAnnotations on the same PSTR triggers evaluateNullAnnotations for MTB Bar, where Bar's <A> is translated to <@NonNull A>.

connectTypeHierarchy finishes without finding any problem.

Fast-forward to CUS.checkParameterTypes() and into CS.checkParameterizedTypeBounds() for the member Bar: we need to check the PST for Foo<A>, which still has no nullness information. At this point, the type to check against is "Foo<@NonNull A>". This is were we report that "The type 'A' is not a valid substitute for the type parameter '@NonNull A'"

Ergo: yes, this test from #683 requires additional help either from #630 or from #1077. But ... (tbc)

[stephan-herrmann]

Point-fix for NullTypeAnnotationTest.testBug522142_bogusError() in #1079 in absence of #630.

This is the most careful thing I can think of for 4.28. @srikanth-sankaran WDYT?

Disable testGH854 to enable running of more test suites

I think that test is dispensable for now, but I'll take another look as promised.

[stephan-herrmann]

Disable testGH854 to enable running of more test suites

I think that test is dispensable for now, but I'll take another look as promised.

The failure is caused in the exact code location where my first attempt of #1067 introduced a lot of new scaffolding to ensure an auspicious order of event. I think adding such stuff on top of this revert commit would be too much.

EDIT: cherry picking that commit would indeed fix testGH854 but I wouldn't recommend doing so.

[srikanth-sankaran]

Point-fix for NullTypeAnnotationTest.testBug522142_bogusError() in #1079 in absence of #630.

This is the most careful thing I can think of for 4.28. @srikanth-sankaran WDYT?

Disable testGH854 to enable running of more test suites

I think that test is dispensable for now, but I'll take another look as promised.

Looking into this ...

[stephan-herrmann]

looks good to me

[srikanth-sankaran]

Point-fix for NullTypeAnnotationTest.testBug522142_bogusError() in #1079 in absence of #630.

This is the most careful thing I can think of for 4.28. @srikanth-sankaran WDYT?

Disable testGH854 to enable running of more test suites

I think that test is dispensable for now, but I'll take another look as promised.

Looking into this ...

@stephan-herrmann , your additional point fix on top of (reverse patch + tests) looks OK to me. I agree it could be too much to also squeeze in a fix for testGH854 right now.

@iloveeclipse, we can proceed to merge this - after project lead approval

[iloveeclipse]

we can proceed to merge this - after project lead approval

Please do! Thank you both!