eclipse-qvto / org.eclipse.qvto

Eclipse Public License 2.0
0 stars 1 forks source link

Return value of BlockExp #1044

Open eclipse-qvt-oml-bot opened 5 days ago

eclipse-qvt-oml-bot commented 5 days ago

| --- | --- | | Bugzilla Link | 540736 | | Status | NEW | | Importance | P3 normal | | Reported | Nov 03, 2018 10:14 EDT | | Modified | Nov 06, 2018 05:22 EDT | | Reporter | Christopher Gerking |

Description

Introducing a BlockExp (i.e., curly braces) inside an operation leads to a silent null return:

query getBool() : Boolean {\ return switch {\ case(true) {true}\ case(false) {false}\ }\ }

This operation will always return null because the boolean return values have been (accidentally) wrapped inside curly braces. This seems to be in accordance with the QVT specification which states that a BlockExp will always return null. But is this behavior really desired? In my opinion, it gives rise to horribly subtle bugs. I therefore suggest that a BlockExp should implicitly return the evaluation result of its last expression, as it is done for operation bodies as well.

eclipse-qvt-oml-bot commented 5 days ago

By Ed Willink on Nov 03, 2018 12:12

A BlockExp is always prefixed by "do", "object" or "switch", so you have wrapped an not a around your switch. An should probably have a value just as you indicate, so perhaps you got confused by terminology or perhaps you have uncovered an Eclipse QVTo bug.

OMG QVTo appears to be sound (this time). OMG QVTo certainly has some (BlockExp/block-expression) / expression-block confusion that may have leaked into Eclipse QVTo. It is weird that a with an explicit return does not propagate the return value. This should be fixed / have a WFR diagnosis.

I'm inclined to change to allowing an explicit return to return an explicit value, and to providing a warning if the implicit null result of a BlockExp is used.

eclipse-qvt-oml-bot commented 5 days ago

By Christopher Gerking on Nov 03, 2018 12:34

