StringIndexOutOfBoundsException when printing warnings in transformed files

[jridgewell]

AMP is seeing an exception being thrown when printing a code warning: ampproject/amp-closure-compiler#22

java.lang.StringIndexOutOfBoundsException: String index out of range: 75
        at java.lang.StringLatin1.charAt(StringLatin1.java:47)
        at java.lang.String.charAt(String.java:693)
        at com.google.javascript.jscomp.LightweightMessageFormatter.padLine(LightweightMessageFormatter.java:197)
        at com.google.javascript.jscomp.LightweightMessageFormatter.padMultipleLines(LightweightMessageFormatter.java:251)
        at com.google.javascript.jscomp.LightweightMessageFormatter.getExcerptWithPosition(LightweightMessageFormatter.java:161)
        at com.google.javascript.jscomp.LightweightMessageFormatter.format(LightweightMessageFormatter.java:133)
        at com.google.javascript.jscomp.LightweightMessageFormatter.formatWarning(LightweightMessageFormatter.java:86)
        at com.google.javascript.jscomp.JSError.format(JSError.java:171)
        at com.google.javascript.jscomp.PrintStreamErrorReportGenerator.println(PrintStreamErrorReportGenerator.java:56)
        at com.google.javascript.jscomp.PrintStreamErrorReportGenerator.generateReport(PrintStreamErrorReportGenerator.java:50)
        at com.google.javascript.jscomp.SortingErrorManager.generateReport(SortingErrorManager.java:116)
        at com.google.javascript.jscomp.ThreadSafeDelegatingErrorManager.generateReport(ThreadSafeDelegatingErrorManager.java:40)
        at com.google.javascript.jscomp.Compiler.generateReport(Compiler.java:724)
        at com.google.javascript.jscomp.AbstractCommandLineRunner.performFullCompilation(AbstractCommandLineRunner.java:1336)
        at com.google.javascript.jscomp.AbstractCommandLineRunner.doRun(AbstractCommandLineRunner.java:1254)
        at com.google.javascript.jscomp.AbstractCommandLineRunner.run(AbstractCommandLineRunner.java:540)
        at org.ampproject.AmpCommandLineRunner.main(Unknown Source)

After a bit of tracking, we've managed to isolate it down to the file's input sourcemap causing a confusion in the source code excerpt printer. Essentially, it's trying to print the transformed file's source code using locations from the original source. This is because the transformed code and the original source file share the same file name (the code transform is happening in memory with Gulp before passing it to Closure).

I've made an easy reproduction at https://github.com/jridgewell/closure-warn-sourcemaps-oob.

[nreid260]

Do you have a suggestion on what a better behaviour would be?

Our understanding is that another tool in your build is modifying the content of a file between two timepoints the compiler is accessing it. If that's the case, crashing seems preferable to producing an incorrect sourcemap.

[jridgewell]

Do you have a suggestion on what a better behaviour would be?

I thought about this a bit. The intention here is is to display the suspicious code. But there's two parts to this, there's the code that Closure actually sees (and considers suspicious), and there's the code the developer actually wrote. I think both are useful for determining what's gone wrong.

For instance, the warning AMP is seeing is:

  226|       if (false && nonStructThis['implementationClassForTesting']) {

The false is suspicious code, but that's now what we as developers wrote:

      if (getMode().test && nonStructThis['implementationClassForTesting']) {
        Ctor = nonStructThis['implementationClassForTesting'];
      }

Seeing the original code would tell me the where this is happening, but without Closure printing the code it's seeing, I wouldn't understand why I'm seeing this. Babel transformed (we have a custom plugin) that getMode().test into false, but expecting every AMP developer to know about that without seeing the actual output would be confusing.

So I think at a minimum, we should be showing the actual code Closure considers suspicious, not the original source code. If we could show both, then that would be even more helpful, but it's probably not necessary.

---

Our understanding is that another tool in your build is modifying the content of a file between two timepoints the compiler is accessing it.

That's not the case. We've generated the sourcemap and transformed file before closure is invoked. The build steps to generate the transformed code and sourcemap were included to describe our setup, but it would still error if you just invoked ./node_modules/google-closure-compiler --js=file.js --js_output_file=out.js.

If that's the case, crashing seems preferable to producing an incorrect sourcemap.

The sourcemap is valid.

The issue is only that you're using original file (via lookup through the sourcemap) locations on the input file. A sample fix is:

diff --git a/src/com/google/javascript/jscomp/LightweightMessageFormatter.java b/src/com/google/javascript/jscomp/LightweightMessageFormatter.java
index b54c6c43f..39915b17b 100644
--- a/src/com/google/javascript/jscomp/LightweightMessageFormatter.java
+++ b/src/com/google/javascript/jscomp/LightweightMessageFormatter.java
@@ -104,17 +104,17 @@ public final class LightweightMessageFormatter extends AbstractMessageFormatter
               : source.getSourceMapping(
                   error.getSourceName(), error.getLineNumber(), error.getCharno());

-      if (mapping != null) {
+      if (mapping == null) {
+        appendPosition(boldLine, sourceName, lineNumber, charno);
+      } else {
         appendPosition(b, sourceName, lineNumber, charno);
-
-        sourceName = mapping.getOriginalFile();
-        lineNumber = mapping.getLineNumber();
-        charno = mapping.getColumnPosition();
-
         b.append("\nOriginally at:\n");
+        appendPosition(
+            boldLine,
+            mapping.getOriginalFile(),
+            mapping.getLineNumber(),
+            mapping.getColumnPosition());
       }
-
-      appendPosition(boldLine, sourceName, lineNumber, charno);
     }

     if (includeLevel) {

[nreid260]

Ahh ok, so there's a real bug here when the input code comes with a sourcemap, we're indirecting through that first sourcemap in an unsafe way when building our own sourcemap. Given that, I think we'd accept a PR for your proposed solution (especially if it came with tests :))

As an aside based on your example case, have you considered the @define mechanism built into the compiler? It seems like it might cover what you're trying to do.

[nreid260]

After some digging, it's now clear that the underlying issue is that we never imagined that multiple inputs could have the same filename. That's happening here, because one of the inputs is source code, and the other is file mentioned in a source map.

We'd like to say that all inputs should have a unique filename, but that's not feasible for your usecase since Glup requires file transformations to happen in-place.

You also mentioned that this wasn't technically blocking you, but the workaround was wot disable warnings.