Bug 329971

Comment 1 Dennis Hendriks 2010-11-11 02:37:04 EST

Created attachment 182878 [details]
Test ecore

Comment 2 Dennis Hendriks 2010-11-11 02:37:20 EST

Created attachment 182879 [details]
Ecore diagram for the test ecore

Comment 3 Dennis Hendriks 2010-11-11 02:37:32 EST

Created attachment 182880 [details]
Genmodel for the test ecore

Comment 4 Dennis Hendriks 2010-11-11 02:37:57 EST

Created attachment 182881 [details]
Test model

Comment 5 Dennis Hendriks 2010-11-11 02:38:12 EST

Created attachment 182882 [details]
Test QVTo transformation

Comment 6 Sergey Boyko 2013-05-15 08:15:50 EDT

Created attachment 231007 [details]
Patch which remediates the bug

JUnit test script:

modeltype mm_test "strict" uses 'http://test1/1.0';
modeltype Ecore uses ecore('http://www.eclipse.org/emf/2002/Ecore');

transformation bug329971(in inModel : mm_test);

main() {
    var cls = inModel.rootObjects()![Class1];
    var x: EString = cls.attr1;
    var y: String = cls.attr1;
    var z: TestIdentifier = cls.attr1;

    assert fatal (cls.attr1 = "TestValue");
    assert fatal (x = "TestValue");
    assert fatal (y = "TestValue");
    assert fatal (z = "TestValue");
}

Comment 7 Sergey Boyko 2013-05-20 11:16:33 EDT

Pushed to master for Kepler RC1.

Commit ID: 880fd0f11e0dd007610731c0877a5b94dbfa40a3

Comment 8 Christopher Gerking 2013-06-26 09:03:12 EDT

Sergey, you went for a "blacklist" approach in your patch, enumerating all conditions that make an assertion fail. Wouldn't it be more sensible to specify a "whitelist" here, i.e. fail whenever the evaluation result is not equal to Boolean.TRUE? This would reduce the amount of specified conditions, covering additional cases like a 'null' evaluation result (which should also make the assertion fail IMO).

Anyway, good to see this addressed by you. I fell into this trap for bug 302594 when the invalid result did not lead to a failing assertion.

Comment 9 Sergey Boyko 2013-06-27 15:49:28 EDT

(In reply to comment #8)
> Sergey, you went for a "blacklist" approach in your patch, enumerating all
> conditions that make an assertion fail. Wouldn't it be more sensible to
> specify a "whitelist" here, i.e. fail whenever the evaluation result is not
> equal to Boolean.TRUE? This would reduce the amount of specified conditions,
> covering additional cases like a 'null' evaluation result (which should also
> make the assertion fail IMO).
> 

Hi Christopher,

You mean to change condition of assert from
  Boolean.FALSE.equals(assertionValue) || assertionValue == getInvalid()
to
  !Boolean.TRUE.equals(assertionValue)

Actually they are completely identical since only OclExpression which evaluates to Boolean type is permitted. Condition expression can't be evaluated to 'null' (expression "assert (null)" is not possible).

I'm agree that second variant looks more elegant but first variant is more specific - condition evaluates to Boolean but an exception may occur during evaluation (implicitly converts to 'invalid').

Comment 10 Christopher Gerking 2013-08-12 03:20:49 EDT

Are they really identical?

The following code should IMO make the assertion fail, but it does not:


main() {
   assert fatal (foo());
}

query foo() : Boolean {
   return null;	
}

Comment 11 Christopher Gerking 2013-08-19 05:35:08 EDT

Reopening to foreground my latest counter example.

Comment 12 Sergey Boyko 2013-08-19 06:43:58 EDT

(In reply to comment #11)
> Reopening to foreground my latest counter example.

Ok, let it evaluates this ways.

Personally I don't like such tri-state logic of 'Boolean' and prefer clear abstraction of 'boolean' type.

Comment 13 Sergey Boyko 2013-08-19 06:46:11 EDT

Pushed to master.

Commit ID: 62674b5761b8a18d618ccef46fbae404fe9293db