Suspect code in org.eclipse.jdt.internal.compiler.ast.ConditionalExpression.resolveType(BlockScope)

[srikanth-sankaran]

The lines below look suspect and need to be studied in detail. This was introduced for https://bugs.eclipse.org/bugs/show_bug.cgi?id=569498 which addresses a regression introduced by https://bugs.eclipse.org/bugs/show_bug.cgi?id=566332

It is likely the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=566332 violates Poly Expression resolution design and the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=569498 somehow covers up for it.

		if (isPolyExpression()) {
			if (this.expectedType == null || !this.expectedType.isProperType(true)) {
				// We will be back here in case of a PolyTypeBinding. So, to enable
				// further processing, set it back to default.
				this.constant = Constant.NotAConstant;
				return new PolyTypeBinding(this);
			}
			return this.resolvedType = computeConversions(scope, this.expectedType) ? this.expectedType : null;
		}

[srikanth-sankaran]

Coincidentally, MessageSend seems to have had similar re-entry issues - see #520 (comment)

In general resetting that constant status is a smelly implementation and is asking for trouble.

[stephan-herrmann]

@srikanth-sankaran do you have a specific scenario in mind, where this code might cause trouble, or is this just a matter of some principle? If principle: which is it?

Coincidentally, MessageSend seems to have had similar re-entry issues - see #520 (comment)

In that case, the problem was not in MessageSend, but in SwitchExpression, which shouldn't have triggered re-entrance in case of hard failure of the first invocation. I.e., SwitchExpression, which itself can be poly must not cause undue repeated attempts at its children.

Is something similar (w/ nested polies) happening with ConditionalExpression as well?

[srikanth-sankaran]

@srikanth-sankaran do you have a specific scenario in mind, where this code might cause trouble, or is this just a matter of some principle? If principle: which is it?

No, I don't readily know where this may create trouble - I haven't studied it in detail. This ticket was just to schedule such a study in future.

I think it is violating the design intent.

As I recall, an ASTNode type that implements IPolyExpression must be prepared to be asked to resolve itself more than once but at most twice (i.e., a physical instance of it. Combined with its clones it may be resolved many times bounded only by the overloads)

A physical instance of a poly expression ASTNode will be resolved EXACTLY ONCE if in the context in which it features, the target type is available right upfront.

A physical instance of a poly expression ASTNode will be resolved EXACTLY TWICE if in the context in which it features, the target type is NOT available upfront: Once initially without a target type - the expression will do some type specific initial work and return a PolyTypeBinding - and the second time when the target type has been determined.

Historically, we have distinguished these two invocations by using the pattern:

                if (this.constant != Constant.NotAConstant) {
			this.constant = Constant.NotAConstant;
                       // ... First of two resolves or the ONLY resolve
                } else {
                      // ... second of the two resolves.
               } 

In this scheme, any resetting of the this.constant signals misplacement of code - something was placed in the if() {} that doesn't belong there and to work around it, the flag is being cleared would be the inference.

Now this is all from my memory from a decade ago - I haven't actually had to revisit any of these in the past 18 months.
Let is see how much and how accurately I remember things: I have kicked off https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3210 to validate the assumptions.

Let us see what fails

Is something similar (w/ nested polies) happening with ConditionalExpression as well?

Like I said, I don't know of a real problem here - When I was debugging, I saw this reset and alarm bells went off in my head.

While waiting for github CI run to finish, I am running org.eclipse.jdt.core.tests.compiler.regression.TestAll locally - see a handful of failures - interesting!

[srikanth-sankaran]

While waiting for github CI run to finish, I am running org.eclipse.jdt.core.tests.compiler.regression.TestAll locally - see a handful of failures - interesting!

Not a handful - just org.eclipse.jdt.core.tests.compiler.regression.PolymorphicSignatureTest.testBug475996() over many compliance levels,

[srikanth-sankaran]

While waiting for github CI run to finish, I am running org.eclipse.jdt.core.tests.compiler.regression.TestAll locally - see a handful of failures - interesting!

Not a handful - just org.eclipse.jdt.core.tests.compiler.regression.PolymorphicSignatureTest.testBug475996() over many compliance levels,

CI run finished with just the same one failure - need to see what about that test causes resolves > 2

[stephan-herrmann]

As I recall, an ASTNode type that implements IPolyExpression must be prepared to be asked to resolve itself more than once but at most twice

I was about to warn, that inference may invoke resolving of lambdas / reference expressions at unexpected times, but on second look those go through resolveExpressionExpecting where caching of resolved copies prevents repeated resolving, good!

[srikanth-sankaran]

In this scheme, any resetting of the this.constant signals misplacement of code - something was placed in the if() {} that doesn't belong there and to work around it, the flag is being cleared would be the inference.

Indeed, I spot the misplaced code - will propose a patch

[srikanth-sankaran]

As I recall, an ASTNode type that implements IPolyExpression must be prepared to be asked to resolve itself more than once but at most twice

I was about to warn, that inference may invoke resolving of lambdas / reference expressions at unexpected times, but on second look those go through resolveExpressionExpecting where caching of resolved copies prevents repeated resolving, good!

Likewise overload resolution may cause a re-resolution indirectly via calls to org.eclipse.jdt.internal.compiler.ast.LambdaExpression.isCompatibleWith(TypeBinding, Scope) et all, but these operate on the clones.

[srikanth-sankaran]

In this scheme, any resetting of the this.constant signals misplacement of code - something was placed in the if() {} that doesn't belong there and to work around it, the flag is being cleared would be the inference.

Indeed, I spot the misplaced code - will propose a patch

PR #3211 under test.

Problem is that while a conditional operator in an assignment context is indeed a poly expression, since the target type is known upfront we don't need a two phase resolution approach and won't arrange for a two phase resolution. So nothing can be deferred to the second (non-existent) phase and constant folding was.

[srikanth-sankaran]

@srikanth-sankaran do you have a specific scenario in mind, where this code might cause trouble

I have studied it in detail. The way the code was, it wouldn't have cause any incorrectness. So the overall problem with it was it wasn't aligned with the standard protocol for IPolyExpressions (which I doubt is documented actually or at least prominently)