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
Partial Application Syntax: Stage 1 #9343
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10370/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this proposal! I think that it could help having better discussions around the pipeline proposal.
cc @rbuckton (proposal champion) could you take a quick look at the tests?
@@ -807,6 +809,9 @@ export default class ExpressionParser extends LValParser { | |||
} | |||
return this.finishNode(node, "Super"); | |||
|
|||
case tt.question: | |||
node = this.startNode(); | |||
this.raise(node.start, "Partial Application syntax is not allowed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these lines needed for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be able to throw a message whenever the user is using an invalid syntax.
I added a boolean allowPlaceholder
to parseExprListItem
and parseCallExpressionArguments
and thought, I could set it to false whenever we do not want to allow the usage of ?
.
Maybe I could throw the error message with another conditional here:
} else if (this.match(tt.question)) {
if (!allowPlaceholder){
const node = this.startNode();
this.raise(node.start, "Partial Application syntax is not allowed");
}
this.expectPlugin("partialApplication");
const node = this.startNode();
this.next();
elt = this.finishNode(node, "Partial");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another solution could be to let Babel throw its own errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default error would be unexpected ?
. it's ok either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the allowPlaceholder
idea from @byara , it can evolve into something more specific and make the users aware of when a partial application is allowed and when it's not, taking examples from https://github.com/tc39/proposal-partial-application#syntax we can tell the user we can point the user on the right direction instead of having him figure out why he's seeing an unexpected ?
error.
packages/babel-parser/ast/spec.md
Outdated
@@ -603,6 +605,16 @@ Any expression node. Since the left-hand side of an assignment may be any expres | |||
|
|||
## Super | |||
|
|||
```js | |||
interface PartialExpression <: Node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could call it ArgumentPlaceholder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I was just trying to follow the Grammar section of the proposal: Grammar. But I also think, ArgumentPlaceholder
shows the purpose better.
@@ -125,6 +125,10 @@ export function OptionalMemberExpression(node: Object) { | |||
} | |||
} | |||
|
|||
export function Partial() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name doesn't match with the name defined in the parser. Also, could you add a generator test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add tests for the generator.
Do you mean in: packages/babel-parser/src/types.js
I see that I made a mistake there. I'll fix it 👍
I named it PartialExpression
, thinking we can take it as an expression but remembered that the proposal does not consider this to be an expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I suppose, I can move this function to src/generators/types.js
? Because we are not considering Partial
(or if we call it ArgumentPlaceholder
) to be an expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good idea (I still prefer ArgumentPlaceholder)
|
👍
All the files inside |
removed the alias
removed my changes from the I'll try to add more test cases for the parser and generator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed my changes from the generated folder.
Sorry if I wasn't clear; I meant that it is correct to have those auto-generated files.
visitor: ["argument"], | ||
fields: { | ||
argument: { | ||
validate: assertNodeType("ArgumentPlaceholder"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean that the AST is something like this:
{
"type": "ArgumentPlaceholder",
"argument": {
"type": "ArgumentPlaceholder",
// ...
}
}
It should only be this:
defineType("ArgumentPlaceholder", {});
@@ -1940,6 +1943,11 @@ export default class ExpressionParser extends LValParser { | |||
if (refTrailingCommaPos && this.match(tt.comma)) { | |||
refTrailingCommaPos.start = this.state.start; | |||
} | |||
} else if (allowPlaceholder && this.match(tt.question)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt about this logic, to have better errors?
} else if (this.match(tt.question)) {
this.expectPlugin("partialApplication");
if (allowPlaceholder) {
this.raise(this.state.start, "Unexpected argument placeholder");
}
// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! It has a clear message and does not confuse the user. The user will understand that they should not use ArgumentPlaceholder
there. I'll apply it in my next push.
P.S. I should apply the negated one right?
if (!allowPlaceholder) {
this.raise(this.state.start, "Unexpected argument placeholder");
}
packages/babel-parser/ast/spec.md
Outdated
@@ -36,7 +37,7 @@ These are the core @babel/parser (babylon) AST node types. | |||
- [DoWhileStatement](#dowhilestatement) | |||
- [ForStatement](#forstatement) | |||
- [ForInStatement](#forinstatement) | |||
- [ForOfStatement](#forofstatement) | |||
- [ForOfStatement](#forofstatement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these indent changes auto-generated? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't change anything about the list, I just added my change in spec and the list was generated for me 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file is autogenerated, maybe it was one of your ide extensions, or formatter?
Can you revert this line and the one above in Changes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Apparently, Markdown All in One
was doing auto formatting.
@@ -10,6 +10,8 @@ import { | |||
classMethodOrDeclareMethodCommon, | |||
} from "./es2015"; | |||
|
|||
defineType("ArgumentPlaceholder", {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also update the CallExpression
definition to accept placeholder arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolo-ribaudo You mean in /packages/babel-types/src/definitions/core.js
I have to add another assert node type like this:
defineType("CallExpression", {
visitor: ["callee", "arguments", "typeParameters", "typeArguments"],
builder: ["callee", "arguments"],
aliases: ["Expression"],
fields: {
callee: {
validate: assertNodeType("Expression"),
},
arguments: {
validate: chain(
assertValueType("array"),
assertEach(
assertNodeType(
"Expression",
"SpreadElement",
"JSXNamespacedName",
"ArgumentPlaceholder",
),
),
),
},
...
{ | ||
"name": "@babel/plugin-syntax-partial-application", | ||
"version": "7.2.0", | ||
"description": "Allow parsing of optional properties", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description needs to be updated
|
Partial application in tagged templates was pretty much rejected in a previous TC39 meeting, so I plan to remove that from the proposal for the time being. |
Do you mean that |
Right now, we can parse |
I think that that part is only disallowing the old proposed |
I knew I got it wrong! Then I guess everything is alright! |
Lets just wait for anather review so that this PR can be merged. Can you rebase it in the meantime? |
Sure! I'll do that 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I only added some comments about some nits in the markdown file.
Thank you
packages/babel-parser/ast/spec.md
Outdated
@@ -862,6 +865,14 @@ interface SpreadElement <: Node { | |||
} | |||
``` | |||
|
|||
## ArgumentPlaceholder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on more level please ###
packages/babel-parser/ast/spec.md
Outdated
@@ -72,6 +73,7 @@ These are the core @babel/parser (babylon) AST node types. | |||
- [LogicalExpression](#logicalexpression) | |||
- [LogicalOperator](#logicaloperator) | |||
- [SpreadElement](#spreadelement) | |||
- [ArgumentPlaceholder](#argumentplaceholder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also be intended one more level.
packages/babel-parser/ast/spec.md
Outdated
@@ -36,7 +37,7 @@ These are the core @babel/parser (babylon) AST node types. | |||
- [DoWhileStatement](#dowhilestatement) | |||
- [ForStatement](#forstatement) | |||
- [ForInStatement](#forinstatement) | |||
- [ForOfStatement](#forofstatement) | |||
- [ForOfStatement](#forofstatement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file is autogenerated, maybe it was one of your ide extensions, or formatter?
Can you revert this line and the one above in Changes
?
packages/babel-parser/ast/spec.md
Outdated
@@ -601,6 +603,7 @@ interface Expression <: Node { } | |||
|
|||
Any expression node. Since the left-hand side of an assignment may be any expression in general, an expression can also be a pattern. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary
@byara Thank you! We will merge this PR when we decide to release v7.4.0. In the meantime, could you squash it and rebase the other PR on top of this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
I'm merging this locally since it has some conflicts |
* add partial application syntax and some tests * remove unnecessary error message and hasPartial function from parseNewArguments * add types for PartialExpression * Update the tests * rename PartialExpression to Partial * move Partial from expressions to types and rename to ArgumentPlaceholder * add tests for ArgumentPlaceholder in babel-generator * rename Partial to ArgumentPlaceholder * update the tests * remove alias from the type and undo changes in generated folder * adds a nice error message * better definition for the type * auto-generated files * update the conditional for allowPlaceholder message and tests * update CallExpression definition to accept ArgumentPlaceholder * change description * clean up * indent ArgumentPlaceholder entry and revert unwanted changes
parseExprListItem
and added a new condition that checks fortt.comma
and anotherboolean
calledallowPlaceholder
. Then I check for the plugin and add the nodePartial
allowPlaceholder
inparseCallExpressionArguments
andparseExprListItem
to able to check if using?
is allowed or not, so I can throw and error.