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

Simplifiy tracking of valid JSX positions #13891

Merged
merged 6 commits into from Nov 4, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/babel-parser/src/parser/expression.js
Expand Up @@ -22,12 +22,12 @@ import {
tokenCanStartExpression,
tokenIsAssignment,
tokenIsIdentifier,
tokenIsKeyword,
tokenIsKeywordOrIdentifier,
tokenIsOperator,
tokenIsPostfix,
tokenIsPrefix,
tokenIsRightAssociative,
tokenKeywordOrIdentifierIsKeyword,
tokenLabelName,
tokenOperatorPrecedence,
tt,
Expand Down Expand Up @@ -2236,8 +2236,6 @@ export default class ExpressionParser extends LValParser {
prop.key = this.parseMaybeAssignAllowIn();
this.expect(tt.bracketR);
} else {
const oldInPropertyName = this.state.inPropertyName;
this.state.inPropertyName = true;
// We check if it's valid for it to be a private name when we push it.
const type = this.state.type;
(prop: $FlowFixMe).key =
Expand All @@ -2252,8 +2250,6 @@ export default class ExpressionParser extends LValParser {
// ClassPrivateProperty is never computed, so we don't assign in that case.
prop.computed = false;
}

this.state.inPropertyName = oldInPropertyName;
}

return prop.key;
Expand Down Expand Up @@ -2583,12 +2579,16 @@ export default class ExpressionParser extends LValParser {
throw this.unexpected();
}

const tokenIsKeyword = tokenKeywordOrIdentifierIsKeyword(type);

if (liberal) {
// If the current token is not used as a keyword, set its type to "tt.name".
// This will prevent this.next() from throwing about unexpected escapes.
this.state.type = tt.name;
if (tokenIsKeyword) {
this.replaceToken(tt.name);
Copy link
Member

Choose a reason for hiding this comment

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

Is updateContext needed when replacing a keyword with tt.name? (I don't think it is).
Because we could just to this.state.type = tt.name here (and maybe this.state.exprAllowed = false, even if I don't see why it would be needed) and keep the prevType handling all in the JSX plugin (to avoid needing the type error suppression comment).

Copy link
Member

Choose a reason for hiding this comment

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

Uh, or maybe it would break this:

foo.else
<X>2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The updateContext is currently required by the jsx plugin, to disable JSX element forming in the following situations, where a keyword token was re-interpreted as an identifier token and thus it can not start a JSX element:

x.case <jsx ...
x?.case <jsx ...
class C { case <jsx ...

}
} else {
this.checkReservedWord(name, start, tokenIsKeyword(type), false);
this.checkReservedWord(name, start, tokenIsKeyword, false);
}

this.next();
Expand Down
34 changes: 8 additions & 26 deletions packages/babel-parser/src/plugins/jsx/index.js
Expand Up @@ -21,10 +21,6 @@ import { isIdentifierChar, isIdentifierStart } from "../../util/identifier";
import type { Position } from "../../util/location";
import { isNewLine } from "../../util/whitespace";
import { Errors, makeErrorTemplates, ErrorCodes } from "../../parser/error";
import type { LookaheadState } from "../../tokenizer/state";
import State from "../../tokenizer/state";

type JSXLookaheadState = LookaheadState & { inPropertyName: boolean };

const HEX_NUMBER = /^[\da-fA-F]+$/;
const DECIMAL_NUMBER = /^\d+$/;
Expand Down Expand Up @@ -106,7 +102,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
case charCodes.lessThan:
case charCodes.leftCurlyBrace:
if (this.state.pos === this.state.start) {
if (ch === charCodes.lessThan && this.state.exprAllowed) {
if (ch === charCodes.lessThan && this.state.canStartJSXElement) {
++this.state.pos;
return this.finishToken(tt.jsxTagStart);
}
Expand Down Expand Up @@ -556,24 +552,14 @@ export default (superClass: Class<Parser>): Class<Parser> =>
) {
// In case we encounter an lt token here it will always be the start of
// jsx as the lt sign is not allowed in places that expect an expression
this.finishToken(tt.jsxTagStart);
this.replaceToken(tt.jsxTagStart);
return this.jsxParseElement();
} else {
return super.parseExprAtom(refExpressionErrors);
}
}

createLookaheadState(state: State): JSXLookaheadState {
const lookaheadState = ((super.createLookaheadState(
state,
): any): JSXLookaheadState);
lookaheadState.inPropertyName = state.inPropertyName;
return lookaheadState;
}

getTokenFromCode(code: number): void {
if (this.state.inPropertyName) return super.getTokenFromCode(code);

const context = this.curContext();

if (context === tc.j_expr) {
Expand All @@ -600,7 +586,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>

if (
code === charCodes.lessThan &&
this.state.exprAllowed &&
this.state.canStartJSXElement &&
this.input.charCodeAt(this.state.pos + 1) !== charCodes.exclamationMark
) {
++this.state.pos;
Expand All @@ -617,7 +603,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// do not consider JSX expr -> JSX open tag -> ... anymore
// reconsider as closing tag context
context.splice(-2, 2, tc.j_cTag);
this.state.exprAllowed = false;
this.state.canStartJSXElement = false;
} else if (type === tt.jsxTagStart) {
context.push(
tc.j_expr, // treat as beginning of JSX expression
Expand All @@ -627,17 +613,13 @@ export default (superClass: Class<Parser>): Class<Parser> =>
const out = context.pop();
if ((out === tc.j_oTag && prevType === tt.slash) || out === tc.j_cTag) {
context.pop();
this.state.exprAllowed = context[context.length - 1] === tc.j_expr;
this.state.canStartJSXElement =
context[context.length - 1] === tc.j_expr;
} else {
this.state.exprAllowed = true;
this.state.canStartJSXElement = true;
}
} else if (
tokenIsKeyword(type) &&
(prevType === tt.dot || prevType === tt.questionDot)
) {
this.state.exprAllowed = false;
} else {
this.state.exprAllowed = tokenComesBeforeExpression(type);
this.state.canStartJSXElement = tokenComesBeforeExpression(type);
}
}
};
2 changes: 1 addition & 1 deletion packages/babel-parser/src/plugins/typescript/index.js
Expand Up @@ -2125,7 +2125,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// When ! is consumed as a postfix operator (non-null assertion),
// disallow JSX tag forming after. e.g. When parsing `p! < n.p!`
// `<n.p` can not be a start of JSX tag
this.state.exprAllowed = false;
this.state.canStartJSXElement = false;
this.next();

const nonNullExpression: N.TsNonNullExpression = this.startNodeAt(
Expand Down
10 changes: 9 additions & 1 deletion packages/babel-parser/src/tokenizer/index.js
Expand Up @@ -482,7 +482,7 @@ export default class Tokenizer extends ParserErrors {
}

// Called at the end of every token. Sets `end`, `val`, and
// maintains `context` and `exprAllowed`, and skips the space after
// maintains `context` and `canStartJSXElement`, and skips the space after
// the token, so that the next one's `start` will point at the
// right position.

Expand All @@ -498,6 +498,14 @@ export default class Tokenizer extends ParserErrors {
}
}

replaceToken(type: TokenType): void {
this.state.type = type;
// the prevType of updateContext is required
// only when the new type is tt.slash/tt.jsxTagEnd
// $FlowIgnore
Copy link
Member

Choose a reason for hiding this comment

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

We might consider marking it as optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am working on completely removing updateContext() in base parser and jsx parser, at least currently we don't need updateContext in the base parser so hopefully replaceToken can be moved to the jsx plugin.

this.updateContext();
}

// ### Token reading

// This is the function that is called to fetch the next token. It
Expand Down
3 changes: 1 addition & 2 deletions packages/babel-parser/src/tokenizer/state.js
Expand Up @@ -68,7 +68,6 @@ export default class State {
maybeInArrowParameters: boolean = false;
inType: boolean = false;
noAnonFunctionType: boolean = false;
inPropertyName: boolean = false;
hasFlowComment: boolean = false;
isAmbientContext: boolean = false;
inAbstractClass: boolean = false;
Expand Down Expand Up @@ -127,7 +126,7 @@ export default class State {
// or ends a string template
context: Array<TokContext> = [ct.brace];
// Used to track whether a JSX element is allowed to form
exprAllowed: boolean = true;
canStartJSXElement: boolean = true;

// Used to signal to callers of `readWord1` whether the word
// contained any escape sequences. This is needed because words with
Expand Down
6 changes: 6 additions & 0 deletions packages/babel-parser/src/tokenizer/types.js
Expand Up @@ -330,6 +330,12 @@ export function tokenIsIdentifier(token: TokenType): boolean {
return token >= tt._as && token <= tt.name;
}

export function tokenKeywordOrIdentifierIsKeyword(token: TokenType): boolean {
// we can remove the token >= tt._in check when we
// know a token is either keyword or identifier
return token <= tt._while;
}

export function tokenIsKeywordOrIdentifier(token: TokenType): boolean {
return token >= tt._in && token <= tt.name;
}
Expand Down