make InfHelper process comma separator

[enikao]

We had the following stacktrace:

java.lang.NumberFormatException: Character , is neither a decimal digit number, decimal point, nor "e" notation exponential mark.
         at java.base/java.math.BigDecimal.<init>(BigDecimal.java:586)
         at java.base/java.math.BigDecimal.<init>(BigDecimal.java:471)
         at java.base/java.math.BigDecimal.<init>(BigDecimal.java:900)
         at o.i.c.expr.base//org.iets3.core.expr.base.plugin.InfHelper.add(InfHelper.java:46)
         at o.i.c.expr.simpleTypes//org.iets3.core.expr.simpleTypes.typesystem.TypesystemDescriptor$CustomOverloadedOperationsTypesProvider_a_0.getOperationType(TypesystemDescriptor.java:402)
         at jetbrains.mps.lang.typesystem.runtime.OverloadedOperationsManager.getOperationType(OverloadedOperationsManager.java:81)
         at jetbrains.mps.typesystem.inference.RulesManager.getOperationType(RulesManager.java:325)
         at jetbrains.mps.typesystem.inference.TypeCheckerHelper.getOperationType(TypeCheckerHelper.java:74)
         at jetbrains.mps.newTypesystem.context.BaseTypecheckingContext.getOverloadedOperationType(BaseTypecheckingContext.java:148)
         at o.i.c.expr.temporal//org.iets3.core.expr.temporal.typesystem.typeof_BinaryArithmeticExpression_InferenceRule.lambda$applyRule$1(typeof_BinaryArithmeticExpression_InferenceRule.java:63)

The project uses comma as decimal separator.

This PR makes org.iets3.core.expr.base.plugin.InfHelper aware of comma as decimal separator. It also adds nullable annotations, and deprecates unused skewLow flags.

We also replace a UnitTest InfHelperTest with a node test, because it's the only UnitTest. Mixing UnitTests and NodeTests in the same model makes it harder to run all tests in the model at once (in MPS).

[enikao]

TO DISCUSS: Currently, nullable annotations in InfHelper reflect the implementation, but are not very structured. That could be unified if we wanted to.

[enikao]

TO DISCUSS: We added more tests to test.ts.expr.os.m1.InfHelperTest. They currently reflect the existing implementation. Some parts are marked with TODO or commented out because they'd fail. The commented part reflects my expectations, we should discuss what should be the actual outcome.

[alexanderpann]

I agree with every single commented out test in InfHelperTest. What are exactly the inconsistencies between min and max?

That could be unified if we wanted to. I am fine with the changes. How would you unified it?

[enikao]

What are exactly the inconsistencies between min and max?

min(+inf, +inf) == +inf vs. max(-inf, -inf) == null

That could be unified if we wanted to. I am fine with the changes. How would you unified it?

All parameters should be @NotNull, and ideally also all return values. This leaves an issue with min and max returning null if a NumberFormatException occurs.

[alexanderpann]

For me, min(+inf, +inf) looks right and max(-inf, -inf) is incorrect, so we should change the implementation.

All parameters should be @NotNull, and ideally also all return values

That sounds like a breaking change to me. Maybe min and max should just throw the exeption as a RuntimeException instead, so that we can mark it as @NotNull? Swallowing the error makes it only hard to debug in the future.

[enikao]

For me, min(+inf, +inf) looks right and max(-inf, -inf) is incorrect, so we should change the implementation.

I agree.

All parameters should be @NotNull, and ideally also all return values That sounds like a breaking change to me.

Yes, it would be a breaking change.

Maybe min and max should just throw the exeption as a RuntimeException instead, so that we can mark it as @NotNull? Swallowing the error makes it only hard to debug in the future.

I'd say we can just let the NumberFormatException bubble up (it is a RuntimeException). I'd do the same for the other methods -- they also swallow it (equals etc.).

[enikao]

Remaining issues:

1. negate("10,33") (with comma) is accepted with dot separation active. Seems ok to me, even being inconsistent.
2. What's the result of less(-inf,-inf), less(+inf,+inf), greater(-inf,-inf), greater(+inf,+inf)?
3. Currently, only isZero("0") is true. All of isZero("0.0"), isZero("-0"), etc. is false. Really?
4. isPosInf() and isNegInf() don't throw if we're using comma separation in dot separation mode. I don't think we need exceptions in this case.

[enikao]

Hmm interesting: On the build server we get NullPointerExceptions instead of IllegalArgumentExceptions for null values in @NotNull parameters. Is this expected? I thought MPS generates the code for @NonNull parameter check.

[alexanderpann]

The reason is probably that we didn't set the JetBrains Java Compiler in the Build-Script which evaluates those annotations but use the standard one. I am fine if you want to do the change.

[enikao]

Remaining issues:

1. `negate("10,33")` (with comma) is accepted with dot separation active. Seems ok to me, even being inconsistent.

2. What's the result of `less(-inf,-inf)`, `less(+inf,+inf)`, `greater(-inf,-inf)`, `greater(+inf,+inf)`?

3. Currently, _only_ `isZero("0")` is `true`. All of `isZero("0.0")`, `isZero("-0")`, etc. is `false`. Really?

4. `isPosInf()` and `isNegInf()` don't throw if we're using comma separation in dot separation mode. I don't think we need exceptions in this case.

@alexanderpann Any opinion on these?

[alexanderpann]

1. Agree
2. From a mathematical standpoint, they should definitely return all false.
3. This looks like a bug to me. Even something like new BigInteger(value).intValue() == 0with a try catch would be sufficient.
4. Agree

[enikao]

@alexanderpann Would you mind reviewing / approving this?

[enikao]

Thanks a lot @alexanderpann