From f87e7f3a538ba585ed7b435749f7b11d7ddbf39d Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Tue, 24 Dec 2024 12:00:50 +0100 Subject: [PATCH] Fix: add file name in stack traces for compiled classes In interpreter mode, we always put the file name, but in compiled mode we do it only in case of `compilerEnv.isGenerateDebugInfo()`. I think that the interpreter's behavior is much more useful, so I changed the compiler to always include the file name. For an example of the difference, create two files: `one.js` ```js const two = require('./two'); two(); ``` and `two.js`: ```js module.exports = function() { throw new Error('hey') } ``` Then launch the shell passing `-version 200 -opt -1 -require one.js` to get the following, useful stack trace: ``` js: "file:/Users/andrea.bergia/src/rhino/two.js", line 2: exception from uncaught JavaScript throw: Error: hey at file:/Users/andrea.bergia/src/rhino/two.js:2 at file:/Users/andrea.bergia/src/rhino/one.js:2 ``` But with `-opt 0` we get: ``` js: "file:/Users/andrea.bergia/src/rhino/two.js", line 2: exception from uncaught JavaScript throw: Error: hey at (unknown):2 (anonymous) at (unknown):2 ``` which isn't as useful. --- .../mozilla/javascript/optimizer/Codegen.java | 6 +--- .../tests/commonjs/module/RequireTest.java | 31 +++++++++++++++++++ .../tests/commonjs/module/throw-one.js | 2 ++ .../tests/commonjs/module/throw-two.js | 3 ++ 4 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-one.js create mode 100644 tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-two.js diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/Codegen.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/Codegen.java index 08bd386616..96a9e15ca8 100644 --- a/rhino/src/main/java/org/mozilla/javascript/optimizer/Codegen.java +++ b/rhino/src/main/java/org/mozilla/javascript/optimizer/Codegen.java @@ -250,11 +250,7 @@ private byte[] generateCode(String rawSource) { boolean hasFunctions = (scriptOrFnNodes.length > 1 || !hasScript); boolean isStrictMode = scriptOrFnNodes[0].isInStrictMode(); - String sourceFile = null; - if (compilerEnv.isGenerateDebugInfo()) { - sourceFile = scriptOrFnNodes[0].getSourceName(); - } - + String sourceFile = scriptOrFnNodes[0].getSourceName(); ClassFileWriter cfw = new ClassFileWriter(mainClassName, SUPER_CLASS_NAME, sourceFile); cfw.addField(ID_FIELD_NAME, "I", ACC_PRIVATE); diff --git a/tests/src/test/java/org/mozilla/javascript/tests/commonjs/module/RequireTest.java b/tests/src/test/java/org/mozilla/javascript/tests/commonjs/module/RequireTest.java index 8f9785ac2c..6d68d9f481 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/commonjs/module/RequireTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/commonjs/module/RequireTest.java @@ -1,6 +1,8 @@ package org.mozilla.javascript.tests.commonjs.module; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.InputStreamReader; @@ -10,11 +12,14 @@ import java.util.Collections; import org.junit.Test; import org.mozilla.javascript.Context; +import org.mozilla.javascript.RhinoException; +import org.mozilla.javascript.ScriptStackElement; import org.mozilla.javascript.Scriptable; import org.mozilla.javascript.ScriptableObject; import org.mozilla.javascript.commonjs.module.Require; import org.mozilla.javascript.commonjs.module.provider.StrongCachingModuleScriptProvider; import org.mozilla.javascript.commonjs.module.provider.UrlModuleSourceProvider; +import org.mozilla.javascript.tests.Utils; /** * @author Attila Szegedi @@ -137,6 +142,32 @@ public void setMainForAlreadyLoadedModule() throws Exception { } } + @Test + public void stackTracesAlwaysHaveFileName() { + Utils.runWithAllModes( + cx -> { + cx.setGeneratingDebug(false); + final Scriptable scope = cx.initStandardObjects(); + try { + final Require require = getSandboxedRequire(cx, scope, false); + require.install(scope); + RhinoException rhinoException = + assertThrows( + RhinoException.class, + () -> require.requireMain(cx, "throw-one")); + + ScriptStackElement[] stack = rhinoException.getScriptStack(); + assertEquals(2, stack.length); + + assertTrue(stack[0].fileName.contains("throw-two.js")); + assertTrue(stack[1].fileName.contains("throw-one.js")); + } catch (Exception e) { + throw new RuntimeException(e); + } + return null; + }); + } + private Reader getReader(String name) { return new InputStreamReader(getClass().getResourceAsStream(name)); } diff --git a/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-one.js b/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-one.js new file mode 100644 index 0000000000..b3523d280d --- /dev/null +++ b/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-one.js @@ -0,0 +1,2 @@ +const two = require('throw-two'); +two(); \ No newline at end of file diff --git a/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-two.js b/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-two.js new file mode 100644 index 0000000000..86ad37b601 --- /dev/null +++ b/tests/src/test/resources/org/mozilla/javascript/tests/commonjs/module/throw-two.js @@ -0,0 +1,3 @@ +module.exports = function() { + throw new Error('hey') +} \ No newline at end of file