Properly handle index-out-of-bounds on JS #537
Conversation
| scalaJSLinkerConfig ~= { | ||
| _.withSemantics( | ||
| _.withAsInstanceOfs(org.scalajs.linker.interface.CheckedBehavior.Unchecked) | ||
| .withArrayIndexOutOfBounds(org.scalajs.linker.interface.CheckedBehavior.Unchecked) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Although in production mode these are undefined behaviors, in development mode Scala.js raises fatal errors to help detect these issues. As a workaround, we were previously disabling this to silence the fatal errors (my bad 😅 ).
| final protected[this] def byte(i: Int): Byte = { | ||
| if (Platform.isJs) { | ||
| if (i < 0 || i >= src.length) throw new ArrayIndexOutOfBoundsException | ||
| } | ||
| src(i) | ||
| } |
There was a problem hiding this comment.
I decided that platform-specific guards would be the best way to implement this.
Because isJs is a compile-time constant, the Scala compiler will actually elide these checks on JVM+Native before it even reaches the bytecode. So there is zero-cost to JVM and Native due to this change.
The other option is that we split these sources across platform but I think that is worse for readability.
There was a problem hiding this comment.
Because
isJsis a compile-time constant, the Scala compiler will actually elide these checks on JVM+Native before it even reaches the bytecode.
til. Is this eliding part of the Scala 2 inliner / Scala 3 inline keyword or just part of core scalac?
Its ifs the former maybe it makes sense to explicitly inline using @inline/inline keyword respectively so its more clearly?
In any case maybe it makes sense to add this as a brief comment to all of the Platform files for the different platforms?
| if (cs.length == 0) | ||
| throw InvalidLong(cs.toString) |
There was a problem hiding this comment.
This is an existing bug that also applies to JVM and Native.
Follow-up to #535 (comment). On Scala.js, index-out-of-bounds is undefined behavior.
https://www.scala-js.org/doc/semantics.html#undefined-behaviors
Jawn relies on these exceptions to detect parsing failures, so for correctness we rely on well-defined behavior. Thus this PR manually restores the out-of-bounds checks for JS only.
Inline comments incoming.