[WIP][Experimental][Known to fail] Rewire lambda expression in the proper place in the grammar hierarchy

[srikanth-sankaran]

• Lambdas are not primary expressions, but are top level expressions
• Align grammar productions to match with JLS
• Note: All the scanner and parser magic handshake via Vanguard parser and various synthetic tokens are untouched. (so far)

[srikanth-sankaran]

The PR as of now has this grammar change - this aligns perfectly with JLS and is LALR(1) (because the various synthetic harbinger terminal tokens that continue to be injected)

diff --git a/org.eclipse.jdt.core.compiler.batch/grammar/java.g b/org.eclipse.jdt.core.compiler.batch/grammar/java.g
index 309a224..5435370 100644
--- a/org.eclipse.jdt.core.compiler.batch/grammar/java.g
+++ b/org.eclipse.jdt.core.compiler.batch/grammar/java.g
@@ -1751,7 +1751,6 @@
 --                   Start of rules for JSR 335
 -----------------------------------------------------------------------
 
-PrimaryNoNewArray -> LambdaExpression
 PrimaryNoNewArray -> ReferenceExpression
 /:$readableName Expression:/
 
@@ -2083,6 +2082,17 @@
 /.$putCase consumeCastExpressionLL1WithBounds(); $break ./
 CastExpression ::= PushLPAREN Name Dims AdditionalBoundsListOpt PushRPAREN InsideCastExpression UnaryExpressionNotPlusMinus
 /.$putCase consumeCastExpressionWithNameArray(); $break ./
+--------
+CastExpression ::= PushLPAREN Name OnlyTypeArgumentsForCastExpression Dimsopt AdditionalBoundsListOpt PushRPAREN InsideCastExpression LambdaExpression
+/.$putCase consumeCastExpressionWithGenericsArray(); $break ./
+CastExpression ::= PushLPAREN Name OnlyTypeArgumentsForCastExpression '.' ClassOrInterfaceType Dimsopt AdditionalBoundsListOpt PushRPAREN InsideCastExpressionWithQualifiedGenerics LambdaExpression
+/.$putCase consumeCastExpressionWithQualifiedGenericsArray(); $break ./
+CastExpression ::= PushLPAREN Name PushRPAREN InsideCastExpressionLL1 LambdaExpression
+/.$putCase consumeCastExpressionLL1(); $break ./
+CastExpression ::=  BeginIntersectionCast PushLPAREN CastNameAndBounds PushRPAREN InsideCastExpressionLL1WithBounds LambdaExpression
+/.$putCase consumeCastExpressionLL1WithBounds(); $break ./
+CastExpression ::= PushLPAREN Name Dims AdditionalBoundsListOpt PushRPAREN InsideCastExpression LambdaExpression
+/.$putCase consumeCastExpressionWithNameArray(); $break ./
 /:$readableName CastExpression:/
 
 AdditionalBoundsListOpt ::= $empty
@@ -2188,6 +2198,8 @@
 ConditionalExpression -> ConditionalOrExpression
 ConditionalExpression ::= ConditionalOrExpression '?' Expression ':' ConditionalExpression
 /.$putCase consumeConditionalExpression(OperatorIds.QUESTIONCOLON) ; $break ./
+ConditionalExpression ::= ConditionalOrExpression '?' Expression ':' LambdaExpression
+/.$putCase consumeConditionalExpression(OperatorIds.QUESTIONCOLON) ; $break ./
 /:$readableName Expression:/
 
 AssignmentExpression -> ConditionalExpression
@@ -2195,7 +2207,7 @@
 /:$readableName Expression:/
 /:$recovery_template Identifier:/
 
-Assignment ::= PostfixExpression AssignmentOperator AssignmentExpression
+Assignment ::= PostfixExpression AssignmentOperator Expression
 /.$putCase consumeAssignment(); $break ./
 /:$readableName Assignment:/
 
@@ -2238,6 +2250,8 @@
 -- has been reduced.
 Expression ::= AssignmentExpression
 /.$putCase consumeExpression(); $break ./
+Expression ::= LambdaExpression
+/.$putCase consumeExpression(); $break ./
 /:$readableName Expression:/
 /:$recovery_template Identifier:/
 
@@ -2843,6 +2857,7 @@
 /:$readableName Expression:/
 
 Expression_NotName -> AssignmentExpression_NotName
