Skip to content

Commit 5bcfbc6

Browse files
respencer-nclclaude
respencer-ncl
and
claude
committed
Fix System.lineSeparator() returning null bytes in Scala.js
System.lineSeparator() returns \0 in Scala.js, causing esbuild "Unterminated string literal" errors. Replace all shared-code usages with PlatformContext.newline which returns the correct value per platform (System.lineSeparator() on JVM/Native, "\n" on JS). Key changes: - FileBuilder trait now takes (using PlatformContext) parameter, propagated through OutputFile, TextFileWriter, RiddlFileEmitter, PrettifyState, PrettifyVisitor, and PrettifyOutput - StringHelpers.toPrettyString takes (using PlatformContext) - Messages.format uses existing nl constant instead of System call - JVM/Native PlatformContext.newline uses System.lineSeparator() - Command files use PlatformContext.newline consistently - Removed @JSExportTopLevel from PrettifyState/PrettifyOutput (incompatible with using parameter lists in Scala.js exports) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 5b2e768 commit 5bcfbc6

19 files changed

Lines changed: 367 additions & 29 deletions

File tree

‎FIX-SYSTEM-LINESEPARATOR.md‎

Lines changed: 337 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,337 @@
1+
# Fix System.lineSeparator() for Scala.js Compatibility
2+
3+
## Problem
4+
5+
`System.lineSeparator()` returns `\0` (null character) when compiled
6+
to Scala.js. This null byte embeds into JavaScript string constants,
7+
which terminates the string literal prematurely. The result is an
8+
"Unterminated string literal" error from esbuild/vite when bundling
9+
the JS output for Electron or browser use.
10+
11+
This was partially fixed in RIDDL 1.2.2 (`RiddlParserInput.scala`
12+
changed to use `"\n"` directly), but several other call sites in
13+
**shared** source remain, blocking Synapify from running.
14+
15+
## Root Cause
16+
17+
`System.lineSeparator()` is a JVM API. In Scala.js, it compiles to
18+
a call that returns `\0` (null character) instead of `"\n"`. When
19+
this value is interpolated into a string constant in the compiled JS
20+
output, the null byte truncates the string at the JS parser level.
21+
22+
## Design: Centralize in PlatformContext
23+
24+
`PlatformContext` already has `def newline: String` (line 93 of
25+
`PlatformContext.scala`). This is the correct abstraction point.
26+
The pattern is:
27+
28+
1. **Trait** (`PlatformContext`) defines the contract: `def newline`
29+
2. **Platform implementations** provide correct values:
30+
- JVM: `System.lineSeparator()` (correct `\r\n` on Windows)
31+
- JS: `"\n"` (safe, avoids null byte from broken JS impl)
32+
- Native: `System.lineSeparator()` (Scala Native implements
33+
this correctly — returns `"\r\n"` on Windows, `"\n"` on Unix)
34+
3. **Shared code** accesses via `pc.newline` through the `given`
35+
instance, never calling `System.lineSeparator()` directly
36+
4. The `given pc: PlatformContext` in each platform's package object
37+
ensures the correct implementation is resolved at compile time
38+
39+
## Changes Required
40+
41+
### 1. PlatformContext Trait (no change needed)
42+
43+
**File:** `utils/shared/src/main/scala/.../PlatformContext.scala`
44+
45+
The trait already defines the contract at line 93:
46+
47+
```scala
48+
/** The newline character for this platform */
49+
def newline: String
50+
```
51+
52+
No change needed here.
53+
54+
### 2. Platform Implementations (2 changes, 1 confirmation)
55+
56+
#### JVM — CHANGE
57+
58+
**File:** `utils/jvm/src/main/scala/.../JVMPlatformContext.scala`
59+
**Line 83:**
60+
61+
```scala
62+
// Before:
63+
override def newline: String = "\n"
64+
65+
// After:
66+
override def newline: String = System.lineSeparator()
67+
```
68+
69+
This restores correct platform-specific behavior on JVM (e.g.,
70+
`\r\n` on Windows).
71+
72+
#### JavaScript — NO CHANGE (confirm correct)
73+
74+
**File:** `utils/js/src/main/scala/.../DOMPlatformContext.scala`
75+
**Line 61:**
76+
77+
```scala
78+
override def newline: String = "\n"
79+
```
80+
81+
Already correct. Must NOT use `System.lineSeparator()` here — that
82+
is the entire bug. Scala.js's implementation returns `\0` (null
83+
character). Keeping `"\n"` is intentional and safe.
84+
85+
#### Native — CHANGE
86+
87+
**File:** `utils/native/src/main/scala/.../NativePlatformContext.scala`
88+
**Line 95:**
89+
90+
```scala
91+
// Before:
92+
override def newline: String = "\n"
93+
94+
// After:
95+
override def newline: String = System.lineSeparator()
96+
```
97+
98+
Scala Native implements `System.lineSeparator()` correctly — it
99+
checks `isWindows` at runtime and returns `"\r\n"` on Windows or
100+
`"\n"` on Unix/macOS. This is safe to use here (unlike Scala.js).
101+
See: [Scala Native javalib System.scala](https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/lang/System.scala)
102+
103+
Note: RIDDL's own `ExceptionUtils.scala` in the native module
104+
already calls `System.lineSeparator` at line 34, confirming it
105+
works on this platform.
106+
107+
#### Given Instances (no changes needed)
108+
109+
Each platform's package object provides the `given` that wires
110+
everything together:
111+
112+
- **JVM:** `given pc: PlatformContext = JVMPlatformContext()`
113+
- **JS:** `given pc: PlatformContext = DOMPlatformContext()`
114+
- **Native:** `given pc: PlatformContext = NativePlatformContext()`
115+
116+
Shared code doing `(using pc: PlatformContext)` then
117+
`pc.newline` automatically gets the right value for the platform.
118+
119+
---
120+
121+
### 3. Messages.scala — CRITICAL (shared, compiles to JS)
122+
123+
**File:** `language/shared/src/main/scala/.../Messages.scala`
124+
**Line 232:** `msgs.map(_.format).mkString(System.lineSeparator())`
125+
126+
This is an extension method on `Messages` (`List[Message]`). It
127+
already has a `val nl: String = "\n"` at line 114 with a comment
128+
explaining the Scala.js issue. Use it:
129+
130+
```scala
131+
// Before:
132+
def format: String = {
133+
msgs.map(_.format).mkString(System.lineSeparator())
134+
}
135+
136+
// After:
137+
def format: String = {
138+
msgs.map(_.format).mkString(nl)
139+
}
140+
```
141+
142+
This is the simplest fix — 1-line change, no signature changes,
143+
uses the existing `nl` constant that was created for this purpose.
144+
145+
Alternatively, add `(using pc: PlatformContext)` and use
146+
`pc.newline`, but that would propagate to all callers of `.format`
147+
on message lists which is more invasive. The `nl` constant approach
148+
is pragmatic and already documented.
149+
150+
---
151+
152+
### 4. StringHelpers.scala — CRITICAL (shared, compiles to JS)
153+
154+
**File:** `utils/shared/src/main/scala/.../StringHelpers.scala`
155+
**Line 31:** `val nl = System.lineSeparator()`
156+
157+
Add `(using PlatformContext)` to `toPrettyString`:
158+
159+
```scala
160+
// Before:
161+
def toPrettyString(
162+
obj: Any,
163+
depth: Int = 0,
164+
paramName: Option[String] = None
165+
): String = {
166+
val buf = new StringBuffer(1024)
167+
val nl = System.lineSeparator()
168+
...
169+
170+
// After:
171+
def toPrettyString(
172+
obj: Any,
173+
depth: Int = 0,
174+
paramName: Option[String] = None
175+
)(using pc: PlatformContext): String = {
176+
val buf = new StringBuffer(1024)
177+
val nl = pc.newline
178+
...
179+
```
180+
181+
**Callers to update** (all in commands module, JVM/Native only —
182+
all already have `PlatformContext` in scope via `using`, so the
183+
given propagates automatically, no call-site changes needed):
184+
185+
- `Command.scala:68` — `toPrettyString(opt, 1)`
186+
- `Command.scala:147` — `toPrettyString(...)`
187+
- `CommonOptionsHelper.scala:272` — `toPrettyString(options, ...)`
188+
- `DumpCommand.scala:37` — `toPrettyString(result, 1, None)`
189+
- `FromCommand.scala:70` — `toPrettyString(newCO)`
190+
191+
**Tests:** `StringHelpersTest.scala` needs
192+
`import com.ossuminc.riddl.utils.pc` so the `given PlatformContext`
193+
is in scope. All ~15 test calls pick it up implicitly. The assertion
194+
at line 225-226 that checks `System.lineSeparator()` should change
195+
to `"\n"` or `pc.newline`.
196+
197+
---
198+
199+
### 5. FileBuilder.scala — CRITICAL (shared, compiles to JS)
200+
201+
**File:** `utils/shared/src/main/scala/.../FileBuilder.scala`
202+
**Line 19:** `protected val new_line: String = System.lineSeparator()`
203+
204+
**Recommended approach — Scala 3 trait parameter:**
205+
206+
```scala
207+
// Before:
208+
trait FileBuilder {
209+
protected val new_line: String = System.lineSeparator()
210+
...
211+
212+
// After:
213+
trait FileBuilder(using pc: PlatformContext) {
214+
protected val new_line: String = pc.newline
215+
...
216+
```
217+
218+
**Implementors to update** (3 classes):
219+
220+
1. **OutputFile.scala** (jvm-native):
221+
```scala
222+
// File: utils/jvm-native/src/main/scala/.../OutputFile.scala
223+
// Before:
224+
trait OutputFile extends FileBuilder {
225+
// After:
226+
trait OutputFile(using PlatformContext) extends FileBuilder {
227+
```
228+
229+
2. **RiddlFileEmitter.scala** (shared, passes module):
230+
```scala
231+
// File: passes/shared/src/main/scala/.../RiddlFileEmitter.scala
232+
// Before:
233+
case class RiddlFileEmitter(url: URL) extends FileBuilder {
234+
// After:
235+
case class RiddlFileEmitter(url: URL)(using PlatformContext)
236+
extends FileBuilder {
237+
```
238+
239+
3. **FileBuilderTest.scala** (jvm test):
240+
```scala
241+
// File: utils/jvm/src/test/scala/.../FileBuilderTest.scala
242+
// Add import: import com.ossuminc.riddl.utils.pc
243+
// Before:
244+
class TestFileBuilder extends FileBuilder {
245+
// After:
246+
class TestFileBuilder(using PlatformContext) extends FileBuilder {
247+
```
248+
249+
**Note:** Check for further downstream classes that extend
250+
`OutputFile` or `RiddlFileEmitter` — they may also need `using
251+
PlatformContext` propagated. Most pass code already has
252+
`PlatformContext` in scope.
253+
254+
**Simpler alternative** if trait parameters cause too many cascading
255+
changes: Just change the default to `"\n"`:
256+
257+
```scala
258+
protected val new_line: String = "\n"
259+
```
260+
261+
This is safe on all platforms and requires zero downstream changes.
262+
JVM code that truly needs `\r\n` on Windows can override `new_line`.
263+
Less architecturally pure but pragmatic.
264+
265+
---
266+
267+
### 6. Command files — LOW PRIORITY (JVM/Native only)
268+
269+
These are JVM/Native only, so `System.lineSeparator()` works
270+
correctly and won't produce null bytes. Fix for consistency with the
271+
PlatformContext pattern but not urgent.
272+
273+
**Files:**
274+
- `commands/shared/.../Command.scala:150`
275+
- `commands/shared/.../HelpCommand.scala:50,55,59`
276+
- `commands/shared/.../AboutCommand.scala:45`
277+
278+
All already have `using PlatformContext` in their call chain.
279+
Replace `System.lineSeparator()` with `pc.newline`.
280+
281+
---
282+
283+
## Implementation Order
284+
285+
1. **JVMPlatformContext** and **NativePlatformContext** — change
286+
`newline` to `System.lineSeparator()` (JS stays as `"\n"`)
287+
2. **Messages.scala:232** — use existing `nl` constant (1-line fix,
288+
immediate impact, no signature changes)
289+
3. **StringHelpers.toPrettyString** — add `using PlatformContext`,
290+
use `pc.newline`, update tests
291+
4. **FileBuilder** — add trait parameter or change default to `"\n"`
292+
5. **Command files** — update for consistency
293+
294+
## Testing
295+
296+
After changes:
297+
```bash
298+
# Verify all platform tests pass
299+
sbt tJVM
300+
301+
# Verify JS compilation produces clean output (no null bytes)
302+
sbt "languageJS/fastLinkJS" "utilsJS/fastLinkJS"
303+
304+
# Optionally scan for null bytes in JS output:
305+
python3 -c "
306+
with open('path/to/fastopt/main.js', 'rb') as f:
307+
content = f.read()
308+
nulls = [i for i, b in enumerate(content) if b == 0]
309+
if nulls: print(f'NULL bytes at offsets: {nulls[:20]}')
310+
else: print('No null bytes found - clean!')
311+
"
312+
313+
# Verify in Synapify (downstream consumer):
314+
cd ../synapify
315+
sbt clean fastLinkJS
316+
npm run dev
317+
```
318+
319+
## Version Impact
320+
321+
- If only changing `new_line` default and `Messages.nl` usage:
322+
**patch** bump (no API changes)
323+
- If `FileBuilder` gets a trait parameter: **minor** bump (binary
324+
incompatible for implementors)
325+
- If `StringHelpers.toPrettyString` gets `using PlatformContext`:
326+
**minor** bump (source compatible via given, but binary change)
327+
328+
## Audit: Remaining System.getProperty Calls
329+
330+
These `System.getProperty` calls also exist in shared code but are
331+
handled with `Option(...)` null-safety:
332+
333+
- `URL.scala:159` — `Option(System.getProperty("user.dir"))
334+
.getOrElse("").drop(1)` — safe (returns empty string on JS)
335+
336+
The commands-only `System.getProperty` calls (InfoCommand,
337+
PrettifyCommand) are JVM/Native only and safe.

