Skip to content

Commit 51f0ad7

Browse files
Connor ClarkDevtools-frontend LUCI CQ
authored andcommitted
[scopes] Map to original positions for AST-created scopes
When a source map does not contain scope info, scopes are created from the AST. However, the positions created for the original scope were incorrectly using generated positions. Update createFromAst: * Map to original positions for original scopes * Generate original scopes in a properly structured tree, one root scope per original source file Fixed: 461572921 Change-Id: I21e7db0e0e6ea95e553d97dc32593092e19bc86e Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7156875 Commit-Queue: Connor Clark <cjamcl@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Auto-Submit: Connor Clark <cjamcl@chromium.org>
1 parent 4101661 commit 51f0ad7

3 files changed

Lines changed: 296 additions & 36 deletions

File tree

front_end/core/sdk/SourceMap.test.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1350,17 +1350,29 @@ describeWithEnvironment('SourceMap', () => {
13501350
});
13511351

13521352
it('builds scopes fallback when the source map does not have any scope information', async () => {
1353+
// TODO: this test fails when this experiment is on, because "hasScopeInfo"
1354+
// returns true, because addOriginalScopes is called in parseMap. Explicitly
1355+
// disable the experiment for now because otherwise it will be enabled incidentally
1356+
// from previous tests in this file, which results in different results when
1357+
// running this test directly vs all together.
1358+
//
1359+
// This should be resolved: presently it seems that when this experiment is on,
1360+
// the "fallback" scopes are never generated (only blank ones are).
1361+
Root.Runtime.experiments.disableForTest(Root.Runtime.ExperimentName.USE_SOURCE_MAP_SCOPES);
1362+
13531363
const scopeTreeStub = sinon.stub(Formatter.FormatterWorkerPool.formatterWorkerPool(), 'javaScriptScopeTree')
1354-
.returns(Promise.resolve({start: 0, end: 10, variables: [], kind: 1, children: []}));
1364+
.returns(Promise.resolve({start: 0, end: 38, variables: [], kind: 1, children: []}));
13551365
const script = sinon.createStubInstance(SDK.Script.Script, {
13561366
requestContentData: Promise.resolve(
13571367
new TextUtils.ContentData.ContentData('function f() { console.log("hello"); }', false, 'text/javascript'))
13581368
});
13591369
const sourceMap = new SDK.SourceMap.SourceMap(
13601370
compiledUrl, sourceMapJsonUrl, {
13611371
version: 3,
1362-
mappings: 'ACAA', // [0, 1, 0, 0]
1363-
sources: [],
1372+
// [ 0, 1, 0, 0]
1373+
// [37, 0, 0, 0]
1374+
mappings: 'ACAA,qCAAA',
1375+
sources: ['module1.js', 'module2.js'],
13641376
names: [],
13651377
},
13661378
script);
@@ -1369,7 +1381,7 @@ describeWithEnvironment('SourceMap', () => {
13691381

13701382
// Trigger processing.
13711383
assert.isNotNull(sourceMap.findEntry(0, 0));
1372-
await new Promise(r => setTimeout(r, 0)); // Wait one event loop tick.
1384+
await sourceMap.scopesFallbackPromiseForTest;
13731385

13741386
assert.isTrue(sourceMap.hasScopeInfo());
13751387
sinon.assert.calledOnceWithExactly(scopeTreeStub, 'function f() { console.log("hello"); }', 'script');

front_end/core/sdk/SourceMapScopesInfo.test.ts

Lines changed: 139 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -955,24 +955,154 @@ describe('SourceMapScopesInfo', () => {
955955

956956
const sourceMapJSON = encodeSourceMap([
957957
'0:10 => original.js:0:10@foo', // function f() => function foo()
958-
'0:44 => original.js:5:9@bar', // function b() => function bar()
958+
'0:33 => original.js:3:0', // end of f
959+
'0:44 => original2.js:5:9@bar', // function b() => function bar()
960+
'0:57 => original2.js:7:0', // end of b
959961
]);
960962
const sourceMap = new SDK.SourceMap.SourceMap(urlString`compiled.js`, urlString`compiled.js.map`, sourceMapJSON);
961963

962964
const info = SourceMapScopesInfo.createFromAst(sourceMap, ast, new TextUtils.Text.Text(generatedCode));
963965

964-
// Check function name for a position at the beginning of the function name mapping.
965-
assert.strictEqual(info.findOriginalFunctionName({line: 0, column: 10}), 'foo');
966-
967-
// Check function name for a position inside the function body.
968-
assert.strictEqual(info.findOriginalFunctionName({line: 0, column: 25}), 'foo');
966+
// Check function name/scope for a position at the beginning of the function name mapping,
967+
// and for a position at the beginning of the function name mapping.
968+
for (const column of [10, 25]) {
969+
assert.strictEqual(info.findOriginalFunctionName({line: 0, column}), 'foo');
970+
971+
const {scope, url} = info.findOriginalFunctionScope({line: 0, column}) ?? {};
972+
assert.strictEqual(url, 'original.js');
973+
assert.isOk(scope);
974+
assert.strictEqual(scope.name, 'foo');
975+
assert.strictEqual(scope.start.line, 0);
976+
assert.strictEqual(scope.start.column, 10);
977+
assert.strictEqual(scope.end.line, 3);
978+
assert.strictEqual(scope.end.column, 0);
979+
}
969980

970-
// Check function name for the second function.
971-
assert.strictEqual(info.findOriginalFunctionName({line: 0, column: 44}), 'bar');
972-
assert.strictEqual(info.findOriginalFunctionName({line: 0, column: 52}), 'bar');
981+
// Check function name/scope for the second function.
982+
for (const column of [44, 52]) {
983+
assert.strictEqual(info.findOriginalFunctionName({line: 0, column}), 'bar');
984+
985+
const {scope, url} = info.findOriginalFunctionScope({line: 0, column}) ?? {};
986+
assert.strictEqual(url, 'original2.js');
987+
assert.isOk(scope);
988+
assert.strictEqual(scope.name, 'bar');
989+
assert.strictEqual(scope.start.line, 5);
990+
assert.strictEqual(scope.start.column, 9);
991+
assert.strictEqual(scope.end.line, 7);
992+
assert.strictEqual(scope.end.column, 0);
993+
}
973994

974995
// Check a position in the global scope.
975996
assert.isNull(info.findOriginalFunctionName({line: 0, column: 38})); // Between the two functions
997+
assert.isNull(info.findOriginalFunctionScope({line: 0, column: 38}));
998+
});
999+
1000+
it('handles nested scopes across multiple original files', () => {
1001+
// Generated:
1002+
// main.js: function outer() { function inner() { console.log('hi'); } }
1003+
// util.js: function util() {}
1004+
const generatedCode = `function outer() { function inner() { console.log('hi'); } } function util() {}`;
1005+
1006+
const ast = Formatter.ScopeParser.parseScopes(generatedCode)?.export();
1007+
1008+
const sourceMapJSON = encodeSourceMap([
1009+
// main.js
1010+
'0:14 => main.js:0:14@outer', // outer start
1011+
'0:33 => main.js:0:33@inner', // inner start (inside outer)
1012+
'0:58 => main.js:0:58', // inner end
1013+
'0:60 => main.js:0:60', // outer end
1014+
1015+
// utils.js
1016+
'0:74 => utils.js:0:14@util', // util start
1017+
'0:79 => utils.js:0:18', // util end
1018+
]);
1019+
1020+
const sourceMap = new SDK.SourceMap.SourceMap(urlString`compiled.js`, urlString`compiled.js.map`, sourceMapJSON);
1021+
const info = SourceMapScopesInfo.createFromAst(sourceMap, ast!, new TextUtils.Text.Text(generatedCode));
1022+
1023+
// Test inner scope.
1024+
for (let i = 33; i < 58; i++) {
1025+
const result = info.findOriginalFunctionScope({line: 0, column: i});
1026+
assert.isOk(result);
1027+
assert.strictEqual(result.url, 'main.js');
1028+
assert.strictEqual(result.scope.name, 'inner');
1029+
assert.strictEqual(result.scope.parent?.name, 'outer');
1030+
}
1031+
1032+
// Test scope from second file.
1033+
for (let i = 74; i < 79; i++) {
1034+
const result = info.findOriginalFunctionScope({line: 0, column: i});
1035+
assert.isOk(result);
1036+
assert.strictEqual(result.url, 'utils.js');
1037+
assert.strictEqual(result.scope.name, 'util');
1038+
assert.isUndefined(result.scope.parent?.name);
1039+
}
1040+
});
1041+
1042+
it('discards scopes where start and end map to different source files', () => {
1043+
const generatedCode = `function bad() { }`;
1044+
const ast = Formatter.ScopeParser.parseScopes(generatedCode)?.export();
1045+
1046+
const sourceMapJSON = encodeSourceMap([
1047+
// The start maps to file A.
1048+
'0:12 => fileA.js:0:12@bad',
1049+
// The end maps to a different file.
1050+
'0:18 => fileB.js:0:0',
1051+
]);
1052+
1053+
const sourceMap = new SDK.SourceMap.SourceMap(urlString`compiled.js`, urlString`compiled.js.map`, sourceMapJSON);
1054+
const info = SourceMapScopesInfo.createFromAst(sourceMap, ast!, new TextUtils.Text.Text(generatedCode));
1055+
1056+
// Although the AST found a function, the source map is invalid.
1057+
for (let i = 0; i < 20; i++) {
1058+
const result = info.findOriginalFunctionScope({line: 0, column: i});
1059+
assert.isNull(result, 'Should not return a scope when source files mismatch');
1060+
}
1061+
});
1062+
1063+
it('inserts scopes correctly when an inner AST node maps to a larger scope than its parent', () => {
1064+
// Scenario:
1065+
//
1066+
// AST: In the generated code, 'wrapper' encompasses 'big'
1067+
// Original: According to the source map, 'big' actually encompasses 'wrapper'
1068+
//
1069+
// This may be a contrived and unlikely example, but the point is to ensure
1070+
// that the children scopes are kept in the correct order no matter what the
1071+
// mappings are.
1072+
1073+
const generatedCode = `function wrapper() { function big() { } }`;
1074+
const ast = Formatter.ScopeParser.parseScopes(generatedCode)?.export();
1075+
1076+
const sourceMapJSON = encodeSourceMap([
1077+
// 'wrapper' maps to a small range in original.js
1078+
'0:16 => original.js:10:16@wrapper',
1079+
1080+
// 'big' maps to a huge enclosing range in original.js
1081+
'0:33 => original.js:0:0@big',
1082+
'0:39 => original.js:100:0', // big ends way later
1083+
1084+
'0:41 => original.js:11:0', // wrapper ends
1085+
]);
1086+
1087+
const sourceMap = new SDK.SourceMap.SourceMap(urlString`compiled.js`, urlString`compiled.js.map`, sourceMapJSON);
1088+
const info = SourceMapScopesInfo.createFromAst(sourceMap, ast!, new TextUtils.Text.Text(generatedCode));
1089+
1090+
// Check 'big'.
1091+
const big = info.findOriginalFunctionScope({line: 0, column: 33});
1092+
assert.isOk(big);
1093+
assert.strictEqual(big.url, 'original.js');
1094+
assert.strictEqual(big.scope.name, 'big');
1095+
assert.strictEqual(big.scope.start.line, 0);
1096+
assert.strictEqual(big.scope.end.line, 100);
1097+
assert.isNotEmpty(big.scope.children, 'The outer scope should have adopted the inner scope');
1098+
assert.strictEqual(big.scope.children[0].name, 'wrapper');
1099+
1100+
// Check 'wrapper'.
1101+
const wrapper = info.findOriginalFunctionScope({line: 0, column: 16});
1102+
assert.isOk(wrapper);
1103+
assert.strictEqual(wrapper.url, 'original.js');
1104+
assert.strictEqual(wrapper.scope.name, 'wrapper');
1105+
assert.isEmpty(wrapper.scope.children);
9761106
});
9771107
});
9781108

0 commit comments

Comments
 (0)