Skip to content
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.

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

Convert parse-stack.js to TypeScript #10653

Merged
merged 8 commits into from
Apr 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion tools/tests/build-errors.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
var selftest = require('../tool-testing/selftest.js');
var Sandbox = selftest.Sandbox;

// This test was originally written to test the behavior of parse-stack.js when
// This test was originally written to test the behavior of parse-stack.ts when
// there's a colon in a filename. We now try a lot harder to avoid putting
// colons in filenames. But it's still a decent test that errors in legacy
// source handlers work.
Expand Down
2 changes: 1 addition & 1 deletion tools/tests/parse-stack-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import selftest from '../tool-testing/selftest.js';
import { parse, markBottom } from '../utils/parse-stack.js';
import { parse, markBottom } from '../utils/parse-stack';
import _ from 'underscore';
import Fiber from 'fibers';
import Future from 'fibers/future';
Expand Down
3 changes: 1 addition & 2 deletions tools/tool-testing/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@
import { spawn } from 'child_process';
import * as files from '../fs/files';
import {
markTop as parseStackMarkTop,
parse as parseStackParse,
} from '../utils/parse-stack.js';
} from '../utils/parse-stack';
import { Console } from '../console/console.js';
import Matcher from './matcher.js';
import OutputLog from './output-log.js';
Expand Down
2 changes: 1 addition & 1 deletion tools/tool-testing/selftest.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
markBottom as parseStackMarkBottom,
markTop as parseStackMarkTop,
parse as parseStackParse,
} from '../utils/parse-stack.js';
} from '../utils/parse-stack';
import { Console } from '../console/console.js';
import { loadIsopackage } from '../tool-env/isopackets.js';
import TestFailure from './test-failure.js';
Expand Down
2 changes: 1 addition & 1 deletion tools/utils/buildmessage.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
var _ = require('underscore');
var files = require('../fs/files');
var parseStack = require('./parse-stack.js');
var parseStack = require('./parse-stack');
var fiberHelpers = require('./fiber-helpers.js');
var Progress = require('../console/progress').Progress;

Expand Down
120 changes: 75 additions & 45 deletions tools/utils/parse-stack.js → tools/utils/parse-stack.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,40 @@
const _ = require('underscore');

