eclipse / xtext

Eclipse Xtext™ is a language development framework
http://www.eclipse.org/Xtext
Eclipse Public License 2.0
769 stars 321 forks source link

Invalid generation of variable declarations against lambda declaration #2365

Open gallandarakhneorg opened 6 years ago

gallandarakhneorg commented 6 years ago

Let the DSL code:

import java.util.UUID
class MyClass {
    def void fct2((UUID) => boolean lambda) {
    }
    def void fct() {
        var UUID id
        id = UUID.randomUUID

        fct2 [it == id]
    }
}

The generated Java code causes the error: Java problem: Local variable id defined in an enclosing scope must be final or effectively final.

The expected behavior is: no error.

The generated Java code for the fct function is:

UUID id = null;
id = UUID.randomUUID();
final Function1<UUID, Boolean> _function = (UUID it) -> {
   return Boolean.valueOf(Objects.equal(it, id));
};
this.fct2(_function);

The expected Java code is:

UUID id;
id = UUID.randomUUID();
final Function1<UUID, Boolean> _function = (UUID it) -> {
   return Boolean.valueOf(Objects.equal(it, id));
};
this.fct2(_function);

The initialization to null of the local variable id is invalid within the Java code. And, it is not expected because it is not specified into the DSL code.

Possible solution: avoid the XbaseCompiler to generate initialization to the default value when declaring a local variable.

cdietrich commented 6 years ago

see https://bugs.eclipse.org/bugs/show_bug.cgi?id=378320 too

cdietrich commented 6 years ago

and (partially) https://bugs.eclipse.org/bugs/show_bug.cgi?id=434760

gallandarakhneorg commented 6 years ago

Thank you for the links.

I have already implemented part of the mentionned features regarding initialization and uses of the local variables in lambdas within my DSL.

For solving the current specific issue, I have changed the XbaseCompiler implementation in order to avoid implicitly initialization of the local variable within the Java code:

protected void _toJavaStatement(XVariableDeclaration varDeclaration, ITreeAppendable appendable, boolean isReferenced) {
    if (varDeclaration.getRight() != null) {
        internalToJavaStatement(varDeclaration.getRight(), appendable, true);
    }
    appendable.newLine();
    final LightweightTypeReference type = appendVariableTypeAndName(varDeclaration, appendable);
    if (varDeclaration.getRight() != null) {
        appendable.append(" = "); //$NON-NLS-1$
        internalToConvertedExpression(varDeclaration.getRight(), appendable, type);
    }
    appendable.append(";"); //$NON-NLS-1$
}

If you think this change will not have a bad impact, I could submit a PR.

cdietrich commented 6 years ago

@szarnekow what do you think?

szarnekow commented 6 years ago

My impression is that we should rather fix the validation here. From my understanding it should not be allowed to use the variable id within the lambda

gallandarakhneorg commented 6 years ago

The DSL code is valid, and follows the rules from Java: when the variable is not initialized in the XVariableDeclaration, the variable is considered a final and could be used within lambda.

Because XBasecompiler adds the initialization into the Java code, this Java code becomes invalid.

The problem seems that a false error is created within the Java code.

I agree to add specific error messages related to the usage of the variables within lambdas, but the example within the first post of this message should compile. That is not the case with the current XbaseCompiler implementation.

szarnekow commented 6 years ago

I'm afraid according to the spec. the DSL code is not valid: Any final variables and parameters visible at construction time can be referred to in the lambda expression’s body. (https://www.eclipse.org/Xtext/documentation/305_xbase.html#xbase-expressions-lambda) id is not final thus it may not be referenced.

gallandarakhneorg commented 6 years ago

I agree with @szarnekow, adding error message within the XbaseValidator should be done (replacing the error messages coming from the Java compiler).

But, there is still an open question because Xtext spec is not so precise as Java spec.

What is the position of Xtext regarding the "effectively final" variables? Variables that are declared with final modifier and variables that are not declared with the final modifier but never modified could be used within Java lambdas. They are considered as "effectively final".

See Listing 13 and related explanation at: http://www.oracle.com/technetwork/articles/java/architect-lambdas-part1-2080972.html

I think that the XbaseCompiler breaks the support of the "variables that are not declared with the final modifier but never modified" because of the implicit initialization within the generated code. It means that even if the XbaseValidator does not report error, the generated Java does not compiler with the example at the begining of this thread.

Do you think that Xtext spec should be more restrictive than Java spec? If yes, I agree that only the XbaseValidator should be changed. If not, both XbaseValidator and XbaseCompiler should be updated.

szarnekow commented 6 years ago

Xbase has no notion of effectively final. It is usually not necessary, since it is possible to richer expressions as the variable initializer, e.g. var x = if (cond) try { foo } catch(Exception e) bar else zonk. Of course this does not work if two or more variables should be initialized in such a code block, but in these cases it should be safe to create a wrapper object and store all values in one instance.

Nevertheless the generated code could be improved, but first and foremost the validation appears to be broken.

gallandarakhneorg commented 6 years ago

I agree with the plan to work on the validator first, and on the compiler if needed.

gallandarakhneorg commented 6 years ago

I would like to list the cases that may be unit tested.

Case 1: Expecting no error in the DSL

var v  = 4
fct2 [ it == v ]

Case 2: Expecting error on v within the lambda because it is not a final variable

var v  = 4
v = 6
fct2 [ it == v ]

Case 3: Expecting no error in the DSL, but the corresponding Java code will have errors -> an error should be output for by XbaseValidator.

var int v
v = 4
fct2 [ it == v ]

Case 4: Expecting no error in the DSL

val v  = 4
fct2 [ it == v ]

Case 5: Expecting error on v in lambda because it is not initialized.

var int v
fct2 [ it == v ]

Case 6: Expecting error on v within the lambda because it is not a final variable

var int v
v = 4
v = 5
fct2 [ it == v ]

The case that is not clear is the case 3.

If you see other cases, please let me know.

szarnekow commented 6 years ago

According to the spec for Xbase, the following expectations are to be used: Case 1: Expecting an error since we refer to a non-final local variable. Case 2: Expecting an error since we refer to a non-final local variable. Case 3: Expecting an error since we refer to a non-final local variable.

Case 4: Expecting no errors. Case 5: Expecting an error since we refer to a non-final local variable. Case 6: Expecting an error since we refer to a non-final local variable.

Please note that the syntax for local variable declarations is not var v:int but var int v.