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.

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

Jump to bottom

Improve errors location tracking #14130

Merged
merged 6 commits into from Jan 18, 2022
Merged

Improve errors location tracking #14130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
57d6e98
Have raise() take a Position instead of an integer index so that we c…
tolmasky Jan 10, 2022
65b7aae
Update test results to reflect that Errors.ElementAfterRest is now re…
tolmasky Jan 14, 2022
2d355b4
Fix linter error by changing return type to boolean.
tolmasky Jan 14, 2022
42ebf70
Corresponding change in typescript's checkCommaAfterRest.
tolmasky Jan 14, 2022
3014752
Make checkExpressionErrors return whether it has errors again, but we…
tolmasky Jan 17, 2022
22d3112
Use a WeakMap to associate indexes to Positions until we decide wheth…
tolmasky Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion eslint/babel-eslint-plugin-development-internal/src/rules/dry-error-messages.ts
View file
Open in desktop
Expand Up @@ -146,7 +146,7 @@ export default {
"CallExpression[callee.type='MemberExpression'][callee.object.type='ThisExpression'][callee.property.name='raise'][arguments.length>=2]"(
node,
) {
const [, errorMsgNode] = node.arguments;
const [errorMsgNode] = node.arguments;
const nodesToCheck = findIdNodes(errorMsgNode);

if (
Expand Down
144 changes: 72 additions & 72 deletions eslint/babel-eslint-plugin-development-internal/test/rules/dry-error-messages.js
View file
Open in desktop

Large diffs are not rendered by default.

44 changes: 24 additions & 20 deletions packages/babel-parser/src/parser/error.js
View file
Open in desktop
@@ -1,8 +1,9 @@
// @flow
/* eslint sort-keys: "error" */
import { getLineInfo, type Position } from "../util/location";
import { type Position, indexes } from "../util/location";
import CommentsParser from "./comments";
import { type ErrorCode, ErrorCodes } from "./error-codes";
import { type Node } from "../types";

// This function is used to raise exceptions on parse errors. It
// takes an offset integer (into the current `input`) to indicate
Expand All @@ -28,7 +29,14 @@ export type ErrorTemplates = {
[key: string]: ErrorTemplate,
};

type SyntaxPlugin = "flow" | "typescript" | "jsx" | typeof undefined;
type Origin = {| node: Node |} | {| at: Position |};

type SyntaxPlugin =
| "flow"
| "typescript"
| "jsx"
| "placeholders"
| typeof undefined;

function keepReasonCodeCompat(reasonCode: string, syntaxPlugin: SyntaxPlugin) {
if (!process.env.BABEL_8_BREAKING) {
Expand Down Expand Up @@ -64,31 +72,26 @@ export {
SourceTypeModuleErrorMessages as SourceTypeModuleErrors,
} from "./error-message";

export type raiseFunction = (number, ErrorTemplate, ...any) => void;
export type raiseFunction = (ErrorTemplate, Origin, ...any) => void;
export type ErrorData = {| message: ErrorTemplate, loc: Position |};

export default class ParserError extends CommentsParser {
// Forward-declaration: defined in tokenizer/index.js
/*::
+isLookahead: boolean;
*/

getLocationForPosition(pos: number): Position {
let loc;
if (pos === this.state.start) loc = this.state.startLoc;
else if (pos === this.state.lastTokStart) loc = this.state.lastTokStartLoc;
else if (pos === this.state.end) loc = this.state.endLoc;
else if (pos === this.state.lastTokEnd) loc = this.state.lastTokEndLoc;
else loc = getLineInfo(this.input, pos);

return loc;
}

raise(
pos: number,
{ code, reasonCode, template }: ErrorTemplate,
origin: Origin,
...params: any
): Error | empty {
Comment on lines 84 to 88
Copy link
Member

Choose a reason for hiding this comment

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

To avoid having to always specify a new object when calling .raise, maybe we could divide it in two separate functions:

raise(pos: Position, { code, reasonCode, template }: ErrorTemplate, ...params: any): Error | empty {
  return this.raiseWithData(pos, { code, reasonCode }, template, ...params);
}

raiseAtNode(node: Node, { code, reasonCode, template }: ErrorTemplate, ...params: any): Error | empty {
  return this.raiseWithData(origin.node.loc.start, { code, reasonCode }, template, ...params);
}

Copy link
Contributor Author

@tolmasky tolmasky Jan 16, 2022 •
edited

Choose a reason for hiding this comment

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

I actually originally had this, but decided against it for a couple reasons:

  1. I wanted it to read like throwing an error "throw SomeError(metadata)", which calls out what matters most at that line "hey, this is a MissingSemicolonError" or "hey, this is RestTrailingCommaError" vs. front-loading the information that matters least, the associated metadata, when reading the code, like doing throw "metadata on SomeError";
  2. I think we will potentially want to add more to this object later. As I mentioned in the above, we may eventually want to expand this to { at: length: } for ranges (or { start, end } or however you want to do that), and I think it would also eventually be nice to be able to supply context info (we currently have this info but it gets stringified for example, but can be very useful to the user to be able to do error.enumName vs. parsing message for enumName). As such, I wanted to put in the scaffolding for something that could carry additional info that could be easily read in the future.
  3. From my testing there seems to be zero performance benefit to avoiding object creation at this point, for two reasons. First, it is a seldom occurring event, as it happens once per error raise (vs. getLineInfo which was an iterative event that can lead to an O(N) slowdown per Error, where N = source length). Secondly, if we did consider it to be performance sensitive, then we'd for example be much better served by instead changing raiseWithData to take (missingPlugin?: Array<string>, code?: string) instead of { missingPlugin?: Array<string>, code?: string, } (since this object data object gets thrown away immediately when yet another object is created on the corresponding call to _raise), and this change would basically affect all error raises "for free" transparently since it's an internal function call in raise. However, I also don't think this is a performance issue.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good 👍

return this.raiseWithData(pos, { code, reasonCode }, template, ...params);
return this.raiseWithData(
origin.node ? origin.node.loc.start : origin.at,
{ code, reasonCode },
template,
...params,
);
}

/**
Expand All @@ -104,11 +107,12 @@ export default class ParserError extends CommentsParser {
* @memberof ParserError
*/
raiseOverwrite(
pos: number,
loc: Position,
{ code, template }: ErrorTemplate,
...params: any
): Error | empty {
const loc = this.getLocationForPosition(pos);
// $FlowIgnore[incompatible-type] We know this exists, so it can't be undefined.
const pos: number = indexes.get(loc);
const message =
template.replace(/%(\d+)/g, (_, i: number) => params[i]) +
` (${loc.line}:${loc.column})`;
Expand All @@ -127,15 +131,15 @@ export default class ParserError extends CommentsParser {
}

raiseWithData(
pos: number,
loc: Position,
data?: {
missingPlugin?: Array<string>,
code?: string,
},
errorTemplate: string,
...params: any
): Error | empty {
const loc = this.getLocationForPosition(pos);
const pos = indexes.get(loc);
const message =
errorTemplate.replace(/%(\d+)/g, (_, i: number) => params[i]) +
` (${loc.line}:${loc.column})`;
Expand Down