+Expression_NotName -> LambdaExpression
 /:$readableName Expression:/
 -----------------------------------------------
 -- 1.5 features : end of generics
@@ -3119,3 +3134,4 @@
 
 $end
 -- need a carriage return after the $end
+

[srikanth-sankaran]

An earlier very partial attempt at this can be seen at this at the abandoned PR #1750

[srikanth-sankaran]

@laeubi @iloveeclipse - for this kind of a report : https://github.com/eclipse-jdt/eclipse.jdt.core/pull/4667/checks?check_run_id=56905329431 where would I access the differing class files ? It is not immediately obvious to me - Thanks for any pointers

When the class file comparison fails on an I-build, we get an archive of differing files. Can I get similar data for CI builds ?

[iloveeclipse]

You have to click on the "Details" of jenkins/pr-head entry above, and there on the Jenkins build number:

https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4667/9/

[srikanth-sankaran]

You have to click on the "Details" of jenkins/pr-head entry above, and there on the Jenkins build number:

https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4667/9/

Thanks - that is useful.

For posterity, this is what I had to do:

From the main PR page, I clicked the failing check Compiler. This took me to https://github.com/eclipse-jdt/eclipse.jdt.core/pull/4667/checks?check_run_id=56905329431
From that page I clicked View more details on Jenkins - Eclipse JDT to be taken to https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4667/9/eclipse/
From this page, if I click on Status on the top left, I get to see the comparator failing files.

[srikanth-sankaran]

OK, those comparator differences are expected and don't account for the 477 tests failures seen in CI. These tests are not failing locally, so something mysterious is happening here

[srikanth-sankaran]

OK, those comparator differences are expected and don't account for the 477 tests failures seen in CI. These tests are not failing locally, so something mysterious is happening here

Not only are these tests not failing locally - they don't fail in an inner eclipse instance launched from a JDT-Core development workspace and into which all jdt source and tests projects are imported as source projects.

Plot thickens 🤔

[srikanth-sankaran]

Here is one interesting observation - all failing tests are from the package org.eclipse.jdt.core.tests.compiler.parser and these tests fail only in 1.8 mode and seemingly pass in all other compliance modes -

[iloveeclipse]

Just a guess

• something local is not committed
• local build does not build (or clean) same bits like remote
• execution of tests in Eclipse uses different arguments

You could run build and tests locally by maven to verify that.

[srikanth-sankaran]

Just a guess

• something local is not committed
• local build does not build (or clean) same bits like remote
• execution of tests in Eclipse uses different arguments

You could run build and tests locally by maven to verify that.

I would/could readily rule out the first two, the maven run suggestion I will try after resorting to some easier (for me) experiments such as rolling back the last change - these failures started suddenly (earlier there were failures but along expected lines and locally reproducible). I don't see how the last change itself could be connected but it would be a worthwhile exercise to roll back to a cleaner state and move forward.

(note to self - I need to stop amending previous commits 😧)

[srikanth-sankaran]

Backing out the recent change still produces the same 477 AIOOBs - I am 100% certain these failures were NOT there before the last one line change to the grammar file - I suspect some flakiness in github CI infra

To check all is well, I created a dummy PR #4669 - that fails with:

15:15:13 org.eclipse.jdt.core.tests.compiler.regression.LocalStaticsTest.testBug564557AnnotInterface_001 - 21
Creating placeholder flownodes because failed loading originals.
ERROR: Cannot resume build because FlowNode 34 for FlowHead 1 could not be loaded. This is expected to happen when using the PERFORMANCE_OPTIMIZED durability setting and Jenkins is not shut down cleanly. Consider investigating to understand if Jenkins was not shut down cleanly or switching to the MAX_SURVIVABILITY durability setting which should prevent this issue in most cases.
[GitHub Checks] GitHub check (name: Jenkins, status: completed) has been published.

[mpalat]

Updating with some related info on an offline conversation from Srikant:

• Artifact here
• log here
• Launcher Command-line link

Additional (maybe useless) info - @srikanth-sankaran : I did a parser generation of the "new" java.g from the Pulled PR. PR looks good - no omissions/delta after the generation as well - this was just to double check. Tests are passing on my machine.

Note: Just did a rebuild as well in case it turns out to be a timeout issue

