Skip to content

Fix Live Debugger parenthesized arrow body instrumentation #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
watson wants to merge 1 commit into
base: watson/fix-nested-functions2
Choose a base branch
from
Draft

Fix Live Debugger parenthesized arrow body instrumentation #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions packages/plugins/live-debugger/src/transform/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,48 @@ describe('transformCode', () => {
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
{
description: 'parenthesized inner arrow abutting the closing paren',
code: 'const f = (a) => ((b) => a + b);',
namedOnly: false,
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
{
description: 'parenthesized inner arrow abutting the closing paren (namedOnly)',
code: 'const f = (a) => ((b) => a + b);',
namedOnly: true,
expectedInstrumentedCount: 1,
expectedTotalFunctions: 2,
},
{
description: 'double-parenthesized inner arrow',
code: 'const f = (a) => (((b) => a + b));',
namedOnly: false,
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
{
description: 'nested parenthesized curried arrows',
code: 'const f = (a) => ((b) => ((c) => a + b + c));',
namedOnly: false,
expectedInstrumentedCount: 3,
expectedTotalFunctions: 3,
},
{
description: 'parenthesized inner arrow returning JSX',
code: 'const f = (a) => ((b) => <li>{a}{b}</li>);',
namedOnly: false,
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
{
description: 'parenthesized inner arrow returning a parenthesized object',
code: 'const f = (a) => ((b) => ({ sum: a + b }));',
namedOnly: false,
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
];

test.each(nestedSyntaxCases)(
Expand All @@ -409,6 +451,149 @@ describe('transformCode', () => {
);
});

describe('parenthesized arrow bodies with comments', () => {
// The wrapper-paren scanner must skip comments; a stray paren inside a
// comment must not be mistaken for a wrapping paren, otherwise removal
// misaligns and produces invalid JavaScript.
const commentCases = [
'const f = () => (/* ( */ x);',
'const f = () => (x /* ) */);',
'const f = () => (/* ( */ (x));',
'const f = () => ((x) /* ) */);',
'const f = () => (\n // returns (something)\n value\n);',
'const f = () => (/* le(ading */ (y) /* trai)ling */);',
// A regex/division `/` inside the body must not be treated as a
// comment start by the wrapper-paren scanner.
'const f = () => (/[)(]/);',
'const f = () => (/[)(]/g.test(x) ? a : b);',
];

test.each(commentCases.map((code) => ({ code })))(
'should produce valid output for $code',
({ code }) => {
const result = transformCode({ ...BASE_OPTIONS, code });

expect(result.instrumentedCount).toBe(1);
expect(validateSyntax(result.code, '/src/utils.ts')).toBeNull();
expect(result.code).toContain('$dd_return');
expect(result.code).toContain('catch(e)');
},
);
});

describe('async functions', () => {
const asyncCases = [
{
description: 'async function declaration with await',
code: 'async function f(a) { return await g(a); }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async arrow expression body',
code: 'const f = async (a) => await g(a);',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async arrow with unparenthesized param',
code: 'const f = async a => a;',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async arrow block body',
code: 'const f = async (a) => { return await g(a); };',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async arrow with parenthesized object body',
code: 'const f = async (a) => ({ v: await g(a) });',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async object method',
code: 'const o = { async m(a) { return await g(a); } };',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'async class method',
code: 'class C { async m(a) { return await g(a); } }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'curried async arrow with parenthesized inner arrow',
code: 'const f = async (a) => ((b) => a + b);',
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
];

test.each(asyncCases)(
'should produce valid output for $description',
({ code, expectedInstrumentedCount, expectedTotalFunctions }) => {
const result = transformCode({ ...BASE_OPTIONS, code });

expect(validateSyntax(result.code, '/src/utils.ts')).toBeNull();
expect(result.instrumentedCount).toBe(expectedInstrumentedCount);
expect(result.totalFunctions).toBe(expectedTotalFunctions);
expect(result.code).toContain('$dd_probes');
expect(result.code).toContain('catch(e)');
},
);
});

describe('accessors and static members', () => {
const accessorCases = [
{
description: 'class getter',
code: 'class C { get x() { return this._x; } }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'class setter',
code: 'class C { set x(v) { this._x = v; } }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'object getter and setter',
code: 'const o = { get x() { return 1; }, set x(v) { this._x = v; } };',
expectedInstrumentedCount: 2,
expectedTotalFunctions: 2,
},
{
description: 'static method',
code: 'class C { static m(a) { return a; } }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
{
description: 'static block alongside a method',
code: 'class C { static { init(); } m() { return 1; } }',
expectedInstrumentedCount: 1,
expectedTotalFunctions: 1,
},
];

test.each(accessorCases)(
'should produce valid output for $description',
({ code, expectedInstrumentedCount, expectedTotalFunctions }) => {
const result = transformCode({ ...BASE_OPTIONS, code });

expect(validateSyntax(result.code, '/src/utils.ts')).toBeNull();
expect(result.instrumentedCount).toBe(expectedInstrumentedCount);
expect(result.totalFunctions).toBe(expectedTotalFunctions);
expect(result.code).toContain('catch(e)');
},
);
});

describe('skipping', () => {
it('should skip generators', () => {
const result = transformCode({
Expand Down
64 changes: 53 additions & 11 deletions packages/plugins/live-debugger/src/transform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,13 @@ function injectInstrumentation(s: MagicStringType, code: string, target: Functio
// otherwise the output would be => ({block_code}) which is a syntax error.
// Babel stores nested bodies like `((1))` as body `1`, so every wrapper
// paren around the body range must be removed before injecting a block.
//
// Blank the paren with `update(..., '')` rather than `remove()`. When the
// body is itself an instrumented function whose end abuts the closing
// paren (e.g. `(a) => ((b) => a + b)`), that inner function has already
// appended its instrumentation suffix to the same offset. `remove()`
// clears the chunk's intro/outro and would drop that suffix, producing
// invalid JavaScript; `update()` edits the content only and preserves it.
if (bodyParenStart != null) {
const openingParens = getLeadingArrowBodyParens(code, bodyParenStart, bodyStart);
const closingParens = getTrailingArrowBodyParens(code, bodyEnd, functionEnd);
Expand All @@ -494,8 +501,8 @@ function injectInstrumentation(s: MagicStringType, code: string, target: Functio
for (let i = 0; i < parenCount; i++) {
const openingParen = openingParens[i];
const closingParen = closingParens[i];
s.remove(openingParen, openingParen + 1);
s.remove(closingParen, closingParen + 1);
s.update(openingParen, openingParen + 1, '');
s.update(closingParen, closingParen + 1, '');
}
}

Expand Down Expand Up @@ -812,21 +819,56 @@ function getReturnCaptureArgs(
}

function getLeadingArrowBodyParens(code: string, start: number, end: number): number[] {
const parens: number[] = [];
for (let i = start; i < end; i++) {
if (code[i] === '(') {
parens.push(i);
}
}
return parens;
return collectArrowWrapperParens(code, start, end, '(');
}

function getTrailingArrowBodyParens(code: string, start: number, end: number): number[] {
return collectArrowWrapperParens(code, start, end, ')');
}

/**
* Collect the positions of wrapper parens around an arrow expression body.
*
* Only whitespace, comments, and the wrapping parens themselves can appear
* between the outermost paren and the body (leading) or between the body and
* the arrow's end (trailing). Comments are skipped so that parens inside a
* comment are not mistaken for wrapper parens, which would otherwise misalign
* removal and emit invalid JavaScript.
*
* Any `/` in this range necessarily starts a comment (division/regex are not
* grammatically valid here), so no string/regex handling is required.
*/
function collectArrowWrapperParens(
code: string,
start: number,
end: number,
paren: '(' | ')',
): number[] {
const parens: number[] = [];
for (let i = end - 1; i >= start; i--) {
if (code[i] === ')') {
let i = start;
while (i < end) {
const char = code[i];
if (char === '/') {
const next = code[i + 1];
if (next === '/') {
i += 2;
while (i < end && code[i] !== '\n') {
i++;
}
continue;
}
if (next === '*') {
i += 2;
while (i < end && !(code[i] === '*' && code[i + 1] === '/')) {
i++;
}
i += 2;
continue;
}
} else if (char === paren) {
parens.push(i);
}
i++;
}
return parens;
}
Expand Down
Loading