Invalid code not rejected by compiler (Duplicate of #367)

[trancexpress]

See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=481133

Snippet:

import java.util.function.Consumer;
import java.util.function.Supplier;

/**
 * CFSXXX
 * 
 * @author connors
 */
public class CFSXXX {

    public static void main(String[] args) {
        boolean mybool_XXXWhoopsGotTheWrongNameHereXXX = true;
        if (mybool) {
            System.out.println("within if - true part");
            setSupplier(() -> x -> System.out.println("x" + x); );
        } else {
            System.out.println("within if - false part");
        }
    }
    
    public static void setSupplier(Supplier<Consumer<String>> supplier) {
        System.out.println("setSupplier called: " + supplier);
    }
}

This code should not compile, but does with Eclipse. E.g. javac reports errors:

CFSXXX.java:16: error: ')' expected
            setSupplier(() -> x -> System.out.println("x" + x); );
                                                              ^
CFSXXX.java:16: error: illegal start of expression
            setSupplier(() -> x -> System.out.println("x" + x); );
                                                                ^
2 errors

[trancexpress]

First ecj version that supports -source 1.8 is 4.4, the bug already exists there. So not a regression.

The snippet without errors (that can be compiled) is:

import java.util.function.Consumer;
import java.util.function.Supplier;

public class CFSXXX {

    public static void main(String[] args) {
        boolean mybool= true;
        if (mybool) {
            System.out.println("within if - true part");
            setSupplier(() -> x -> System.out.println("x" + x) );
        } else {
            System.out.println("within if - false part");
        }
    }
    public static void setSupplier(Supplier<Consumer<String>> supplier) {
        System.out.println("setSupplier called: " + supplier);
    }
}

Reduced snippet that breaks the parser:

public class X {
	public interface C<T> {
	    T c(T t);
	}
	public interface S<T> {
	    T s();
	}
    public void f() {
    	S<C<Integer>> s = () -> x -> x  ) ;
    	boolean b;
    }
}

Its not exactly the nested lambda that breaks the parser, but the nested lambda followed by );.

This commit seems oddly matching the problem here: 6a1671a

It adds this code:

if (insertedToken == TokenNameElidedSemicolonAndRightBrace) {
    /* https://bugs.eclipse.org/bugs/show_bug.cgi?id=383046, we should never ever report the diagnostic, "Syntax error, insert ElidedSemicolonAndRightBraceto complete LambdaBody"
       as it is a synthetic token. Instead we should simply repair and move on. See how the regular Parser behaves at Parser.consumeElidedLeftBraceAndReturn and Parser.consumeExpression.
       See also: point (4) in https://bugs.eclipse.org/bugs/show_bug.cgi?id=380194#c15
    */
    break;
}

The break prevents problem reporting in the snippet above... After the problem in the lambda line, the code doesn't report any problems. This is due to the code here:

"org.eclipse.jdt.internal.ui.text.JavaReconciler" #108 daemon prio=1 os_prio=0 cpu=3915.72ms elapsed=11639.94s tid=0x00007f50d704dbd0 nid=0x2bdd at breakpoint [0x00007f4ee45fb000]
   java.lang.Thread.State: RUNNABLE
        at org.eclipse.jdt.internal.compiler.parser.Parser.parse(Parser.java:13059)
        at org.eclipse.jdt.internal.compiler.parser.Parser.parse(Parser.java:13512)
        at org.eclipse.jdt.internal.compiler.ast.MethodDeclaration.parseStatements(MethodDeclaration.java:251)
        at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.parseMethods(TypeDeclaration.java:1143)
        at org.eclipse.jdt.internal.compiler.parser.Parser.getMethodBodies(Parser.java:12174)
        at org.eclipse.jdt.internal.compiler.SourceElementParser.parseCompilationUnit(SourceElementParser.java:1138)
        at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:273)
        at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:193)
        at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:266)
        at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:597)
        at org.eclipse.jdt.internal.core.CompilationUnit.makeConsistent(CompilationUnit.java:1148)
        at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:173)
        at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:94)
        at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:740)
        at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:806)
        at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1325)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:132)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:94)
        at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:91)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:158)
        at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.reconcile(CompositeReconcilingStrategy.java:94)
        at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.reconcile(JavaCompositeReconcilingStrategy.java:107)
        at org.eclipse.jface.text.reconciler.MonoReconciler.process(MonoReconciler.java:78)
        at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:207)