(In reply to Ed Willink from comment #1)

A BlockExp is always prefixed by "do", "object" or "switch"

No, not necessarily because the specification says: "However, when used within the following control expressions: if, switch, compute, and for expressions the do keyword can be skipped." (8.2.2.2 BlockExp) Skipping is exactly what Eclipse QVTo does here.

so you have wrapped an not a around your switch.

The problem is not the block around the switch, but the block around the boolean literal. This one is actually a BlockExp in Eclipse QVTo, I double-checked this at the AST level.

It is weird that a with an explicit return does not propagate the return value.

Eclipse QVTo does. At least that's my impression when looking at the respective code in QvtOperationalEvaluationVisitorImpl.visitBlockExpImpl(...).

An should probably have a value just as you indicate

Why should differ from in this respect? Anyway, I agree that a warning in case of a null return would at least be a helpful improvement.

eclipse-qvt-oml-bot commented 5 days ago

By Ed Willink on Nov 03, 2018 14:53

(In reply to Christopher Gerking from comment #2)

(In reply to Ed Willink from comment #1)

A BlockExp is always prefixed by "do", "object" or "switch" No, not necessarily because the specification says: "However, when used within the following control expressions: if, switch, compute, and for expressions the do keyword can be skipped." (8.2.2.2 BlockExp) Skipping is exactly what Eclipse QVTo does here.

This is where the spec has a (BlockExp/block-expression) / expression-block confusion. "if" etc use an expression-block not a block-expression.

An expression-block is an expression and so has a value.

A block-expression is a statement and so does not have value.

The grammar fails to distinguish as an OCL if...then...else...endif with values throughout by introducing a separate imperative with optional else and no return values. I raised an OMG issue long ago that QVTo is not an extension of Essential OCL as claimed. is a very clear example.

(In reply to Christopher Gerking from comment #2)

so you have wrapped an not a around your switch. The problem is not the block around the switch, but the block around the boolean literal. This one is actually a BlockExp in Eclipse QVTo, I double-checked this at the AST level.

Ah, yes but...

::= 'case' '(' ')' A switch case is a statement and so has no value. So you actually have two missing warnings a) "{true}" and "{false}" are imperative statements with no side effect. They are therefore dead code. b) "return switch" attempts to use the non-value of a switch statement. QVTo is designed around a clear side-effect free OCL expression, and a side-effecting statement. You risk creating obscure problems if you start to blur the statement/expression distinction. Ultimately a clean resolution depends on https://issues.omg.org/browse/QVT14-22 which is the residue of the textual fix in QVT 1.3 from https://issues.omg.org/browse/QVT13-8
eclipse-qvt-oml-bot commented 5 days ago

By Christopher Gerking on Nov 05, 2018 07:54

(In reply to Ed Willink from comment #3)

A block-expression is a statement and so does not have value.

Why? I see no law that restricts a side effect to not produce an additional value.

A switch case is a statement and so has no value.

It is an 'expression statement' which is either an expression or a non-empty 'expression block'. If such an 'expression block' does not have a value, how is it able to represent the 'when' clause of a mapping?

::= 'when' > So you actually have two missing warnings > > b) "return switch" attempts to use the non-value of a switch statement. If what you say above is true without any alternative, then "return switch" could even be an error because it should never return something useful. And the bug in Eclipse QVTo would be that it allows a switch to produce a value nevertheless.
eclipse-qvt-oml-bot commented 5 days ago

By Ed Willink on Nov 05, 2018 08:21

(In reply to Christopher Gerking from comment #4)

(In reply to Ed Willink from comment #3)

A block-expression is a statement and so does not have value. Why? I see no law that restricts a side effect to not produce an additional value.

It is a widespread language practice.

An expression is a computation that may be composed into a bigger expression, e.g. as the argument to an operation call.

A statement is something that performs a complete action. In many languages a statement ends in a ";". A statement does not 'return' anything, although special purpose statements such as a return statement may do something interesting with one of the expressions that contributes to the statement.

An expression by itself cannot be used until embedded in an expression-statement or ...

Since Java and C++ statements all end in ";" I cannot see how any Java or C++ statement can 'return' a value used by a composing statement. Unfortunately QVTo lacked the diversity of design expertise and so there are many poor design decisions that we have to try to make sense of.


It is certainly a bug to attempt to return the 'value' of a switch and it would be nice if Eclipse QVTo diagnosed it. Of course OMG does not specify what diagnosis tools should provide so the QVTo 'bug' is just a poor usability in Eclipse QVTo.

eclipse-qvt-oml-bot commented 5 days ago

By Christopher Gerking on Nov 06, 2018 04:58

(In reply to Ed Willink from comment #5)

It is a widespread language practice.

Maybe but for QVTo, the separation seems very vague, only indicated by the word "statement" somewhere inside the grammar? I also feel like the separation is not that easy to follow through, consider again the example of a 'when' clause. How can it provide a boolean result if its doesn't produce a value? Maybe one of the poor design decisions you mentioned above?

It is certainly a bug to attempt to return the 'value' of a switch and it would be nice if Eclipse QVTo diagnosed it.

Eclipse QVTo does not only attempt that, but actually does. So changing this would introduce regressions.

If I understand you right, the fundamental bug is that a switch produces a value. Then the following statement is potentially wrong: "Semantically it behaves almost as nested OCL if expressions." (8.2.2.8 SwitchExp) This is because OCL if expressions do produce values.

Depending on what kind of expression is used as body, a switch doesn't necessarily have side-effects: case(...) {true} vs. case(...) true

Thus wouldn't it be an option to allow a switch to have a value? I just like the current behavior because it makes very explicit the conditional return, leading to an improved code readability compared to multiple nested returns.

eclipse-qvt-oml-bot commented 5 days ago

By Ed Willink on Nov 06, 2018 05:22

"behaves almost as" is a sure sign of a specification author desperately avoiding the need to think through/explain a problem properly. The problem is chucked over the fence for a tool vendor to try to come up with a consistency and then for users to complain about tool inconsistencies / stupidities.

::= 'switch' ('(' ')')? \ ::= '{' + ? '}'\ ::= 'case' '(' ')' \ ::= 'else' demonstrates the mess. An magically evolves via a into a that can be used anywhere. Hence the new text in QVT 1.3 strongly restricting side-effecting ImperativeOCLExpression (a statement) in side-effect-free OCLExpression contexts. There is a plausible side-effect-free switch-expression that could be added to OCL. This must return a value; else is mandatory. There is a different side-effecting switch-statement that can be added by Imperative OCL. This does not return a value; else is optional. OMG QVTo is vague as to which is provided. IMHO since else is optional it is a switch-statement. Eclipse QVTo implementation is also vague offering the opportunities for the user confusion that underpins the original bug report. --- The simplest fix is to diagnose the inconsistent usage of switch as an expression. A more major fix would be to support both switch-exp and switch-stmt. You will encounter some horrible grammar ambiguities without a thorough disentangling of expression/statement. ("Remark: The concurrent usage of OCL-like syntax “if exp then body endif” and java-like syntax “if (exp) body …” may\ produce a grammar conflict in parsers which can, however, be solved through appropriate look-ahead." is another indication of a specification author too 'lazy'/unable to provide a formal explanation/solution.) Ultimately once you have a switch-exp, it will diagnose your original example as having an error through omission of an else clause.