compiler crashes with integer overflow when evaluating expressions

[nicks]

{namespace examples.overflow}

/**
 * test
 */
{template .test}
  {let $y: 1000 * 1000 * 1000 * 1000 /}
{/template}

Crashes with the following exception:

$ java -jar target/soy-2.5.0-SNAPSHOT-SoyToJsSrcCompiler.jar --outputPathFormat simple.js --srcs hello.soy 
INTERNAL SOY ERROR.
Please open an issue at https://github.com/google/closure-templates/issues with this stack trace and repro steps
java.lang.IllegalStateException: Casting long to integer results in overflow: 1000000000000
    at com.google.common.base.Preconditions.checkState(Preconditions.java:200)
    at com.google.template.soy.data.restricted.IntegerData.integerValue(IntegerData.java:130)
    at com.google.template.soy.data.internalutils.InternalValueUtils.convertPrimitiveDataToExpr(InternalValueUtils.java:66)
    at com.google.template.soy.sharedpasses.opti.SimplifyExprVisitor.attemptPreeval(SimplifyExprVisitor.java:231)
    at com.google.template.soy.sharedpasses.opti.SimplifyExprVisitor.visitExprNode(SimplifyExprVisitor.java:204)

Reproduced with the compiler at head.

This is perfectly valid javascript because numbers are doubles? And in Java, it should convert to double or long?

Relatedly, it's not clear to me why the soy compiler is trying to evaluate the expression at all? This seems like something the Java or JS compiler would be more effective at.

[nicks]

also, would you accept a patch to fix this?

[lukesandberg]

We are evaluating the expression because it is part of our tree simplifying pass where we 'preevaluate' constant expressions

the runtime error is a bit weird, soy does most of its calculations on integers using java longs, but at various points we assert that it isn't 'too big' I believe the goal is all about server/client consistency. We don't want a server rendered template to give different results because it has higher precision integers (64 vs. 52 bits). imho it is a bit misguided, but I also didn't add these checks so i am not entirely sure about the motivation.

@Ubehebe any opinion on this?

[lukesandberg]

ok, just took a quicker look at the specific code in question and there are additional bits of weirdness.

apparently soy only allows integer constants to be declared if they fit in 32 bits. as a work around you should be able to add a '.0' suffix to any one of those numbers and flip into double math.

imho we should probably just switch all soy integer math to use 'long's instead of ints. There are a few places where we need 'int's (like indexing into collections), but unchecked downcasts should be fine in those places.

[nicks]

fwiw, there's already an exception catch that tells the simplifier to back off and leave the expression alone.

closure-templates/java/src/com/google/template/soy/sharedpasses/opti/SimplifyExprVisitor.java

Line 231 in 50a0b69

PrimitiveNode newNode =

I was going to suggest that we move the convertPrimitiveDataToExpr call into the exception block. That fixes the obvious problem without turning this into a referendum on how soy represents numbers, or on the whole SimplifyExpr enterprise.