After Parser.hasError is set, some recovery code runs which results in ignoring statements around the lambda line.

Removing the break statement results in a problem reported for the snippet. Same for the original reproduction snippet.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=383046 was merged with the beta branch for Java 8 support (oddly into 3.8), which should explain why I didn't find a working ecj version (w.r.t. this bug).

As written in the comment, recovery takes place after the break and the recovered statement "doesn't know" about the closing bracket error. The recovered element is:

S<C<Integer>> s = () -> x -> x 

If the recovered statement itself has errors, those errors are reported (e.g. x -> z).

So the ) error seems to be get swallowed, but because its detected no further errors in the scope are reported...

[trancexpress]

I don't really understand the comment above in the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=383046 ...

we should never ever report the diagnostic, "Syntax error, insert ElidedSemicolonAndRightBraceto complete LambdaBody"

For this snippet:

public class X {
	public interface S<T> {
	    T s(T t);
	}
    public void f() {
    	S<?> s = v -> v ) ;
    	{} // empty block to force scope handling for the lambda line
    }
}

ecj reports:

----------
1. ERROR in /data/workspaces/runtime-Eclipse/TestBug481133/src/X.java (at line 6)
        S<?> s = v -> v ) ;
                        ^
Syntax error on token ")", ElidedSemicolonAndRightBrace expected
----------
1 problem (1 error)

Here the SUBSTITUTION_CODE branch is taken (value of SecondaryRepairInfo.code)... Unlike SCOPE_CODE it apparently doesn't care about ElidedSemicolonAndRightBrace?

Also is text like this expected in an error message from a compiler? And is the error above much different to the one from the comment?

See also the starting comment in ProblemReporter.replaceIfSynthetic(String):

Java 8 grammar changes use some synthetic tokens to make the grammar LALR(1). These tokens should not be exposed in messages as it would make no sense to the programmer whatsoever. Replace such artificial tokens with some "suitable" alternative. At the moment, there are two synthetic tokens that need such massaging viz : "BeginLambda" and "BeginTypeArguments". There is a third synthetic token "ElidedSemicolonAndRightBrace" that we don't expect to show up in messages since it is manufactured by the parser automatically.

Opened #903.

[trancexpress]

@srikanth-sankaran , first and foremost, great to see you are working again on JDT!

If you have time in future, it would be great if you can take a look here, as well as #125, #367 and #904. I believe those issues are all closely related, if not one and the same bug.

[srikanth-sankaran]

@srikanth-sankaran , first and foremost, great to see you are working again on JDT!

If you have time in future, it would be great if you can take a look here, as well as #125, #367 and #904. I believe those issues are all closely related, if not one and the same bug.

Yes, @trancexpress, I will take a look at these in due course. Thanks for the pointers

[srikanth-sankaran]

This is the same issue as #367 - My candidate fix for that ticket handles the code snippets here correctly - as in rejects them. I will include the test case from here in my patch for #367 and add my observations there.

[srikanth-sankaran]

@trancexpress, some explanations:

(1) "we should never ever report the diagnostic, "Syntax error, insert ElidedSemicolonAndRightBrace to complete LambdaBody""

There are two reasons for this comment:

(i) ElidedSemicolonAndRightBrace is an internal synthetic token. A programmer can do NOTHING on his/her accord to insert these tokens at the indicated place. So including such tokens in messages addressed to the users is wasteful, confusing, not actionable and needlessly exposing internal implementation details.

