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

Improve generation of comments without location #15037

Merged
merged 9 commits into from Oct 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 45 additions & 9 deletions packages/babel-generator/src/printer.ts
Expand Up @@ -2,6 +2,12 @@ import Buffer from "./buffer";
import type { Loc } from "./buffer";
import * as n from "./node";
import type * as t from "@babel/types";
import {
isFunction,
isStatement,
isClassBody,
isTSInterfaceBody,
} from "@babel/types";
import type {
RecordAndTuplePluginOptions,
PipelineOperatorPluginOptions,
Expand Down Expand Up @@ -626,18 +632,18 @@ class Printer {

this._lastCommentLine = 0;

this._printLeadingComments(node);
this._printLeadingComments(node, parent);

const loc = nodeType === "Program" || nodeType === "File" ? null : node.loc;

this.exactSource(loc, printMethod.bind(this, node, parent));

if (noLineTerminator && !this._noLineTerminator) {
this._noLineTerminator = true;
this._printTrailingComments(node);
this._printTrailingComments(node, parent);
this._noLineTerminator = false;
} else {
this._printTrailingComments(node, trailingCommentsLineOffset);
this._printTrailingComments(node, parent, trailingCommentsLineOffset);
}

if (shouldPrintParens) this.token(")");
Expand Down Expand Up @@ -768,16 +774,22 @@ class Printer {
this.print(node, parent);
}

_printTrailingComments(node: t.Node, lineOffset?: number) {
_printTrailingComments(node: t.Node, parent?: t.Node, lineOffset?: number) {
const comments = this._getComments(false, node);
if (!comments?.length) return;
this._printComments(COMMENT_TYPE.TRAILING, comments, node, lineOffset);
this._printComments(
COMMENT_TYPE.TRAILING,
comments,
node,
parent,
lineOffset,
);
}

_printLeadingComments(node: t.Node) {
_printLeadingComments(node: t.Node, parent: t.Node) {
const comments = this._getComments(true, node);
if (!comments?.length) return;
this._printComments(COMMENT_TYPE.LEADING, comments, node);
this._printComments(COMMENT_TYPE.LEADING, comments, node, parent);
}

printInnerComments(node: t.Node, indent = true) {
Expand Down Expand Up @@ -936,6 +948,7 @@ class Printer {
type: COMMENT_TYPE,
comments: readonly t.Comment[],
node: t.Node,
parent?: t.Node,
lineOffset: number = 0,
) {
{
Expand Down Expand Up @@ -1004,11 +1017,34 @@ class Printer {
hasLoc = false;

if (len === 1) {
this._printComment(comment, COMMENT_SKIP_NEWLINE.SKIP_ALL);
const singleLine = comment.loc
? comment.loc.start.line === comment.loc.end.line
: !comment.value.includes("\n");

const shouldSkipNewline =
singleLine &&
!isStatement(node) &&
!isClassBody(parent) &&
!isTSInterfaceBody(parent);

if (type === COMMENT_TYPE.LEADING) {
this._printComment(
comment,
(shouldSkipNewline && node.type !== "ObjectExpression") ||
(singleLine && isFunction(parent, { body: node }))
? COMMENT_SKIP_NEWLINE.SKIP_ALL
: COMMENT_SKIP_NEWLINE.DEFAULT,
);
} else if (shouldSkipNewline && type === COMMENT_TYPE.TRAILING) {
this._printComment(comment, COMMENT_SKIP_NEWLINE.SKIP_ALL);
} else {
this._printComment(comment, COMMENT_SKIP_NEWLINE.DEFAULT);
}
} else if (
type === COMMENT_TYPE.INNER &&
!(node.type === "ObjectExpression" && node.properties.length > 1) &&
node.type !== "ClassBody"
node.type !== "ClassBody" &&
node.type !== "TSInterfaceBody"
) {
// class X {
// /*:: a: number*/
Expand Down
212 changes: 212 additions & 0 deletions packages/babel-generator/test/index.js
Expand Up @@ -483,6 +483,218 @@ describe("generation", function () {

expect(generate(ast).code).toBe("/*#__PURE__*/\n/*#__PURE__*/");
});

it("comments without loc", () => {
const ast = parse(
`
import {
Attribute,
AttributeSDKType
}
from "../../base/v1beta1/attribute";
import {
Rpc
}
from "../../../helpers";
import * as _m0 from "protobufjs/minimal";
import {
MsgSignProviderAttributes,
MsgSignProviderAttributesSDKType,
MsgSignProviderAttributesResponse,
MsgSignProviderAttributesResponseSDKType,
MsgDeleteProviderAttributes,
MsgDeleteProviderAttributesSDKType,
MsgDeleteProviderAttributesResponse,
MsgDeleteProviderAttributesResponseSDKType
}
from "./audit";
/** Msg defines the provider Msg service */
export interface Msg {
/** SignProviderAttributes defines a method that signs provider attributes */
signProviderAttributes(request: MsgSignProviderAttributes): Promise < MsgSignProviderAttributesResponse > ;
/** DeleteProviderAttributes defines a method that deletes provider attributes */
deleteProviderAttributes(request: MsgDeleteProviderAttributes): Promise < MsgDeleteProviderAttributesResponse > ;
}
export class MsgClientImpl implements Msg {
private readonly rpc: Rpc;
constructor(rpc: Rpc) {
this.rpc = rpc;
}
/* SignProviderAttributes defines a method that signs provider attributes */
signProviderAttributes = async(request: MsgSignProviderAttributes): Promise < MsgSignProviderAttributesResponse > => {
const data = MsgSignProviderAttributes.encode(request).finish();
const promise = this.rpc.request("akash.audit.v1beta1.Msg", "SignProviderAttributes", data);
return promise.then(data => MsgSignProviderAttributesResponse.decode(new _m0.Reader(data)));
};
/* DeleteProviderAttributes defines a method that deletes provider attributes */
deleteProviderAttributes = async(request: MsgDeleteProviderAttributes): Promise < MsgDeleteProviderAttributesResponse > => {
const data = MsgDeleteProviderAttributes.encode(request).finish();
const promise = this.rpc.request("akash.audit.v1beta1.Msg", "DeleteProviderAttributes", data);
return promise.then(data => MsgDeleteProviderAttributesResponse.decode(new _m0.Reader(data)));
};
}
`,
{ sourceType: "module", plugins: ["typescript"] },
);

for (const comment of ast.comments) {
comment.loc = undefined;
}

expect(generate(ast).code).toMatchInlineSnapshot(`
"import { Attribute, AttributeSDKType } from \\"../../base/v1beta1/attribute\\";
import { Rpc } from \\"../../../helpers\\";
import * as _m0 from \\"protobufjs/minimal\\";
import { MsgSignProviderAttributes, MsgSignProviderAttributesSDKType, MsgSignProviderAttributesResponse, MsgSignProviderAttributesResponseSDKType, MsgDeleteProviderAttributes, MsgDeleteProviderAttributesSDKType, MsgDeleteProviderAttributesResponse, MsgDeleteProviderAttributesResponseSDKType } from \\"./audit\\";
/** Msg defines the provider Msg service */
export interface Msg {
/** SignProviderAttributes defines a method that signs provider attributes */
signProviderAttributes(request: MsgSignProviderAttributes): Promise<MsgSignProviderAttributesResponse>;
/** DeleteProviderAttributes defines a method that deletes provider attributes */
deleteProviderAttributes(request: MsgDeleteProviderAttributes): Promise<MsgDeleteProviderAttributesResponse>;
}
export class MsgClientImpl implements Msg {
private readonly rpc: Rpc;
constructor(rpc: Rpc) {
this.rpc = rpc;
}
/* SignProviderAttributes defines a method that signs provider attributes */
signProviderAttributes = async (request: MsgSignProviderAttributes): Promise<MsgSignProviderAttributesResponse> => {
const data = MsgSignProviderAttributes.encode(request).finish();
const promise = this.rpc.request(\\"akash.audit.v1beta1.Msg\\", \\"SignProviderAttributes\\", data);
return promise.then(data => MsgSignProviderAttributesResponse.decode(new _m0.Reader(data)));
};
/* DeleteProviderAttributes defines a method that deletes provider attributes */
deleteProviderAttributes = async (request: MsgDeleteProviderAttributes): Promise<MsgDeleteProviderAttributesResponse> => {
const data = MsgDeleteProviderAttributes.encode(request).finish();
const promise = this.rpc.request(\\"akash.audit.v1beta1.Msg\\", \\"DeleteProviderAttributes\\", data);
return promise.then(data => MsgDeleteProviderAttributesResponse.decode(new _m0.Reader(data)));
};
}"
`);
});

it("comments without loc2", () => {
const ast = parse(
`
(function (_templateFactory) {
"use strict";

const template = (0, _templateFactory.createTemplateFactory)(
/*{{somevalue}}*/
{
"id": null,
"block": "[[[1,[34,0]]],[],false,[\\"somevalue\\"]]",
"moduleName": "(unknown template module)",
"isStrictMode": false
});
});

const template = (0, _templateFactory.createTemplateFactory)(
/*
{{somevalue}}
*/
{
"id": null,
"block": "[[[1,[34,0]]],[],false,[\\"somevalue\\"]]",
"moduleName": "(unknown template module)",
"isStrictMode": false
});
`,
{ sourceType: "module" },
);

for (const comment of ast.comments) {
comment.loc = undefined;
}

expect(generate(ast).code).toMatchInlineSnapshot(`
"(function (_templateFactory) {
\\"use strict\\";

const template = (0, _templateFactory.createTemplateFactory)(
/*{{somevalue}}*/
{
\\"id\\": null,
\\"block\\": \\"[[[1,[34,0]]],[],false,[\\\\\\"somevalue\\\\\\"]]\\",
\\"moduleName\\": \\"(unknown template module)\\",
\\"isStrictMode\\": false
});
});
const template = (0, _templateFactory.createTemplateFactory)(
/*
{{somevalue}}
*/
{
Comment on lines +624 to +628
Copy link
Member Author

Choose a reason for hiding this comment

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

There's nothing we can do about this at the moment, except to make comment carry more information.(not in loc)

\\"id\\": null,
\\"block\\": \\"[[[1,[34,0]]],[],false,[\\\\\\"somevalue\\\\\\"]]\\",
\\"moduleName\\": \\"(unknown template module)\\",
\\"isStrictMode\\": false
});"
`);
});

it("comments without node.loc", () => {
const ast = parse(
`
(function (_templateFactory) {
"use strict";

const template = (0, _templateFactory.createTemplateFactory)(
/*{{somevalue}}*/
{
"id": null,
"block": "[[[1,[34,0]]],[],false,[\\"somevalue\\"]]",
"moduleName": "(unknown template module)",
"isStrictMode": false
});
});

const template = (0, _templateFactory.createTemplateFactory)(
/*
{{somevalue}}
*/
{
"id": null,
"block": "[[[1,[34,0]]],[],false,[\\"somevalue\\"]]",
"moduleName": "(unknown template module)",
"isStrictMode": false
});
`,
{ sourceType: "module" },
);

const ast2 = t.cloneNode(ast, true, true);

for (let i = 0; i < ast.comments.length; i++) {
ast2.comments[i].loc = ast.comments[i].loc;
}

expect(generate(ast2).code).toMatchInlineSnapshot(`
"(function (_templateFactory) {
\\"use strict\\";

const template = (0, _templateFactory.createTemplateFactory)(
/*{{somevalue}}*/
{
\\"id\\": null,
\\"block\\": \\"[[[1,[34,0]]],[],false,[\\\\\\"somevalue\\\\\\"]]\\",
\\"moduleName\\": \\"(unknown template module)\\",
\\"isStrictMode\\": false
});
});
const template = (0, _templateFactory.createTemplateFactory)(
/*
{{somevalue}}
*/
{
\\"id\\": null,
\\"block\\": \\"[[[1,[34,0]]],[],false,[\\\\\\"somevalue\\\\\\"]]\\",
\\"moduleName\\": \\"(unknown template module)\\",
\\"isStrictMode\\": false
});"
`);
});
});

describe("programmatic generation", function () {
Expand Down
@@ -1,4 +1,5 @@
class X {
foo = 2;
bar /*: number*/ = 3; /*:: baz: ?string*/
bar /*: number*/ = 3;
/*:: baz: ?string*/
}
@@ -1,2 +1,3 @@
class C /*:: <+T, -U>*/{}
function f /*:: <+T, -U>*/() {} /*:: type T<+T, -U> = {};*/
function f /*:: <+T, -U>*/() {}
/*:: type T<+T, -U> = {};*/
@@ -1,2 +1,4 @@
/*:: export type { A } from './types';*/import lib from 'library';
export { foo } from 'foo'; /*:: export type { B, C } from './types';*/
/*:: export type { A } from './types';*/
import lib from 'library';
export { foo } from 'foo';
/*:: export type { B, C } from './types';*/
@@ -1,4 +1,5 @@
import A, { B, E } from './types'; /*:: import { type C, type D } from './types';*/
import A, { B, E } from './types';
/*:: import { type C, type D } from './types';*/
import F, { H } from './types';
/*:: import { typeof G, type I } from './types';*/
/*:: import { type J } from './types';*/
Expand Down
@@ -1,3 +1,4 @@
/*:: import type A, { B, C } from './types';*/import lib from 'library';
/*:: import type A, { B, C } from './types';*/
import lib from 'library';
/*:: import type D from './types';*/
/*:: import typeof E, { F, G } from './types';*/