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.

Sign up for GitHub

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

Improve module expression parsing/printing #14980

Merged
merged 8 commits into from Sep 30, 2022
Merged

Improve module expression parsing/printing #14980

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5dba903
fix: several ModuleExpression printing edgecases
JLHwung Sep 28, 2022
101cad5
fix: end module expression's program before }
JLHwung Sep 28, 2022
f31ab0f
fix: expect braceL after module
JLHwung Sep 28, 2022
5215c61
refactor: introduce ensureNoLineTerminator
JLHwung Sep 29, 2022
5c60588
use this.ensureNoLineTerminator
JLHwung Sep 29, 2022
0dc2ccc
update source-map fixtures
JLHwung Sep 30, 2022
1ec194d
address review comments
JLHwung Sep 30, 2022
199f8e5
consume eof when end is eof
JLHwung Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 12 additions & 9 deletions packages/babel-generator/src/generators/expressions.ts
View file
Open in desktop
Expand Up @@ -351,16 +351,19 @@ export function V8IntrinsicIdentifier(

export function ModuleExpression(this: Printer, node: t.ModuleExpression) {
this.word("module");
this.space();
// ensure no line terminator between `module` and `{`
this.ensureNoLineTerminator(() => {
this.printInnerComments(node);
this.space();
});
this.token("{");
if (node.body.body.length === 0) {
this.token("}");
} else {
this.indent();
const { body } = node;
if (body.body.length || body.directives.length) {
this.newline();
this.printSequence(node.body.body, node, { indent: true });

this.sourceWithOffset("end", node.loc, 0, -1);

this.rightBrace();
}
this.print(body, node);
this.dedent();
this.sourceWithOffset("end", node.loc, 0, -1);
this.rightBrace();
}
16 changes: 13 additions & 3 deletions packages/babel-generator/src/printer.ts
View file
Open in desktop
Expand Up @@ -495,6 +495,16 @@ class Printer {
return this._indentRepeat * this._indent;
}

ensureNoLineTerminator(fn: () => void) {
const { _noLineTerminator } = this;
this._noLineTerminator = true;
try {
fn();
} finally {
this._noLineTerminator = _noLineTerminator;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems that currently our generator is not allowed to throw exceptions, I personally prefer not to have this try.


printTerminatorless(node: t.Node, parent: t.Node, isLabel: boolean) {
/**
* Set some state that will be modified if a newline has been inserted before any
Expand All @@ -512,9 +522,9 @@ class Printer {
* `undefined` will be returned and not `foo` due to the terminator.
*/
if (isLabel) {
this._noLineTerminator = true;
this.print(node, parent);
this._noLineTerminator = false;
this.ensureNoLineTerminator(() => {
this.print(node, parent);
});
} else {
const terminatorState = {
printed: false,
Expand Down
1 change: 1 addition & 0 deletions packages/babel-generator/test/fixtures/comments/module-expression-inner-comment/input.js
View file
Open in desktop
@@ -0,0 +1 @@
module /* 1 */ { /* 2 */ }
3 changes: 3 additions & 0 deletions packages/babel-generator/test/fixtures/comments/module-expression-inner-comment/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["moduleBlocks"]
}
3 changes: 3 additions & 0 deletions packages/babel-generator/test/fixtures/comments/module-expression-inner-comment/output.js
View file
Open in desktop
@@ -0,0 +1,3 @@
module /* 1 */ {
/* 2 */
};
2 changes: 1 addition & 1 deletion packages/babel-generator/test/fixtures/sourcemaps/ModuleExpression/source-map.json
View file
Open in desktop
Expand Up @@ -11,5 +11,5 @@
"sourcesContent": [
"const m = module { export const foo = \"foo\" };\nmodule {\n foo;\n bar;\n};\nfoo(module {});"
],
"mappings": "AAAA,MAAMA,CAAC,GAAG;EAAS,OAAO,MAAMC,GAAG,GAAG,KAAK;AAAC,CAAC;AAC7C;EACEA,GAAG;EACHC,GAAG;AACL,CAAC;AACDD,GAAG,CAAC,SAAS,CAAC"
"mappings": "AAAA,MAAMA,CAAC,GAAG;EAAS,OAAO,MAAMC,GAAG,GAAG,KAAK;AAAC,CAAC;AAC7C;EACEA,GAAG;EACHC,GAAG;AACL,CAAC;AACDD,GAAG,CAAC,QAAQ,CAAC,CAAC"
Copy link
Contributor Author

@JLHwung JLHwung Sep 30, 2022 •
edited

Choose a reason for hiding this comment

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

The source map is updated because we now generate two sections module{ and } and map them respectively for the empty module expression module{}. Before this PR we map module{} as module{}.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could optimize this in the future (directly or in one of @jridgewell's packages) by merging source map segments that are adjacent both in the input and in the output.

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be technically difficult, but I'm not sure that's better. In the last PR I marked the sourcemap for every ";".

Copy link
Member

Choose a reason for hiding this comment

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

This has to be done in Babel, because the sourcemap packages will not know whether this marking is important or not. Eg, function foo() { return bar; }, we should generate a marking for bar and } (the } is required for Chrome's debug stepping). I can't make the source map package ignore this module {} case without also ignoring that case.

}
4 changes: 4 additions & 0 deletions packages/babel-generator/test/fixtures/types/ModuleExpression-directives/input.js
View file
Open in desktop
@@ -0,0 +1,4 @@
module {
"hide source";
secret;
}
4 changes: 4 additions & 0 deletions packages/babel-generator/test/fixtures/types/ModuleExpression-directives/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"plugins": ["moduleBlocks"],
"sourceType": "module"
}
5 changes: 5 additions & 0 deletions packages/babel-generator/test/fixtures/types/ModuleExpression-directives/output.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module {
"hide source";

secret;
};
2 changes: 1 addition & 1 deletion packages/babel-parser/src/parser/expression.ts
View file
Open in desktop
Expand Up @@ -3233,7 +3233,7 @@ export default abstract class ExpressionParser extends LValParser {
this.expectPlugin("moduleBlocks");
const node = this.startNode<N.ModuleExpression>();
this.next(); // eat "module"
this.eat(tt.braceL);
this.expect(tt.braceL);

const revertScopes = this.initializeScopes(/** inModule */ true);
this.enterInitialScopes();
Expand Down
5 changes: 3 additions & 2 deletions packages/babel-parser/src/parser/statement.ts
View file
Open in desktop
Expand Up @@ -222,6 +222,7 @@ export default abstract class StatementParser extends ExpressionParser {
this.raise(Errors.ModuleExportUndefined, { at, localName });
}
}
this.eat(tt.eof); // stop before } when parsing a module expression
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to eat it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tt.eof is eaten when we are parsing top level program. If it is not eaten, the tt.eof token won't be pushed to this.tokens.

Copy link
Member

Choose a reason for hiding this comment

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

Can we move it as tt.expect in parseTopLevel, to assert that we actually consumed all the tokens?

Copy link
Contributor Author

@JLHwung JLHwung Sep 30, 2022 •
edited

Choose a reason for hiding this comment

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

If we eat tt.eof in parseTopLevel, the Program will end earlier than it should be and as a result its trailing comments won't append. I revises it a bit and now it only consume when end is tt.eof, which must be invoked by parseTopLevel.

return this.finishNode(program, "Program");
}

Expand Down Expand Up @@ -1123,6 +1124,7 @@ export default abstract class StatementParser extends ExpressionParser {
tt.braceR,
afterBlockParse,
);
this.next(); // eat tt.braceR
if (createNewLexicalScope) {
this.scope.exit();
}
Expand Down Expand Up @@ -1204,8 +1206,6 @@ export default abstract class StatementParser extends ExpressionParser {
if (!oldStrict) {
this.setStrict(false);
}

this.next();
}

// Parse a regular `for` loop. The disambiguation code in
Expand Down Expand Up @@ -1881,6 +1881,7 @@ export default abstract class StatementParser extends ExpressionParser {
this.prodParam.enter(PARAM);
const body: N.Node[] = (member.body = []);
this.parseBlockOrModuleBlockBody(body, undefined, false, tt.braceR);
this.next(); // eat tt.braceR
this.prodParam.exit();
this.scope.exit();
this.state.labels = oldLabels;
Expand Down
1 change: 1 addition & 0 deletions packages/babel-parser/src/plugins/typescript/index.ts
View file
Open in desktop
Expand Up @@ -1869,6 +1869,7 @@ export default (superClass: ClassWithMixin<typeof Parser, IJSXParserMixin>) =>
/* topLevel */ true,
/* end */ tt.braceR,
);
this.next(); // eat tt.braceR
this.scope.exit();
return this.finishNode(node, "TSModuleBlock");
}
Expand Down
2 changes: 1 addition & 1 deletion ...arser/test/fixtures/experimental/module-blocks/invalid-class-in-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -53,7 +53,7 @@
"start":23,"end":66,"loc":{"start":{"line":3,"column":4,"index":23},"end":{"line":5,"column":5,"index":66}},
"body": {
"type": "Program",
"start":38,"end":66,"loc":{"start":{"line":4,"column":6,"index":38},"end":{"line":5,"column":5,"index":66}},
"start":38,"end":60,"loc":{"start":{"line":4,"column":6,"index":38},"end":{"line":4,"column":28,"index":60}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...test/fixtures/experimental/module-blocks/invalid-duplicated-exports-in-blocks/output.json
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@
"start":10,"end":66,"loc":{"start":{"line":1,"column":10,"index":10},"end":{"line":4,"column":1,"index":66}},
"body": {
"type": "Program",
"start":21,"end":66,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":4,"column":1,"index":66}},
"start":21,"end":64,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":3,"column":17,"index":64}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
1 change: 1 addition & 0 deletions ...babel-parser/test/fixtures/experimental/module-blocks/invalid-module-bracketBarL/input.js
View file
Open in desktop
@@ -0,0 +1 @@
module{|foo:42|}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR we parses this input as if it were

// note the extra required `{` after `module`
module {
  {| foo: 42 |}
}

This is now fixed by expecting { after module.

12 changes: 12 additions & 0 deletions ...l-parser/test/fixtures/experimental/module-blocks/invalid-module-bracketBarL/options.json
View file
Open in desktop
@@ -0,0 +1,12 @@
{
"plugins": [
"moduleBlocks",
[
"recordAndTuple",
{
"syntaxType": "bar"
}
]
],
"throws": "Unexpected token, expected \"{\" (1:6)"
}
2 changes: 1 addition & 1 deletion ...xtures/experimental/module-blocks/invalid-undefined-export-in-module-blocks01/output.json
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@
"start":0,"end":27,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":3,"column":1,"index":27}},
"body": {
"type": "Program",
"start":11,"end":27,"loc":{"start":{"line":2,"column":2,"index":11},"end":{"line":3,"column":1,"index":27}},
"start":11,"end":25,"loc":{"start":{"line":2,"column":2,"index":11},"end":{"line":2,"column":16,"index":25}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...xtures/experimental/module-blocks/invalid-undefined-export-in-module-blocks02/output.json
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@
"start":9,"end":37,"loc":{"start":{"line":2,"column":0,"index":9},"end":{"line":4,"column":1,"index":37}},
"body": {
"type": "Program",
"start":20,"end":37,"loc":{"start":{"line":3,"column":2,"index":20},"end":{"line":4,"column":1,"index":37}},
"start":20,"end":35,"loc":{"start":{"line":3,"column":2,"index":20},"end":{"line":3,"column":17,"index":35}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...er/test/fixtures/experimental/module-blocks/module-block-in-member-expression/output.json
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@
"start":0,"end":9,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":1,"column":9,"index":9}},
"body": {
"type": "Program",
"start":8,"end":9,"loc":{"start":{"line":1,"column":8,"index":8},"end":{"line":1,"column":9,"index":9}},
"start":8,"end":8,"loc":{"start":{"line":1,"column":8,"index":8},"end":{"line":1,"column":8,"index":8}},
"sourceType": "module",
"interpreter": null,
"body": [],
Expand Down
2 changes: 1 addition & 1 deletion ...t/fixtures/experimental/module-blocks/valid-assign-module-blocks-to-variables/output.json
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@
"start":10,"end":48,"loc":{"start":{"line":1,"column":10,"index":10},"end":{"line":3,"column":1,"index":48}},
"body": {
"type": "Program",
"start":21,"end":48,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":3,"column":1,"index":48}},
"start":21,"end":46,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":2,"column":27,"index":46}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...-parser/test/fixtures/experimental/module-blocks/valid-class-in-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@
"start":23,"end":96,"loc":{"start":{"line":3,"column":4,"index":23},"end":{"line":8,"column":5,"index":96}},
"body": {
"type": "Program",
"start":38,"end":96,"loc":{"start":{"line":4,"column":6,"index":38},"end":{"line":8,"column":5,"index":96}},
"start":38,"end":90,"loc":{"start":{"line":4,"column":6,"index":38},"end":{"line":7,"column":7,"index":90}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
4 changes: 2 additions & 2 deletions ...res/experimental/module-blocks/valid-duplicated-exports-between-other-modules/output.json
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@
"start":37,"end":72,"loc":{"start":{"line":2,"column":11,"index":37},"end":{"line":2,"column":46,"index":72}},
"body": {
"type": "Program",
"start":46,"end":72,"loc":{"start":{"line":2,"column":20,"index":46},"end":{"line":2,"column":46,"index":72}},
"start":46,"end":70,"loc":{"start":{"line":2,"column":20,"index":46},"end":{"line":2,"column":44,"index":70}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down Expand Up @@ -115,7 +115,7 @@
"start":85,"end":120,"loc":{"start":{"line":3,"column":11,"index":85},"end":{"line":3,"column":46,"index":120}},
"body": {
"type": "Program",
"start":94,"end":120,"loc":{"start":{"line":3,"column":20,"index":94},"end":{"line":3,"column":46,"index":120}},
"start":94,"end":118,"loc":{"start":{"line":3,"column":20,"index":94},"end":{"line":3,"column":44,"index":118}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...xtures/experimental/module-blocks/valid-duplicated-variables-in-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@
"start":29,"end":60,"loc":{"start":{"line":2,"column":10,"index":29},"end":{"line":4,"column":1,"index":60}},
"body": {
"type": "Program",
"start":40,"end":60,"loc":{"start":{"line":3,"column":2,"index":40},"end":{"line":4,"column":1,"index":60}},
"start":40,"end":58,"loc":{"start":{"line":3,"column":2,"index":40},"end":{"line":3,"column":20,"index":58}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...bel-parser/test/fixtures/experimental/module-blocks/valid-empty-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@
"start":0,"end":9,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":1,"column":9,"index":9}},
"body": {
"type": "Program",
"start":8,"end":9,"loc":{"start":{"line":1,"column":8,"index":8},"end":{"line":1,"column":9,"index":9}},
"start":8,"end":8,"loc":{"start":{"line":1,"column":8,"index":8},"end":{"line":1,"column":8,"index":8}},
"sourceType": "module",
"interpreter": null,
"body": [],
Expand Down
2 changes: 1 addition & 1 deletion ...el-parser/test/fixtures/experimental/module-blocks/valid-import-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@
"start":10,"end":48,"loc":{"start":{"line":1,"column":10,"index":10},"end":{"line":3,"column":1,"index":48}},
"body": {
"type": "Program",
"start":21,"end":48,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":3,"column":1,"index":48}},
"start":21,"end":46,"loc":{"start":{"line":2,"column":2,"index":21},"end":{"line":2,"column":27,"index":46}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...ser/test/fixtures/experimental/module-blocks/valid-module-blocks-as-arguments/output.json
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@
"start":26,"end":66,"loc":{"start":{"line":1,"column":26,"index":26},"end":{"line":3,"column":1,"index":66}},
"body": {
"type": "Program",
"start":39,"end":66,"loc":{"start":{"line":2,"column":4,"index":39},"end":{"line":3,"column":1,"index":66}},
"start":39,"end":64,"loc":{"start":{"line":2,"column":4,"index":39},"end":{"line":2,"column":29,"index":64}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
4 changes: 2 additions & 2 deletions ...el-parser/test/fixtures/experimental/module-blocks/valid-nested-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@
"start":8,"end":62,"loc":{"start":{"line":1,"column":8,"index":8},"end":{"line":5,"column":1,"index":62}},
"body": {
"type": "Program",
"start":19,"end":62,"loc":{"start":{"line":2,"column":2,"index":19},"end":{"line":5,"column":1,"index":62}},
"start":19,"end":60,"loc":{"start":{"line":2,"column":2,"index":19},"end":{"line":4,"column":4,"index":60}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand All @@ -36,7 +36,7 @@
"start":19,"end":59,"loc":{"start":{"line":2,"column":2,"index":19},"end":{"line":4,"column":3,"index":59}},
"body": {
"type": "Program",
"start":32,"end":59,"loc":{"start":{"line":3,"column":4,"index":32},"end":{"line":4,"column":3,"index":59}},
"start":32,"end":55,"loc":{"start":{"line":3,"column":4,"index":32},"end":{"line":3,"column":27,"index":55}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...arser/test/fixtures/experimental/module-blocks/valid-singleline-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@
"start":10,"end":45,"loc":{"start":{"line":1,"column":10,"index":10},"end":{"line":1,"column":45,"index":45}},
"body": {
"type": "Program",
"start":19,"end":45,"loc":{"start":{"line":1,"column":19,"index":19},"end":{"line":1,"column":45,"index":45}},
"start":19,"end":43,"loc":{"start":{"line":1,"column":19,"index":19},"end":{"line":1,"column":43,"index":43}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down
2 changes: 1 addition & 1 deletion ...st/fixtures/experimental/module-blocks/valid-top-level-await-in-module-blocks/output.json
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@
"start":0,"end":18,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":1,"column":18,"index":18}},
"body": {
"type": "Program",
"start":9,"end":18,"loc":{"start":{"line":1,"column":9,"index":9},"end":{"line":1,"column":18,"index":18}},
"start":9,"end":16,"loc":{"start":{"line":1,"column":9,"index":9},"end":{"line":1,"column":16,"index":16}},
"sourceType": "module",
"interpreter": null,
"body": [
Expand Down