Skip to content

Fix some incorrect typeof parsing in flow#10657

Merged
nicolo-ribaudo merged 1 commit intomasterfrom
flow-default
Nov 18, 2019
Merged

Fix some incorrect typeof parsing in flow#10657
nicolo-ribaudo merged 1 commit intomasterfrom
flow-default

Conversation

@existentialism
Copy link
Copy Markdown
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
Copy Markdown

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
startPos = startPos || this.state.start;
startLoc = startLoc || this.state.startLoc;
let node = id || this.parseIdentifier();
let node = id || this.parseIdentifier(true);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ✔️


const reservedTypes = [
const reservedTypes = new Set([
"_",
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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