// Given an Error (eg, 'new Error'), return the stack associated with
// that error as an array. More recently called functions appear first
// and each element is an object with keys:
// - file: filename as it appears in the stack
// - line: 1-indexed line number in file, as a Number
// - column: 1-indexed column in line, as a Number
// - func: name of the function in the frame (maybe null)
//
// Accomplishes this by parsing the text representation of the stack
// with regular expressions. Unlikely to work anywhere but v8.
//
// If a function on the stack has been marked with mark(), don't
// return anything past that function. We call this the "user portion"
// of the stack.
export function parse(err) {
type ParsedStackFrame = {
/**
* filename as it appears in the stack
*/
file: string;

/**
* 1-indexed line in the file
*/
line: number | null;

/**
* 1-indexed column in the line
*/
column: number | null;

/**
* name of the function in the frame
*/
func: string | null;
};

/**
* Returns the stack associated with an error as an array.
* More recently called functions appear first.
*
* Accomplishes this by parsing the text representation of the stack
* with regular expressions. Unlikey to work anywhere but v8.
*
* If a function on the stack has been marked with mark(), will not
* return anything past that function. We call this the "user portion"
* of the stack.
*/
export function parse(err: Error): {
insideFiber?: ParsedStackFrame[],
outsideFiber?: ParsedStackFrame[],
} {
const stack = err.stack;
if (typeof stack !== "string") {
return {};
Expand Down Expand Up @@ -56,38 +76,45 @@ export function parse(err) {
};
}

// Decorator. Mark the point at which a stack trace returned by
// parse() should stop: no frames earlier than this point will be
// included in the parsed stack. Confusingly, in the argot of the
// times, you'd say that frames "higher up" than this or "above" this
// will not be returned, but you'd also say that those frames are "at
// the bottom of the stack". Frames below the bottom are the outer
// context of the framework running the user's code.
export function markBottom(f, context) {
/**
* Decorator. Mark the point at which a stack trace returned by
* parse() should stop: no frames earlier than this point will be
* included in the parsed stack. Confusingly, in the argot of the
* times, you'd say that frames "higher up" than this or "above" this
* will not be returned, but you'd also say that those frames are "at
* the bottom of the stack". Frames below the bottom are the outer
* context of the framework running the user's code.
*/
export function markBottom(f: Function, context: any) {
/* eslint-disable camelcase */
return function __bottom_mark__() {
// @ts-ignore: Implicit this
return f.apply(context || this, arguments);
};
/* eslint-enable camelcase */
}

// Decorator. Mark the point at which a stack trace returned by
// parse() should begin: no frames later than this point will be
// included in the parsed stack. The opposite of markBottom().
// Frames above the top are helper functions defined by the
// framework and executed by user code whose internal behavior
// should not be exposed.
export function markTop(f) {
/**
* Decorator. Mark the point at which a stack trace returned by
* parse() should begin: no frames later than this point will be
* included in the parsed stack. The opposite of markBottom().
* Frames above the top are helper functions defined by the
* framework and executed by user code whose internal behavior
* should not be exposed.
*/
export function markTop(f: Function, context: any) {
/* eslint-disable camelcase */
return function __top_mark__() {
return f.apply(this, arguments);
// @ts-ignore: Implicit this
return f.apply(context || this, arguments);
};
/* eslint-enable camelcase */
}

function parseStackFrames(frames) {
function parseStackFrames(frames: string[]): ParsedStackFrame[] {
let stop = false;
let ret = [];
let parsedFrames: ParsedStackFrame[] = [];

frames.some(frame => {
if (stop) {
return true;
Expand All @@ -110,28 +137,31 @@ function parseStackFrames(frames) {
// m[1] could be Object.__top_mark__ or something like that
// depending on where exactly you put the function returned by
// markTop
ret = [];
parsedFrames = [];
return;
}

if (m[1].match(/(?:^|\.)__bottom_mark__$/)) {
return stop = true;
}
ret.push({

parsedFrames.push({
func: m[1],
file: m[5],
line: m[7] ? +m[7] : undefined,
column: m[9] ? +m[9] : undefined
line: m[7] ? +m[7] : null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆗 I wondered if these changes from undefined to null would be safe, and it looks ok, because the checks in buildmessage.js are for thruthiness:

if (message.line) {
line += ":" + message.line;
if (message.column) {
// XXX maybe exclude unless specifically requested (eg,
// for an automated tool that's parsing our output?)
line += ":" + message.column;
}
}

if (message.func && stack.length <= 1) {
line += " (at " + message.func + ")";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for double-checking and confirming!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to manually use null when I'm marking something for falsyness while otherwise leaving undefined as a feature of the language that tells me when something is actually undefined and never manually using undefined if I can avoid it. It's a semantic difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I feel the same way.

column: m[9] ? +m[9] : null
});
return;
}
/* eslint-enable max-len */

if (m = frame.match(/^\s*at\s+(.+?)(:(\d+))?(:(\d+))?\s*$/)) {
// " at /path/to/myfile.js:532:39"
ret.push({
parsedFrames.push({
file: m[1],
line: m[3] ? +m[3] : undefined,
column: m[5] ? +m[5] : undefined
line: m[3] ? +m[3] : null,
column: m[5] ? +m[5] : null,
func: null,
});
return;
}
Expand All @@ -147,14 +177,14 @@ function parseStackFrames(frames) {
return stop = true;
}

if (_.isEmpty(ret)) {
if (parsedFrames.length === 0) {
// We haven't found any stack frames, so probably we have newlines in the
// error message. Just skip this line.
return;
}

throw new Error("Couldn't parse stack frame: '" + frame + "'");
throw new Error(`Couldn't parse stack frame: '${frame}'`);
});

return ret;
return parsedFrames;
}