Function Expression/Declaration AST Facade #1786

MichaReiser · 2021-11-15T13:35:37Z

Summary

Updates the facade for function declarations and function expression to match the grammar defined in #1719.

Function declaration
- Rename from FnDecl to JsFunctionDeclaration
- Rename ParameterList to JsParameterList
- Introduce new JsIdentifierBinding type for id
- Parses ...rest as JS_REST_PARAMETER instead of JS_REST_PATTERN.
- Split out TsReturnType for the : and the return type
Function expression
- Rename from FnExpr to JsFunctionExpression
- Otherwise the same as for declaration
Arrow function expression
- Rename from ArrowExpr to JsArrowFunctionExpression
- Rename ExprOrBlock to JsAnyArrowFunctionBody
- Otherwise the same as for declaration.

This PR cleans up some of the function parsing (extracts helpers rather than using boolean params). This improves error reporting for the func_decl use case if the name is missing.

This PR also removes the support for decorators. The parser had some support for decorators but they haven't been part of the grammar. We should re-add them when we decide to properly support them (and add tests)

Part of #1725

Test Plan

Verified that the coverage isn't changing.

crates/rslint_parser/src/syntax/decl.rs

MichaReiser · 2021-11-15T13:38:33Z

crates/rslint_parser/src/syntax/expr.rs

-				let m = p.start();
-				p.bump_remap(T![async]);
-				let mut complete = function_decl(
-					&mut *p.with_state(ParserState {
-						in_async: true,
-						..p.state.clone()
-					}),
-					m,
-					true,
-				);
-				complete.change_kind(p, FN_EXPR);


We repetitively parsed the async keyword. I decided to move the consumption of the async keyword into function_expression/function_declaration. This fixes multiple issues where the async identifier wasn't correctly re-mapped to the async keyword.

Are there new tests for this new logic? I get we fixed some bugs, but are they reflected inside some test?

MichaReiser · 2021-11-15T13:38:59Z

crates/rslint_parser/src/syntax/function.rs

+// TODO 1725 This is probably not ideal (same with the `declare` keyword). We should
+// use a different AST type for function declarations. For example, a function declaration should
+// never have a body but that would be allowed with this approach. Same for interfaces, interface
+// methods should never have a body
+/// Either parses a typescript declaration body or the function body


I only copied this method which is why I didn't address the declare problem.

cloudflare-workers-and-pages · 2021-11-15T13:54:50Z

Deploying with Cloudflare Pages

Latest commit:	`834e34a`
Status:	✅ Deploy successful!
Preview URL:	https://fd7cfb27.tools-8rn.pages.dev

View logs

xunilrj · 2021-11-15T20:15:53Z

crates/rslint_parser/src/syntax/function.rs

+		p.eat(T![declare]);
+	}
+
+	let in_async = p.at(T![ident]) && p.cur_src() == "async";


Do you think makes sense to have eat_if and bump_remap_if methods?

let in_async = p.bump_remap_if(T![async], p.at(T![ident]) && p.cur_src() == "async"); // let in_async = p.eat_if(T![ident], p.cur_src() == "async");

I personally like the explicit control flow of the if, I find this clearer than too inline condition. However, something like that may come in handy when I work on signalling the "missing" slots but I don't know exactly how the API would look like.

That's why I prefer to defer to delay adding any such methods until I have a better understanding of the requirements.

* Rename `FnDecl` to `JsFunctionDeclaration` * Rename `ParameterList` to `JsParameterList` * Introduce `TsReturnType` to group the `: Type` * Restructure parsing implementation of function decl/expression * Introduce `JsRestParameter`

crates/rslint_parser/src/syntax.rs

ematipico · 2021-11-16T12:54:15Z

crates/rslint_parser/src/syntax/expr.rs

-				let m = p.start();
-				p.bump_remap(T![async]);
-				let mut complete = function_decl(
-					&mut *p.with_state(ParserState {
-						in_async: true,
-						..p.state.clone()
-					}),
-					m,
-					true,
-				);
-				complete.change_kind(p, FN_EXPR);