[srikanth-sankaran]

You could run build and tests locally by maven to verify that.

This calls for a bit more set up and skills than I have readily at my disposal :)

@mpalat @jarthana and I did some brainstorming and decided to take the equivalent route of downloading the built bits from github CI infra itself. From https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4667/15/artifact/repository/target/, download the zipped bits, explode them and into a clean workspace import these as binary projects with linked content under import plugins and fragments.

This establishes the JDT projects in a debuggable workspace exactly incorporating the bits from jenkins.

Now I imported the test projects from the target platform and ran the failing tests and voila! I was able to reproduce the problem.

Now here is the thing that surprised me totally. When the build reports a class file comparator failure, I always assumed that it is flagging a unchanged source file that has resulted in a changed class file that needed to be validated for the quality gate check to pass and for the PR to be integrated. I had paid scant attention to the part of the diagnostic about the concerned artifact being changed and replaced with the baseline version!!! That step in the present instance replaced valid new changed class file (change due to externally defined inlined constant) with a version with the stale value of the constant.

So when class file differences are reported in the checks steps, the required protocol seem to be:

• ignore test results (for the time being)
• validate class file change, if warranted force qualifier update, trigger a rebuild and wait for fresh test result

as opposed to:

• validate class file change
• Use test results as a further validation that the new code generation is good ---- This is WRONG assumption because it is not testing the new code generation but baseline version of class file for source files unchanged in the PR.

That the logs contain a very clear message about the artifact being replaced with baseline version is undeniably true, but the assumption that a class file can change only when the corresponding source has changed is a very brittle one!

[srikanth-sankaran]

The tests will take a while to finish but it is good news that I see some of the tests were failing earlier with AIOOB now passing

[srikanth-sankaran]

The tests will take a while to finish

Hoorah! All green - this basically refactors the lambda grammar to align closer with JLS. But would/could such alignment result in some simplifications (such as eliminating synthetic terminal injection or vanguard parser detours) - that is to be taken up next week!

[srikanth-sankaran]

Thanks @iloveeclipse @mpalat @jarthana for your tips and inputs in reproducing the failures seen in Jenkins. Appreciated!

[srikanth-sankaran]

Now here is the thing that surprised me totally. When the build reports a class file comparator failure, I always assumed that it is flagging a unchanged source file that has resulted in a changed class file that needed to be validated for the quality gate check to pass and for the PR to be integrated. I had paid scant attention to the part of the diagnostic about the concerned artifact being changed and replaced with the baseline version!!! That step in the present instance replaced valid new changed class file (change due to externally defined inlined constant) with a version with the stale value of the constant.

So when class file differences are reported in the checks steps, the required protocol seem to be:

• ignore test results (for the time being)
• validate class file change, if warranted force qualifier update, trigger a rebuild and wait for fresh test result

as opposed to:

• validate class file change
• Use test results as a further validation that the new code generation is good ---- This is WRONG assumption because it is not testing the new code generation but baseline version of class file for source files unchanged in the PR.

That the logs contain a very clear message about the artifact being replaced with baseline version is undeniably true, but the assumption that a class file can change only when the corresponding source has changed is a very brittle one!

@stephan-herrmann - I checked with @mpalat and @jarthana about this - they confess to being as surprised as I am. What is your reaction ?

[stephan-herrmann]

@stephan-herrmann - I checked with @mpalat and @jarthana about this - they confess to being as surprised as I am. What is your reaction ?

Already in the past I have been confused about the various baseline-related checks performed by the build. Your latest observation adds to that confusion :)

My conclusion: we need to enhance our understanding of what the build does and why.

• Looking at the build overview like https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4667/16/ is not enough, it doesn't show the crucial "has been replaced" message!
• In this case it's the execution of goal tycho-p2-plugin:p2-metadata-default that breaks the build
  • It's the parameter <baselineReplace> that seems to trigger the unwanted replacement.
    • IIUC the intention is exclusively to optimize the final p2 metadata produced by the build.
    • From that description it's a long way to understanding why it could possibly affect tests!!!!
    • I recall being confused by this replacement specifically in builds for BETA_ branches, though I have no exact data here.
    • Perhaps we can disable this behavior for CI builds?
• The above shouldn't be confused with other goals like tycho-p2-extras:compare-version-with-baselines