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

Raise recoverable error for type members with invalid modifiers #12785

Merged
merged 9 commits into from Feb 12, 2021
Merged

Raise recoverable error for type members with invalid modifiers #12785

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e8a5fc1
tsCheckModifiers
sosukesuzuki Feb 9, 2021
9b1b5d8
Rasie error when tsParseModifiers
sosukesuzuki Feb 9, 2021
82a8e85
Fix tests
sosukesuzuki Feb 9, 2021
6739d43
Error for readonly method signature
sosukesuzuki Feb 9, 2021
ededec2
Use callback
sosukesuzuki Feb 9, 2021
c6c531a
Better interface
sosukesuzuki Feb 11, 2021
dc1cfe5
Add review
sosukesuzuki Feb 11, 2021
2a82123
Address review
sosukesuzuki Feb 11, 2021
b79fd6b
Use optional chaining
sosukesuzuki Feb 12, 2021
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
32 changes: 11 additions & 21 deletions packages/babel-parser/src/plugins/typescript/index.js
View file
Open in desktop
Expand Up @@ -210,7 +210,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
accessibility?: N.Accessibility,
},
allowedModifiers: TsModifier[],
callback?: (startPos: number, modifer: TsModifier) => void,
disallowedModifiers?: TsModifier[],
errorTemplate?: string,
): void {
for (;;) {
const startPos = this.state.start;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pass down allowedModifiers.concat(disallowedModifier) to tsParserModifier so we don't have to duplicate disallowedModifier in the tsParseModifiers call.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: allowedModifiers.concat(disallowedModifier ?? [])

Expand All @@ -231,8 +232,12 @@ export default (superClass: Class<Parser>): Class<Parser> =>
modified[modifier] = true;
}

if (callback) {
callback(startPos, modifier);
if (
errorTemplate &&
disallowedModifiers &&
disallowedModifiers.includes(modifier)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
disallowedModifiers.includes(modifier)
disallowedModifiers?.includes(modifier)

) {
this.raise(startPos, errorTemplate, modifier);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can assume errorTemplate is non null if disallowedModifiers is provided. Overall this method not a public interface so we don't have to be defensive.

}
}
}
Expand Down Expand Up @@ -594,24 +599,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
];
this.tsParseModifiers(
node,
[
"readonly",
"declare",
"abstract",
"private",
"protected",
"public",
"static",
],
(startPos, modifier) => {
if (denyModifiers.includes(modifier)) {
this.raise(
startPos,
TSErrors.InvalidModifierOnTypeMember,
modifier,
);
}
},
["readonly", ...denyModifiers],
denyModifiers,
TSErrors.InvalidModifierOnTypeMember,
);

const idx = this.tsTryParseIndexSignature(node);
Expand Down