Are there new tests for this new logic? I get we fixed some bugs, but are they reflected inside some test?

ematipico · 2021-11-16T12:57:22Z

crates/rslint_parser/src/syntax/function.rs

+	block_impl(p, JS_FUNCTION_BODY, None)
+}
+
+// TODO 1725 This is probably not ideal (same with the `declare` keyword). We should


If you add a hash # before the issue number (#1725), it integrates with VSCode/Idea and it will show the issue inside the editor

MichaReiser · 2021-11-16T13:03:09Z

Are there new tests for this new logic? I get we fixed some bugs, but are they reflected inside some test?

Yes, I added new tests

MichaReiser requested review from ematipico and jamiebuilds November 15, 2021 13:35

MichaReiser commented Nov 15, 2021

View reviewed changes

MichaReiser changed the title ~~Function AST Facade~~ Function Expression/Declaration AST Facade Nov 15, 2021

xunilrj reviewed Nov 15, 2021

View reviewed changes

xunilrj approved these changes Nov 15, 2021

View reviewed changes

MichaReiser force-pushed the feature/inline-decl branch from 1476ad6 to 77c6cc8 Compare November 16, 2021 07:19

MichaReiser mentioned this pull request Nov 16, 2021

☂️ AST Façade Improvements #1725

Closed

47 tasks

MichaReiser force-pushed the feature/function-facade branch from c64a86e to c650a2b Compare November 16, 2021 07:23

Base automatically changed from feature/inline-decl to main November 16, 2021 07:24

MichaReiser added 4 commits November 16, 2021 08:25

Function Declaration facade refinements

f0eebb0

* Rename `FnDecl` to `JsFunctionDeclaration` * Rename `ParameterList` to `JsParameterList` * Introduce `TsReturnType` to group the `: Type` * Restructure parsing implementation of function decl/expression * Introduce `JsRestParameter`

Rename FnExpr to JsFunctionExpression and remove decorators

0befa42

Rename ArrowExpr to JsArrowFunctionExpression

4361fca

Update .rast files after merge

3281bb3

MichaReiser force-pushed the feature/function-facade branch from c650a2b to 3281bb3 Compare November 16, 2021 07:25

Address lint error

834e34a

MichaReiser merged commit 30b8e43 into main Nov 16, 2021

MichaReiser deleted the feature/function-facade branch November 16, 2021 07:37

ematipico reviewed Nov 16, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function Expression/Declaration AST Facade #1786

Function Expression/Declaration AST Facade #1786

MichaReiser commented Nov 15, 2021 •

edited

Loading

MichaReiser Nov 15, 2021

ematipico Nov 16, 2021

MichaReiser Nov 15, 2021

cloudflare-workers-and-pages bot commented Nov 15, 2021 •

edited

Loading

xunilrj Nov 15, 2021 •

edited

Loading

MichaReiser Nov 16, 2021

ematipico Nov 16, 2021

ematipico Nov 16, 2021

MichaReiser Nov 16, 2021

MichaReiser commented Nov 16, 2021

Function Expression/Declaration AST Facade #1786

Function Expression/Declaration AST Facade #1786

Conversation

MichaReiser commented Nov 15, 2021 • edited Loading

Summary

Test Plan

MichaReiser Nov 15, 2021

Choose a reason for hiding this comment

ematipico Nov 16, 2021

Choose a reason for hiding this comment

MichaReiser Nov 15, 2021

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Nov 15, 2021 • edited Loading

Deploying with Cloudflare Pages

xunilrj Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

MichaReiser Nov 16, 2021

Choose a reason for hiding this comment

ematipico Nov 16, 2021

Choose a reason for hiding this comment

ematipico Nov 16, 2021

Choose a reason for hiding this comment

MichaReiser Nov 16, 2021

Choose a reason for hiding this comment

MichaReiser commented Nov 16, 2021

MichaReiser commented Nov 15, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 15, 2021 •

edited

Loading

xunilrj Nov 15, 2021 •

edited

Loading