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

Fix some incorrect typeof parsing in flow #10657

Merged
merged 1 commit into from Nov 18, 2019
Merged

Conversation

existentialism
Copy link
Member

@existentialism existentialism commented Nov 5, 2019

Q                       A
Fixed Issues? Fixes #10571
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Went and matched parsing behavior in flow parser v0.111.1, and the error message in cases where user wasn't technically overwriting.

@existentialism existentialism added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: flow pkg: parser labels Nov 5, 2019
@buildsize
Copy link

buildsize bot commented Nov 5, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.78 MB 2.78 MB 210 bytes (0%)
babel-preset-env.min.js 1.67 MB 1.67 MB 91 bytes (0%)
babel.js 2.96 MB 2.96 MB 210 bytes (0%)
babel.min.js 1.63 MB 1.63 MB 91 bytes (0%)
test262.tap 4.84 MB [deleted]

@JLHwung JLHwung self-requested a review November 5, 2019 18:54
@@ -1134,12 +1151,12 @@ export default (superClass: Class<Parser>): Class<Parser> =>
): N.FlowQualifiedTypeIdentifier {
startPos = startPos || this.state.start;
startLoc = startLoc || this.state.startLoc;
let node = id || this.parseIdentifier();
let node = id || this.parseIdentifier(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also replace by flowParseRestrictedIdentifier here because

interface I extends bool.member {}

will throw unexpected reserved type, though the error message is not optimal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with tests!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I don't "agree" with some outputs, bug given that this matches Flow ✔️

@@ -22,12 +22,15 @@ import {
SCOPE_OTHER,
} from "../util/scopeflags";

const reservedTypes = [
const reservedTypes = new Set([
"_",
Copy link
Member

Choose a reason for hiding this comment

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

I wish there was something like https://eslint.org/docs/rules/sort-keys for arrays

@@ -0,0 +1,2 @@
// @flow
const x: typeof type.interface = "hi";
Copy link
Member

Choose a reason for hiding this comment

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

I would report this as bug to the flow team, given that type.interface is valid in JS 🤔

@@ -0,0 +1,15 @@
// @flow
const a: typeof default = "hi";
Copy link
Member

Choose a reason for hiding this comment

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

This also seem like a flow bug, since default is not a valid variable name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Babel chokes on valid Flow syntax (typeof blah.default)
3 participants