(2) The above is also entirely true of the synthetic tokens handled by ProblemReporter.replaceIfSynthetic(String) but ElidedSemicolonAndRightBrace is special for one crucial difference: ElidedSemicolonAndRightBrace is synthesized and inserted right after a non-block lambda, on the fly by the Parser in normal parsing itself - OIOW, normal parsing itself does some amount of "repair" of the input stream to reduce a lambda expression. The other synthetic tokens are inserted into the stream by the scanner but ElidedSemicolonAndRightBrace is not returned by the scanner (See point 4 in https://bugs.eclipse.org/bugs/show_bug.cgi?id=380194#c15)

When the DiagnoseParser is coming up with a recommendation to " insert ElidedSemicolonAndRightBrace to complete LambdaBody" - it is likely mimicking the Parser's own repair of the input stream done at

org.eclipse.jdt.internal.compiler.parser.Parser.consumeElidedLeftBraceAndReturn() and
org.eclipse.jdt.internal.compiler.parser.Parser.consumeExpression()

If that is all there is to it (the assumption when I short circuited that error emission with a break), things would be fine. I think what I overlooked is that DiagnoseParser has its own complicated repair mechanism which can sometimes skip ahead past several tokens until it reaches a certain point of predictability. This skipping past subsequent tokens results in subsequent errors to be not reported and with my code short circuiting the initial error ( insert ElidedSemicolonAndRightBrace to ...) no error shows up leading to the impression that the code is compiled fine.

I am testing a candidate fix that would (a) not expose internal non-actionable synthetic tokens but (b) will still surface a generic/bland error message that is "quite correct"

First order business being to reject invalid programs correctly, quality of diagnostics being secondary to correctness.

I'll wait until #1045 to make any improvements to quality of diagnostics.
Some of these parsing magic may be gotten rid of - who knows.

[srikanth-sankaran]

I don't really understand the comment above in the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=383046 ...

[...]

Here the SUBSTITUTION_CODE branch is taken (value of SecondaryRepairInfo.code)... Unlike SCOPE_CODE it apparently doesn't care about ElidedSemicolonAndRightBrace?

That ElidedSemicolonAndRightBrace shows up in diagnostics emitted by a different path is an oversight/bug. Whether it is SUBSTITUTION_CODE path or SCOPE_CODE path, saying ElidedSemicolonAndRightBrace is expected or to be inserted etc in a message is not actionable by the user and will be confusing.

[srikanth-sankaran]

So in summary, injection of ElidedSemicolonAndRightBrace to reduce a lambda expression is normal part of business. So it makes sense to not show that in an error message.

What was a complete surprise (and hence resulted in this cluster of bugs) was that after thus repairing the input stream, the DiagnoseParser would have been expected to report "some" additional error as it chewed the input stream further. That it skipped further input was something (I and my) the code was not prepared for.

[trancexpress]

Alright, thank you for the explanations.

[srikanth-sankaran]

Here is an experiment that sheds light on how insertion of ElidedSemicolonAndRightBrace is "normal" part of business of parsing lambda expressions, be it by Parser or DiagnoseParser.

(1) Comment out the code:

if (insertedToken == TokenNameElidedSemicolonAndRightBrace) {
    /* https://bugs.eclipse.org/bugs/show_bug.cgi?id=383046, we should never ever report the diagnostic, "Syntax error, insert ElidedSemicolonAndRightBraceto complete LambdaBody"
	as it is a synthetic token. Instead we should simply repair and move on. See how the regular Parser behaves at Parser.consumeElidedLeftBraceAndReturn and Parser.consumeExpression.
        See also: point (4) in https://bugs.eclipse.org/bugs/show_bug.cgi?id=380194#c15
  */
	break;
}

(2) Now compiling this code:

interface IX {
    public void foo();
}
public class X {
    IX i = () -> 42;
    int x
}

results in the perfectly (syntactically) valid lambda being rejected:

1. ERROR in X.java (at line 5)
	IX i = () -> 42;
	             ^^
Syntax error, insert "ElidedSemicolonAndRightBrace" to complete LambdaBody

2. ERROR in X.java (at line 5)
	IX i = () -> 42;
	             ^^
Void methods cannot return a value
----------
3. ERROR in X.java (at line 6)
	int x
	    ^
Syntax error, insert ";" to complete FieldDeclaration

(It is worth investigating why the DiagnoseParser skips past several tokens resulting in no further error at all in the test cases for this cluster of duplicate defects)