SourceMapConsumerV3 feature request: allow disabling of approximated mappings

[lauraharker]

Filing off of https://groups.google.com/g/closure-compiler-discuss/c/rhAsFdwsnmQ so that this shows up in our bug scrub meeting. (and so that someone more familiar with SourceMapConsumerV3 than me will look at it)

Here's Kyle's original request:

I am a consumer of SourceMapConsumerV3 :)

I am using the getMappingForLine API to retrieve a source line from a transpiled line and column.

There are some cases where calling this API will not find a direct mapping and approximate it, either by retrieving previous transpiled line mappings or by grabbing a mapping with a different column than was requested.

It would be nice if one could opt out of this behavior, so that the caller could have the visibility that a mapping was not found vs it being approximated.

Are there any objections to this feature request? If not I would be willing to contribute the change.

and a followup message:

rough example here: https://github.com/kmantzel/closure-compiler/pull/1/files

Also I know I originally mentioned that the approximation can "grab a mapping with a different column than was requested", but I think that was an error on my part for not understanding source mapping. As a variable name can span multiple columns. So I just ended up making the approximation around the existing getPreviousLine method for now.

Just out of curiosity, is changing the behavior without this feature flag an option?

[lauraharker]

Bradford offered to look more into this & if it would be reasonable to always disable "approximate mappings"

[brad4d]

I believe the "approximate mappings" mentioned here represent the two places in the method where getPreviousMapping() gets called.

https://github.com/google/closure-compiler/blob/master/src/com/google/debugging/sourcemap/SourceMapConsumerV3.java#L163

As an experiment I modified these 2 places to return null instead and then ran our usual tests. I got one failure in CompilerTest and several in an internal-only test.

My interpretation of this result is that we do actually depend on this approximation behavior. However, I think it would be reasonable to expand the OriginalMapping data structure defined in mapping.proto to include an enum with values UNKNOWN, EXACT, and APPROXIMATE. (default of UNKNOWN), then modify the API to populate this field.

Kyle would you be interested in doing that?

[kmantzel]

Hi Bradford, thanks for looking into this. I'd definitely be willing to make any changes.

Your understanding is correct that the behavior I'd like to change is specifically around the getPreviousMapping method.

FWIW my expectation matches how the mozilla source-map package behaves for this scenario. I will include an example below of a discrepancy between the mozilla source-map library and GCC to showcase this.

I think I follow your suggestion, but have some questions that would clarify things for me:

1. It sounds like you want to keep the existing control flow intact, with the addition of a new enum added to OriginalMapping. That way, callers of getMappingForLine can determine whether the returned OriginalMapping is EXACT or APPROXIMATE?
2. Is the UNKOWN type intended to be a placeholder, that gets overwritten with EXACT or APPROXIMATE?

[kmantzel]

Discrepancy between GCC and Mozilla source map

Google Closure Compiler behavior (1-indexed column)

Calling SourceMapConsumerV3#getMappingForLine(40, 10) returns 9 when I would expect a null mapping

Mozilla Source Map behavior (0-indexed column)

Calling console.log(consumer.originalPositionFor({ line: 40, column: 9 })); returns a null mapping

Example files used in above Scenario:

Source Script

class Shape {
    constructor (id) {
        this.id = id;
    }
}
class Rectangle extends Shape {
    constructor(id) {
        super(id);
    }
}
var s = new Rectangle("Shape ID");

Transpiled Script

function _typeof(obj) { "@babel/helpers - typeof"; if (typeof Symbol ===  "function" && typeof Symbol.iterator === "symbol") { _typeof =  function _typeof(obj) { return typeof obj; }; } else { _typeof =  function _typeof(obj) { return obj && typeof Symbol ===  "function" && obj.constructor === Symbol && obj !==  Symbol.prototype ? "symbol" : typeof obj; }; } return _typeof(obj); }

function  _inherits(subClass, superClass) { if (typeof superClass !== "function"  && superClass !== null) { throw new TypeError("Super expression  must either be null or a function"); } subClass.prototype =  Object.create(superClass && superClass.prototype, { constructor:  { value: subClass, writable: true, configurable: true } }); if  (superClass) _setPrototypeOf(subClass, superClass); }