‎commands/shared/src/main/scala/com/ossuminc/riddl/command/Command.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ trait Command[OPT <: CommandOptions: ClassTag](val commandName: String)(using io
147147
val pretty = StringHelpers.toPrettyString(
148148
result,
149149
1,
150-
Some(s"Loaded these options:${System.lineSeparator()}")
150+
Some(s"Loaded these options:${io.newline}")
151151
)
152152
println(pretty)
153153
end if

‎commands/shared/src/main/scala/com/ossuminc/riddl/commands/AboutCommand.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class AboutCommand(using io: PlatformContext) extends Command[AboutCommand.Optio
4242
): Either[Messages, PassesResult] = {
4343
if io.options.verbose || !io.options.quiet then {
4444
val about: String = {
45-
CommonOptionsHelper.blurb ++ System.lineSeparator() ++
45+
CommonOptionsHelper.blurb ++ io.newline ++
4646
"Extensive Documentation here: https://riddl.tech"
4747
}
4848

‎commands/shared/src/main/scala/com/ossuminc/riddl/commands/HelpCommand.scala‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,16 @@ class HelpCommand(using val pc: PlatformContext) extends Command[HelpCommand.Opt
4747
val common = OParser.usage(CommonOptionsHelper.commonOptionsParser, OneColumn)
4848
val commands = OParser.usage(CommandLoader.commandOptionsParser, OneColumn)
4949
val improved_commands = commands
50-
.split(System.lineSeparator())
50+
.split(pc.newline)
5151
.flatMap { line =>
5252
if line.isEmpty || line.forall(_.isWhitespace) then {
5353
Seq.empty[String]
5454
} else if line.startsWith("Command:") then {
55-
Seq(System.lineSeparator() + line)
55+
Seq(pc.newline + line)
5656
} else if line.startsWith("Usage:") then { Seq(line) }
5757
else { Seq(" " + line) }
5858
}
59-
.mkString(System.lineSeparator())
59+
.mkString(pc.newline)
6060
common ++ "\n\n" ++ improved_commands
6161
}
6262
pc.log.info(usage)

‎language/shared/src/main/scala/com/ossuminc/riddl/language/Messages.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ object Messages {
229229

230230
/** Format all the messages with a newline in between them. */
231231
@JSExport def format: String = {
232-
msgs.map(_.format).mkString(System.lineSeparator())
232+
msgs.map(_.format).mkString(nl)
233233
}
234234

235235
/** Return true iff all the messages are only warnings */

‎passes/jvm-native/src/test/scala/com/ossuminc/riddl/passes/prettify/RiddlFileEmitterTest.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import com.ossuminc.riddl.language.AST.*
1010
import com.ossuminc.riddl.language.{Contents, *}
1111
import com.ossuminc.riddl.language.At
1212
import com.ossuminc.riddl.passes.prettify.RiddlFileEmitter
13-
import com.ossuminc.riddl.utils.{AbstractTestingBasis, URL}
13+
import com.ossuminc.riddl.utils.{AbstractTestingBasis, URL, pc}
1414

1515
import java.nio.file.{Files, Path}
1616

0 commit comments

Comments
 (0)