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

Jump to bottom

Throw a syntax error for a declare function with a body #12054

Merged
merged 5 commits into from Sep 18, 2020
Merged

Throw a syntax error for a declare function with a body #12054

merged 5 commits into from Sep 18, 2020

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Sep 11, 2020 •
edited by gitpod-io bot

Sorry for creating PR for an issue that hasn't been triaged. If this isn't needed, please close.

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

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 11, 2020 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/28585/

@JLHwung
Copy link
Contributor

JLHwung commented Sep 11, 2020

May need to check ancestry for .declare, or we track isDeclare in a context variable passed as arguments of the parse functions.

declare namespace n {
  function foo() {}
}

is also invalid.

@JLHwung JLHwung added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Sep 11, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 12, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f2ff434:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@sosukesuzuki
Copy link
Member Author

@JLHwung I've fixed it so can you review?

@JLHwung JLHwung self-requested a review September 15, 2020 16:28
kaicataldo
kaicataldo reviewed Sep 16, 2020
View reviewed changes
packages/babel-parser/src/parser/expression.js Outdated
@@ -2101,6 +2101,8 @@ export default class ExpressionParser extends LValParser {
node: N.BodilessFunctionOrMethodBase,
type: string,
isMethod?: boolean = false,
// eslint-disable-next-line no-unused-vars -- this is used in /plugins/typescript
isDeclare?: boolean = false,
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of modifying the base parser for this, but it does seem like it might be complicated to overload or wrap a bunch of functions in the plugin. Curious what the rest of the team thinks.

@nicolo-ribaudo
Copy link
Member

Maybe we can add a Boolean this.state.isDeclareContext and only set/check it from the TS parser?

@sosukesuzuki
Copy link
Member Author

If this.state.isDeclareContext is added, probably it could also be used to fix #11725?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 17, 2020 •
edited

Yes, I think it would help (I haven't checked the code yet), if we set it before parsing the params.

@sosukesuzuki
Copy link
Member Author

I addressed review comments (3471de1) and add test for declare module (c242660)

JLHwung
JLHwung approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Nice work.

existentialism
existentialism approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

💯

nicolo-ribaudo
nicolo-ribaudo reviewed Sep 18, 2020
View reviewed changes
packages/babel-parser/src/plugins/typescript/index.js Outdated
Comment on lines 1779 to 1785
if (
// $FlowIgnore
node.declare
) {
this.finishNode(node, bodilessType);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Question: Why do we need to special-case this, rather than just parsing the (invalid) function body?

Copy link
Member Author

Choose a reason for hiding this comment

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

What does special-case mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it's the same as what you're pointing out, I noticed a my mistake. I should have used super.parseFunctionBodyAndFinish.

EDIT: I've fixed at f2ff434

Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this whole if, since parseFunctionBodyAndFinish would already be done anyway at the end of the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it. But I think we need this if to use bodilessType as the function node type.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh I missed it 🙃

@nicolo-ribaudo nicolo-ribaudo merged commit ae18f9c into babel:main Sep 18, 2020
@sosukesuzuki sosukesuzuki deleted the fix-12046 branch September 18, 2020 23:29
@fisker fisker mentioned this pull request Oct 15, 2020
Merged
3 tasks
sosukesuzuki added a commit to sosukesuzuki/prettier that referenced this pull request Oct 15, 2020
@sosukesuzuki
Add test for babel/babel#12054
cf0ed63
sosukesuzuki added a commit to prettier/prettier that referenced this pull request Oct 20, 2020
@sosukesuzuki
Update @babel/parser to 7.12 (#9408)
3b7a7ba 
* Install babel/parser 7.12

* Add tests for babel/babel#12161

* Add test for babel/babel#12076

* Add test for babel/babel#12085

* Add test for babel/babel#12108

* Add test for babel/babel#12120

* Add test for babel/babel#12054

* Add test for babel/babel#12061

* Add test babel/babel#12093

* Add test for babel/babel#12065

* Add test for babel/babel#12111

* Add test for babel/babel#12072

* Switch syntax-module-attributes to syntax-import-assertion

* Support "String import/export specifier"

* Remove tests for module-attributes

* Add changelog

* Update to 7.12.3

* Fix by linter

* Fix by spellchecker

* Add tests for module attributes to errors

* Add error test for module string name with import

* Remove TSTypeCastExpression

* Add tests for funny import-assertions

* Update snapshots|

* Add more tests
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaicataldo kaicataldo kaicataldo left review comments

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert a TypeScript declare function with a body into a function.
6 participants
@sosukesuzuki @babel-bot @JLHwung @nicolo-ribaudo @existentialism @kaicataldo