function  _setPrototypeOf(o, p) { _setPrototypeOf = Object.setPrototypeOf ||  function _setPrototypeOf(o, p) { o.__proto__ = p; return o; }; return  _setPrototypeOf(o, p); }

function _createSuper(Derived) { var  hasNativeReflectConstruct = _isNativeReflectConstruct(); return function  _createSuperInternal() { var Super = _getPrototypeOf(Derived), result;  if (hasNativeReflectConstruct) { var NewTarget =  _getPrototypeOf(this).constructor; result = Reflect.construct(Super,  arguments, NewTarget); } else { result = Super.apply(this, arguments); }  return _possibleConstructorReturn(this, result); }; }

function  _possibleConstructorReturn(self, call) { if (call &&  (_typeof(call) === "object" || typeof call === "function")) { return  call; } return _assertThisInitialized(self); }

function  _assertThisInitialized(self) { if (self === void 0) { throw new  ReferenceError("this hasn't been initialised - super() hasn't been  called"); } return self; }

function _isNativeReflectConstruct() {  if (typeof Reflect === "undefined" || !Reflect.construct) return false;  if (Reflect.construct.sham) return false; if (typeof Proxy ===  "function") return true; try {  Date.prototype.toString.call(Reflect.construct(Date, [], function ()  {})); return true; } catch (e) { return false; } }

function  _getPrototypeOf(o) { _getPrototypeOf = Object.setPrototypeOf ?  Object.getPrototypeOf : function _getPrototypeOf(o) { return o.__proto__  || Object.getPrototypeOf(o); }; return _getPrototypeOf(o); }

function  _classCallCheck(instance, Constructor) { if (!(instance instanceof  Constructor)) { throw new TypeError("Cannot call a class as a  function"); } }

var Shape = function Shape(id) {
  "use strict";

  _classCallCheck(this, Shape);

  this.id = id;
};

var Rectangle = /*#__PURE__*/function (_Shape) {
  "use strict";

  _inherits(Rectangle, _Shape);

  var _super = _createSuper(Rectangle);

  function Rectangle(id) {
    _classCallCheck(this, Rectangle);

    return _super.call(this, id);
  }

  return Rectangle;
}(Shape);

Source Map

{"version":3,"sources":["../src/class_inheritance.js"],"names":["Shape","id","Rectangle","s"],"mappings":";;;;;;;;;;;;;;;;;;IAAMA,K,GACL,eAAaC,EAAb,EAAiB;AAAA;;AAAA;;AAChB,OAAKA,EAAL,GAAUA,EAAV;AACA,C;;IAEIC,S;;;;;;;AACL,qBAAYD,EAAZ,EAAgB;AAAA;;AAAA,6BACTA,EADS;AAEf;;;EAHsBD,K;;AAKxB,IAAIG,CAAC,GAAG,IAAID,SAAJ,CAAc,UAAd,CAAR","sourcesContent":["class  Shape {\n\tconstructor (id) {\n\t\tthis.id = id;\n\t}\n}\nclass  Rectangle extends Shape {\n\tconstructor(id)  {\n\t\tsuper(id);\n\t}\n}\nvar s = new Rectangle(\"Shape  ID\");"],"file":"class_inheritance.js"}

[brad4d]

FYI, since the data structure we're talking about is defined using protocol buffers, here's a link to documentation on using those in Java.

https://developers.google.com/protocol-buffers/docs/javatutorial

1. It sounds like you want to keep the existing control flow intact, with the addition of a new enum added to OriginalMapping. That way, callers of getMappingForLine can determine whether the returned OriginalMapping is EXACT or APPROXIMATE?

I believe you understand me correctly, yes.

2. Is the UNKNOWN type intended to be a placeholder, that gets overwritten with EXACT or APPROXIMATE?

Very relevant section of the doc I already linked above: https://developers.google.com/protocol-buffers/docs/proto#updating

tl;dr is that when you add a new enum field to a proto (nickname for a particular "protocol buffer" message definition), you must always have a default value of UNKNOWN, so that when code that knows about the new field sees messages created by a source that didn't know about the field, they will be able to tell that the field is just missing and act accordingly.

This may seem irrelevant to you in this case, since you probably expect that the code that generates the messages and the code that reads them are all compiled together, but that isn't always the case. In particular I think we have cases within Google where these messages get generated by one process then consumed by another.