round() method returns a float if called on a float

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 462386 | | Status | RESOLVED FIXED | | Importance | P3 normal | | Reported | Mar 17, 2015 12:02 EDT | | Modified | May 21, 2015 10:29 EDT | | Reporter | William Piers |

Description

When invoking [attribute.round()/], where :

• the attribute type is a custom EDataType of java type float
• the value is 100.0,\ Acceleo writes "100.0" instead of "100"

round() should return an Integer as its documentation indicates.

Doing [attribute.toString().toReal().round()/] or a service calling Math.round() workarounds the issue.

[eclipse-ocl-bot]

By Laurent Goubet on May 19, 2015 08:35

This operation is an OCL standard operation, and I believe the result you see is because the OCL (legacy) implementation only seems to care about Double, not Float. Any value that is neither a Double nor the unlimited literal will just be returned as-is by "round"

case PredefinedType.ROUND:\ if (sourceVal instanceof Double) {\ // Real::round()\ return (int) Math.round((Double) sourceVal);\ }\ if (sourceType == getUnlimitedNatural()) {\ long sourceInt = (Long) higherPrecisionNumber((Number) sourceVal);\ \ // the unlimited value can't be rounded\ if (sourceInt == UnlimitedNaturalLiteralExp.UNLIMITED) {\ return getInvalid();\ }\ }

// Integer::round()\
return sourceVal;

reaffecting to OCL.

[eclipse-ocl-bot]

By Ed Willink on May 19, 2015 09:16

Classic OCL has only limited support for Java types. In particular only Double for computation.

The bug is therefore a failure of the provider to convert the Float to Double, rather than a failure of the consumer to support Float.

(In the Pivot OCL all incoming numerics are converted to RealValue instances providing support for BigDecimnal too.)

Foxing all consumers in the Classic OCL code is not appropriate.

[eclipse-ocl-bot]

By Laurent Goubet on May 19, 2015 09:46

Closing as wontfix then. You will have to rely on your workaround converting to double before calling round as OCL will not support the legacy implementation any longer.

[eclipse-ocl-bot]

By Ed Willink on May 19, 2015 10:41

(In reply to Ed Willink from comment #2)\ Bouncing back to Acceleo too soon.

The bug is therefore a failure of the provider to convert the Float to Double, rather than a failure of the consumer to support Float.

(In reply to William Piers from comment #0)

When invoking [attribute.round()/], where :

The producer is an OCL PropertyCallExp of "self.attribute", which is unlikely to be enhanced by Acceleo so this is an OCL problem.

Fixing all consumers in the Classic OCL code is not appropriate.

Looking for the general policy in EvaluationVisitorImpl.java, it is clear that the assumption is that incoming values are good and outgoing values are coerced. An "abs" of a Float will actually give a CCE.

coerceNumber is called in many places, so it seems that the coerce (to Collection/non-Collection) in {Ecore/UML}EvaluationEnvironment.coerceValue should also coerceNumber.

No tests break: Branch ewillink/462386.

Impact: some previous code using non-Double/Integer/Long may now compute 'better'. However some Longs may be converted to Integer. In general simple computations in which an Ecore source type was preserved will now be coerced causing a change of result type. In particular a trivial computation involving Float will now return Double rather than Float. Probably 99% better, 1% debateable.

I'mn inclined to WONTFIX since the 1% debateable can be very irritating.

[eclipse-ocl-bot]

By Ed Willink on May 19, 2015 11:14

(In reply to Ed Willink from comment #4)\ There is also an unclear impact for Collection of Float usage.

I'm inclined to WONTFIX since the 1% debateable can be very irritating.

But leaving the clear bug unfixed is worse.

Adding a few instanceof Float's may only tackle 10% of the potential Float bugs, but it at least fixes this and related problems. Only users relying on a Float malfunction can realistically be impacted. 99.9% better 0.1% justifiably broken.

[eclipse-ocl-bot]

By Ed Willink on May 19, 2015 11:46

(In reply to Ed Willink from comment #5)

Adding a few instanceof Float's may only tackle 10% of the potential Float bugs, but it at least fixes this and related problems.

Fortunately the binary Real operations all go through NumberUtil.higherPrecisionNumber which converts Float and modest BigDecimals to Double. So the numeric operation problems are confined to the unary operations: -/abs/floor/round/oclAsType.

Testing for Double/Float/BigDecimal and then replacing the (Double)sourceValue by ((Number)sourceValue).doubleValue() should provide Float and modest BigDecimal tolerance at very little cost or risk.

(coerceValue solution pushed to archive/462386.)

Simple safe solution pushed to ewillink/462386. Please review for RC2.

[eclipse-ocl-bot]

By Ed Willink on May 21, 2015 06:05

Laurent: I think that Adolfo is on holiday. Would you care to eyeball the changes?

Branch ewillink/462386 or the patch below:

diff --git a/plugins/org.eclipse.ocl/src/org/eclipse/ocl/EvaluationVisitorImpl.java b/plugins/org.eclipse.ocl/src/org/eclipse/ocl/EvaluationVisitorImpl.java\ index c829713..dc5b3cb 100644\ --- a/plugins/org.eclipse.ocl/src/org/eclipse/ocl/EvaluationVisitorImpl.java\ +++ b/plugins/org.eclipse.ocl/src/org/eclipse/ocl/EvaluationVisitorImpl.java\ @@ -16,6 +16,7 @@\ \ package org.eclipse.ocl;\ \ +import java.math.BigDecimal;\ import java.util.AbstractList;\ import java.util.ArrayList;\ import java.util.Collection;\ @@ -453,7 +454,7 @@\ }\ \ // Double::minus()\

• return - (Double) sourceVal;\
• return -((Number)sourceVal).doubleValue();\ \ case PredefinedType.ABS:\ if (sourceVal instanceof Integer) {\ @@ -488,7 +489,7 @@\ }\ \ // Real::abs()\
• return Math.abs((Double) sourceVal);\
• return Math.abs(((Number)sourceVal).doubleValue());\ \ case PredefinedType.CHARACTERS:\ String sourceString = (String)sourceVal;\ @@ -499,9 +500,9 @@\ return results;\ \ case PredefinedType.FLOOR:\
• if (sourceVal instanceof Double) {\
• if ((sourceVal instanceof Double) || (sourceVal instanceof Float) || (sourceVal instanceof BigDecimal)) {\ // Real::floor()\
• return (int) Math.floor((Double) sourceVal);\
• return (int) Math.floor(((Number)sourceVal).doubleValue());\ }\ \ if (sourceType == getUnlimitedNatural()) {\ @@ -517,9 +518,9 @@\ return sourceVal;\ \ case PredefinedType.ROUND:\
• if (sourceVal instanceof Double) {\
• if ((sourceVal instanceof Double) || (sourceVal instanceof Float) || (sourceVal instanceof BigDecimal)) {\ // Real::round()\
• return (int) Math.round((Double) sourceVal);\
• return (int) Math.round(((Number)sourceVal).doubleValue());\ }\ \ if (sourceType == getUnlimitedNatural()) {\ @@ -737,9 +738,9 @@\ if (sourceVal instanceof String\ && ((TypeExp) arg).getReferredType() == getString()) {\ return sourceVal;\
• } else if (sourceVal instanceof Double\
• } else if (((sourceVal instanceof Double) || (sourceVal instanceof Float) || (sourceVal instanceof BigDecimal))\ && (argType == getInteger())) {
• return new Integer(((Double) sourceVal).intValue());\
• return new Integer(((Number) sourceVal).intValue());\ } else if (sourceVal instanceof Boolean\ && ((TypeExp) arg).getReferredType() == getBoolean()) {\ return sourceVal;

[eclipse-ocl-bot]

By Laurent Goubet on May 21, 2015 07:51

Would be easier to review in the form of a gerrit change, but these seem fine to me.

[eclipse-ocl-bot]

By Ed Willink on May 21, 2015 10:29

(In reply to Laurent Goubet from comment #8)

Would be easier to review in the form of a gerrit change, but these seem fine to me.

Thanks. Pushed to master for RC2.

commit af76b8d5f82cd454428d93e754f798e167d9526c

An I-build is available at

https://www.eclipse.org/downloads/download.php?file=/modeling/mdt/ocl/downloads/drops/6.0.0/I201505210935/mdt-ocl-Update-I201505210935.zip