case of integer matching does not work correctly for larger numbers

[radeusgd]

Repro:

from Standard.Base import all

foo x = case x of
    1 -> "foo: ONE"
    2 -> "foo: TWO"
    200 -> "foo: TWO HUNDRED"
    9999 -> "foo: NINE THOUSAND NINE HUNDRED NINETY NINE"
    _ -> "foo: OTHER - "+x.to_text

main =
    IO.println "foo:"
    IO.println (foo 1)
    IO.println (foo 2)
    IO.println (foo 200)
    IO.println (foo 9999)
    IO.println (foo 3)

Actual behaviour

It gives:

foo:
foo: ONE
foo: TWO
foo: OTHER - 200
foo: OTHER - 9999
foo: OTHER - 3

missing the 200 and 9999 branches.

Expected behaviour

foo:
foo: ONE
foo: TWO
foo: TWO HUNDRED
foo: NINE THOUSAND NINE HUNDRED NINETY NINE
foo: OTHER - 3

[radeusgd]

I suspect we are likely performing a == on boxed Long values. For small values, the JVM uses the AutoBoxCache to re-use the boxed values. For larger values, new instances get created and they are no longer ==.

[radeusgd]

Looking at sources confirms that to be the case:
https://github.com/enso-org/enso/blob/develop/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java#L39-L46

[radeusgd]

So applying the following patch

Index: engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java	(revision 2182072c7cad5725e3a5187e7c68e2ff02f03190)
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/NumericLiteralBranchNode.java	(revision 2ff98076e0244074a88ffdfa937d77413dc58442)
@@ -42,7 +42,7 @@
       Object state,
       Object target,
       @CachedLibrary(limit = "1") InteropLibrary interop) {
-    if (numProfile.profile(literal == target)) accept(frame, state, new Object[0]);
+    if (numProfile.profile(literal.equals(target))) accept(frame, state, new Object[0]);
   }
 
   @Fallback

seems to fix the issue.

@JaroslavTulach do you think that fix seems OK?

I'm happy to integrate it as part of my PR for ticket #8352 as I need it there to fix HTTP status code translation.

[hubertp]

@radeusgd I'd rather we do it in a separate PR (as @JaroslavTulach mentioned, there are problems with native image build in that PR).

[JaroslavTulach]

Yes, numProfile.profile(literal == target) seems to be wrong. CCing @hubertp - alas I believe that using equals isn't particularly PE friendly and your CI shall fail in engine-runner/buildNativeImage.

For long and double values we should be using PrimitiveValueProfile (or implement something like that manually). For BigInteger we can fast track == and then .equals behind @TruffleBoundary.

do it in a separate PR

Yes, this is not going to be straightforward fix.

[radeusgd]

Yes, numProfile.profile(literal == target) seems to be wrong. CCing @hubertp - alas I believe that using equals isn't particularly PE friendly and your CI shall fail in engine-runner/buildNativeImage.

For long and double values we should be using PrimitiveValueProfile (or implement something like that manually). For BigInteger we can fast track == and then .equals behind @TruffleBoundary.

do it in a separate PR

Yes, this is not going to be straightforward fix.

Okay. I guess I will be figuring out a workaround for my PR then, or will be waiting with integration until a fix.

[enso-bot]

Jaroslav Tulach reports a new STANDUP for yesterday (2023-12-11):

Progress: - fix of #8493 by #8510

• emails
• advent of code
• Wojciech: review of my GeeCON presentation It should be finished by 2023-12-11.

Next Day: Meetings, planning