Logging inside lambda fails with error

[srikanth-sankaran]

What it does

• Overly defensive short circuiting of lambda exception analysis results in compilation failure

• Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=547231

Author checklist

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

[srikanth-sankaran]

@stephan-herrmann : Adding one more to your review queue.

I am trying to rack my brain on the one hand and on the other do software archeology to reconstruct the background behind this method: LE.tagAsHavingIgnoredMandatoryErrors(int) and this is my best recollection:

The core issue seems to be:

* analyzeCode needs to be called during lambda resolution to compute thrown exceptions, this is one of earliest instances of Java 8 twisting and contorting our compiler pipeline.
* JDT pipeline was set up so that if there are resolution errors, flow analysis would never be triggered, but the above requirement upends this nice property
* analyseCode invocation on the lambda body is akin to a minefield - for two reasons. If there are resolution errors, various ASTNode fields may be in uninitialized or partly cooked state and the code in analysis phase may trigger various exceptions (NPE ...) due to this partly cooked state.
(See https://bugs.eclipse.org/bugs/show_bug.cgi?id=422516 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=422489)
Secondly, we begin the flow analysis right at the beginning of the lambda without having analyzed the enclosing context.

Per https://bugs.eclipse.org/bugs/show_bug.cgi?id=422489#c2 at some point,

An earlier draft of JSR 335 required lambda bodies to be analyzed over and
over for every potentially applicable target type during overload resolution
and an error encountered in the lambda body upon the imposition of the target
type would disqualify that type (i.e the method whose suitable positional
parameter the target type was) and it would be declared incompatible and
eliminated from further reckoning.

and

The latest draft has eliminated this stipulation and (non-structural) errors in
lambda bodies do not influence the overload resolution.

The method LE.tagAsHavingIgnoredMandatoryErrors(int) seems to have been created when the "earlier draft" was in effect and was secondarily overloaded to short circuit exception analysis in the presence of resolve errors.

I am wondering if that short circuting is really necessary since

* we suppress all errors implementing a silent error handling policy while flow analyzing
* also we catch any exceptions that may result from analyzeCode () and silently drop them.

So the present fix is to get rid of the machinery that short circuits the lambda exception analysis. The fact that the offending test case passes with this and no other ill effect is observed suggests, (a) silent error handling and (b) catching and dropping exceptions is enough of a safety net and there is no occasion/call to bypass exception analysis altogether in the presence of resolution errors (which is deleterious in the offending program's case)

Let me know what you think.

(A plausible approach would have been to bite the bullet and run analysis even in the presence of resolve errors and fix defuse all minefield - I don't think we had the luxury of doing that in Java 8 time frame. Alternately, as was done with org.eclipse.jdt.internal.compiler.ast.Statement.doesNotCompleteNormally() implement a completely parallel mechanism which again would have been prohibitively expensive against human resources available then)

[srikanth-sankaran]

@stephan-herrmann If you have time this is marked for your review for M1. Thanks.

[stephan-herrmann]

(@srikanth-sankaran I have seen your nice reasoning, why your change should be good. For now I'm intentionally ignoring that reasoning, as to try to provide an orthogonal view, please bear with me)

How do we prove, that a particular piece of code is unnecessary, in particular, how can I assert this after not having been involved in its creation in the first place? :)

To see if I have a chance to step through relevant code paths (before the removal), I gathered this little statistic from LambdaExpressionsTest plus LambdaRegressionTest:

Which tests skip LE.analyseExceptions() due to a mandatory error:

• LambdaRegressionTest.testBug472648

Which test cases trigger LE.getThrownExceptions() (where the result of analysis is used):

• LambdaExpressionsTest.testGH1162
• LambdaExpressionsTest.testGH1162_2

So, the set of test cases where analysis is skipped despite being used downstream is the empty set.

Can we construct a test case that combines both properties? Yes, by adding notExist(); into the inner lambda of foo() of testGH1162, I can cause skipping of analyseExceptions(). As a result, some invocations of getThrownExceptions() will answer the empty set instead of a singleton set containing the binding for java.io.IOException. The resulting difference is: normally we would generate this constraint: ⟨java.io.IOException <: X#3⟩ but in presence of the mandatory error we don't.

Without the mandatory error, we then produce the "interesting" constraint ⟨java.io.IOException <: java.lang.Throwable⟩ :) and also infer ThrowingConsumer<java.nio.file.Path,java.io.IOException> throwingConsumer(ThrowingConsumer<java.nio.file.Path,java.io.IOException>)

By contrast, in presence of the mandatory error we infer ThrowingConsumer<java.nio.file.Path,java.lang.RuntimeException> throwingConsumer(ThrowingConsumer<java.nio.file.Path,java.lang.RuntimeException>)

So, yay, there is a difference with/without the mandatory error - internally. The test outcome still doesn't change, because inferring RuntimeException instead of IOException does no harm in this test case.

I dare say that inference of thrown exceptions isn't sufficiently tested in our suite for using this as the guide for bug fixes :(

Assuming that corresponding test cases were mainly added as bug reports were coming in (and thus the result is fairly random), can we improve on this by systematically creating tests challenging this particular area? I must admit, I don't have a good intuition, where and how exception inference is needed, and what are its distinguishing parameters.

[stephan-herrmann]

Just a sign of background activity: I'm still trying to create a better understanding of situations where inference of thrown exceptions is relevant, and how this could be disturbed by shape analysis with half-cooked information.

[stephan-herrmann]

To reduce the problem at hand I outsourced my attempts to come to terms with exception inference in general into #2198.

[stephan-herrmann]

Trying to trace this change:

An earlier draft of JSR 335 required lambda bodies to be analyzed over and
over for every potentially applicable target type during overload resolution
and an error encountered in the lambda body upon the imposition of the target
type would disqualify that type (i.e the method whose suitable positional
parameter the target type was) and it would be declared incompatible and
eliminated from further reckoning.

I came across the following from JSR 335 0.6.2 (15.27.3):

A lambda expression is congruent with a function descriptor if the following are true:

• ...
• Where the lambda parameters are assumed to have the same types as the descriptor parameter types:
  • There are no compile-time errors in the lambda body, except for (possibly) the exception checking errors specified by 11.2.3.
    ...

@srikanth-sankaran is this what the implementation was originally based upon?

With this reference I can confirm that this rule has been dropped without any replacement. Compile errors in a lambda body are of no significance for type inference! Hooray! 🍾

[stephan-herrmann]

After meandering around the issue, one final discussion about what could go wrong:

• the compiler could crash? No, with ignoring problems and catching exceptions I see no way how this code could possibly harm anyone
• exception analysis vis-a-vis a non-final target type could go wild? This wouldn't matter, since the effect, LE.thrownExceptions is only ever used during invocation type inference, i.e., after applicability inference, i.e., after all target types but one have been eliminated.
• exception analysis with disabled problem reporting could swallow flow errors, or could leave unwanted side effects influencing the final flow analysis -> back to the debugger trying to explain this away.

[stephan-herrmann]

exception analysis with disabled problem reporting could swallow flow errors, or could leave unwanted side effects influencing the final flow analysis

The copy on which we run analyseExceptions() is not part of the actual AST. Final flow analysis will run on the original lambda AST which is not tainted by any of this magic.

Last attempt to break it: what are the side effects caused by flow analysis:

1. local FlowInfo, FlowContext not a problem 👍
2. local bits in ASTNode.bits, not a problem, as that's only the AST copy 👍
3. synthetics in outer scopes / types, for outer emulation, synthetic arguments etc.

Can it happen that along the lines of (3) analyseExceptions() will leave side effects outside the copy, which could have an ill effect on the final flow analysis? @srikanth-sankaran, do you have a ready answer for this, or should we defer this to another issue?

[srikanth-sankaran]

exception analysis with disabled problem reporting could swallow flow errors, or could leave unwanted side effects influencing the final flow analysis

The copy on which we run analyseExceptions() is not part of the actual AST. Final flow analysis will run on the original lambda AST which is not tainted by any of this magic.

Last attempt to break it: what are the side effects caused by flow analysis:

1. local FlowInfo, FlowContext not a problem 👍
2. local bits in ASTNode.bits, not a problem, as that's only the AST copy 👍
3. synthetics in outer scopes / types, for outer emulation, synthetic arguments etc.

Can it happen that along the lines of (3) analyseExceptions() will leave side effects outside the copy, which could have an ill effect on the final flow analysis? @srikanth-sankaran, do you have a ready answer for this, or should we defer this to another issue?

My preference is to deal with this if/when it arises - so we have a concrete case to dissect/inspect etc. The present change inasmuch as simply pulls out the vestiges of a withdrawn clause of spec appears to move the needle in the right direction.

[stephan-herrmann]

My preference is to deal with this if/when it arises - so we have a concrete case to dissect/inspect etc. The present change inasmuch as simply pulls out the vestiges of a withdrawn clause of spec appears to move the needle in the right direction.

I captured this topic in #2208

[stephan-herrmann]

I tried hard to find reason against this change and found none :)

Please go ahead.

[srikanth-sankaran]

My preference is to deal with this if/when it arises - so we have a concrete case to dissect/inspect etc. The present change inasmuch as simply pulls out the vestiges of a withdrawn clause of spec appears to move the needle in the right direction.

I captured this topic in #2208

Thanks, I will assign this to myself although I will likely take it up for work only when a problem surfaces that may be likely be linked to it.