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

Add support for keeping single line comments from typescript source code #64

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for keeping single line comments from typescript source code #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b061a11
Add support for keeping single line comments from typescript source code
adamuso Dec 20, 2021
c944d62
Refactor names and add comments
adamuso Dec 20, 2021
0d693b3
Add options to parseNode instead of warpping it in another function f…
adamuso Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion parse_node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import { parseTemplateExpression } from "./parse_node/parse_template_expression"
import { parseNoSubstitutionTemplateLiteral } from "./parse_node/parse_no_substitution_template_expression"
import { AssetSourceFile } from "./project/assets/asset_source_file"
import { LibraryFunctionName } from "./parse_node/library_functions"
import { parseComments } from "./parse_node/parse_node_with_comments"

export type ParseState = {
isConstructor: boolean
Expand All @@ -82,6 +83,7 @@ export type ParseState = {
usages: Map<ts.Identifier, utils.VariableInfo>
sourceFile: ts.SourceFile
sourceFileAsset: AssetSourceFile
commentsStack?: ts.CommentRange[]
}

export enum ExtraLineType {
Expand Down Expand Up @@ -247,8 +249,17 @@ export function combine(args: {

export const parseNode = (
genericNode: ts.Node,
props: ParseState
props: ParseState,
options?: { ignoreComments?: boolean }
): ParseNodeType => {
// TsGdProjectClass.Paths is undefined when running tests
if (
!(TsGdProjectClass.Paths ?? {}).removeComments &&
!options?.ignoreComments
) {
return parseComments(genericNode, props)
}

switch (genericNode.kind) {
case SyntaxKind.SourceFile:
return parseSourceFile(genericNode as ts.SourceFile, props)
Expand Down
188 changes: 188 additions & 0 deletions parse_node/parse_node_with_comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
import ts from "typescript"
import { parseNode, ParseNodeType, ParseState } from "../parse_node"
import { Test } from "../tests/test"

const countNewlinesBeforePosition = (text: string, pos: number) => {
let count = 0

for (let i = pos - 1; i >= 0; i--) {
const char = text.charAt(i)
if (char === "\t" || char === " ") {
continue
}

if (char === "\n") {
count++
continue
}

break
}

return count
}

export const parseComments = (
node: ts.Node,
props: ParseState
): ParseNodeType => {
if (ts.isSourceFile(node)) {
return parseNode(node, props, { ignoreComments: true })
}

// gather up all leading and trailing comments from a node
let leadingComments =
ts
.getLeadingCommentRanges(
node.getSourceFile().getFullText(),
node.getFullStart()
)
?.filter((v) => v.kind === ts.SyntaxKind.SingleLineCommentTrivia) ?? []

let trailingComments =
ts
.getTrailingCommentRanges(
node.getSourceFile().getFullText(),
node.getFullStart() + node.getFullWidth()
)
?.filter((v) => v.kind === ts.SyntaxKind.SingleLineCommentTrivia) ?? []

let lengthBeforeParsingThisNode = props.commentsStack?.length ?? 0

adamuso marked this conversation as resolved.
Show resolved Hide resolved
if (leadingComments.length > 0 || trailingComments.length > 0) {
if (!props.commentsStack) {
props.commentsStack = []
}

/* all nodes in the same line report the same comments
we need to take only first of the node into account and
ignore the rest

example code returning the same commment multiple times:
// some comment
myFunc(1 + 2, someVar)
*/
leadingComments = leadingComments.filter(
(v) =>
!props.commentsStack!.find((c) => c.pos === v.pos && c.end === v.end)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe you that we need to dedupe them, but why? Does TS give us back the same comment multiple times? When and why?

It might help to leave a comment about this.

Copy link
Contributor Author

@adamuso adamuso Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that multiple nodes can give the same comment when they are in the same line. For example:

// some comment
myFunc(1 + 2, someVar)

This code generates AST like: CallExpression (params: BinaryExpression (left: NumberLiteral (1), right: NumberLiteral (2)), Identifier (someVar)). All nodes in this AST will report // some comment that is why we need to handle only first node (CallExpression) that report this comments and ignore the rest otherwise we would generate # some comment in .gd four times. I will add some comments.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I understand. Bit of a wild guess here, but maybe comments are mapped 1:1 with statements, and then copied into child nodes? Could you try just checking if node is a statement type and ONLY if it is appending the comments?

e.g. like this:
https://github.com/johnfn/ts2gd/blob/main/parse_node.ts#L169

Copy link
Owner

Choose a reason for hiding this comment

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

Because it seems if we could solve this then a lot of this code would be simplified - we wouldn't even need to track comments at all in the ParseState!

Copy link
Contributor Author

@adamuso adamuso Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I tried limiting it only to statements but then cases like one below are removed. I don't think comments are mapped 1:1 with statements because when single statement is divided to multiple lines and there are comments inside those lines then comments are bound to expression nodes and not to the statement.

someCall(
  // parameter comment
  "abcd",
  // next comment
 123
)
someCall("abcd", 123)

I also tested your example from other comment and indeed it generates invalid GDScript code, so I think we somehow handle these weird cases or we do as you said: limit comment generation only for statements and this way we won't need too worry about such cases.

// example from other comment
let foo = { bar: 1 }
console.log(foo. 
  // Hello
bar)

I don't really know which way to move forward so I'll wait for your opinion. Also there is not so good another way, just leave it as it is and if someone creates comment that generates invalid GDScript then Godot will actually tell him that something is wrong anyway.

)
trailingComments = trailingComments.filter(
(v) =>
!props.commentsStack!.find((c) => c.pos === v.pos && c.end === v.end)
)

props.commentsStack.push(...leadingComments, ...trailingComments)
}

const result = parseNode(node, props, { ignoreComments: true })

if (leadingComments.length > 0 || trailingComments.length > 0) {
const fullText = node.getSourceFile().getFullText()

if (leadingComments.length > 0) {
const leadingCommentsText = leadingComments.map(
(v) => "#" + fullText.slice(v.pos + 2, v.end)
)
const addFirstNewLineToLeadingComments =
countNewlinesBeforePosition(fullText, leadingComments[0].pos) > 1 ||
!result.content.endsWith("\n")

result.content =
(addFirstNewLineToLeadingComments ? "\n" : "") +
leadingCommentsText.join("\n") +
"\n" +
result.content
}

if (trailingComments.length > 0) {
const trailingCommentsText = trailingComments.map(
(v) => "#" + fullText.slice(v.pos + 2, v.end)
)
const addFirstNewLineToTrailingComments =
countNewlinesBeforePosition(fullText, trailingComments[0].pos) > 0

result.content =
result.content +
(addFirstNewLineToTrailingComments ? "\n" : " ") +
trailingCommentsText.join("\n") +
"\n"
}

props.commentsStack = props.commentsStack?.slice(
0,
lengthBeforeParsingThisNode
)
}

return result
}

export const testComments: Test = {
keepComments: true,
ts: `
// This is a test class
// abc
class Test extends Area2D {
// This a constructor
constructor() {
// This is a super call

}

// this is a method
method() {
// this is a print
print("a")

// This is an if statement
if (2 == 3) {
// This is an empty block
}

print(/* this is block comment */"l")

print(
Copy link
Owner

Choose a reason for hiding this comment

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

It's extremely cool that we can do inline comments without pulling them out into the previous statement... but does it work in the even more general case? I mean, TS can handle all sorts of weird stuff, like:

let foo = { bar: 1 }
console.log(foo. /* Hello */ bar)

That's actually valid and it does indeed print 1!

I feel like Godot would choke on that if we did it this way! Or would it?? Why don't we toss a few of these "weirder" cases into our test suite and test the codegen in Godot? If they all pass, that would be awesome, but if not, we could pull out inline comments into the previous line, e.g. the codegen for the above would be

var foo = { "bar" : 1 }
# Hello
console.log(foo.bar)

Copy link
Owner

Choose a reason for hiding this comment

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

If you do decide to go by the per-line route, I think the proper place to inject the comments would be here:

https://github.com/johnfn/ts2gd/blob/main/parse_node.ts#L195

Basically all code in ts2gd flows through combine so this is a pretty safe place to put it. We have this notion of beforeLines and afterLines that allow you to say "emit this code as a standalone line before/after the line currently being codegened" so we could use that for comments as well.

Copy link
Contributor Author

@adamuso adamuso Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's extremely cool that we can do inline comments without pulling them out into the previous statement... but does it work in the even more general case? I mean, TS can handle all sorts of weird stuff, like:

I'm a bit confused because this example print(/* this is block comment */"l") shows that all block/multiline /* ... */ comments are currently strpped out. Did I misunderstand something?

I didn't implement block comments because I do not really know how to handle them and I think they can be separated into another PR. In this PR I wanted to create base algorithm for comment parsing and only focus on single line comments :)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh - good point on the block comments, I glossed over that. However, what about something like this?

let foo = { bar: 1 }
console.log(foo. 
  // Hello
bar)

Copy link
Owner

Choose a reason for hiding this comment

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

I realize that specifically may seem like an obscure case, but I think the thing that I'm more generally worried about is that there are possibly a lot of cases like this: you can really put comments almost anywhere in JS/TS, but if you map that 1:1 to Godot, there might be a lot of weird cases that don't compile correctly.

// this is a call with parameters in multiple lines
"hello",
// this is second parameter
"world"
)

// this is a print
// this is a print
// and this is third comment
print("b")

print("x") // leading comment
}
}
`,
expected: `
# This file has been autogenerated by ts2gd. DO NOT EDIT!
extends Area2D
class_name Test
# This is a test class
# abc
# This a constructor
func _ready():
pass
# this is a method
func method():
# this is a print
print("a")
# This is an if statement
if ((typeof(2) == typeof(3)) and (2 == 3)):
pass
print("l")
print(
# this is a call with parameters in multiple lines
"hello",
# this is second parameter
"world")
# this is a print
# this is a print
# and this is third comment
print("b")
print("x") # leading comment
`,
}
4 changes: 4 additions & 0 deletions project/tsgd_json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ export class Paths {
/** The path to the Godot repository, e.g. /Users/johnfn/Godot */
godotSourceRepoPath: string | undefined

/** Should we strip comments from the godot output? */
removeComments: boolean

constructor(args: ParsedArgs) {
if (args.init) {
this.init()
Expand Down Expand Up @@ -95,6 +98,7 @@ export class Paths {
"dynamic"
)

this.removeComments = tsgdJson.removeComments || false
Copy link
Owner

Choose a reason for hiding this comment

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

nit: ?? (maybe change the other line too :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean why this is in Paths class or why this is called like that?

It is in Paths because I couldn't find another class which have access to ts2gd.json file and I wanted comment generation to be configurable from config.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I just mean instead of || we can use ?? e.g. this.removeComments = tsgdJson.removeComments ?? false

Copy link
Contributor Author

@adamuso adamuso Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, still getting used to newer TypeScript after working a lot in old JS code :P

Copy link
Owner

Choose a reason for hiding this comment

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

:D I know that feeling. Welcome to the future my friend!!

this.godotSourceRepoPath = tsgdJson.godotSourceRepoPath || undefined

this.tsconfigPath = path.join(
Expand Down
37 changes: 25 additions & 12 deletions tests/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export type Test = {
isAutoload?: boolean
only?: boolean
expectFail?: boolean
keepComments?: boolean
}

type TestResult = TestResultPass | TestResultFail
Expand Down Expand Up @@ -191,13 +192,19 @@ const removeCommentLines = (s: string) => {
.join("\n")
}

const normalize = (s: string) => {
return removeCommentLines(trim(s))
const normalize = (s: string, keepComments?: boolean) => {
return keepComments ? trim(s) : removeCommentLines(trim(s))
}

const areOutputsEqual = (left: string, right: string) => {
const leftTrimmed = removeCommentLines(trim(left))
const rightTrimmed = removeCommentLines(trim(right))
const areOutputsEqual = (
left: string,
right: string,
keepComments?: boolean
) => {
const leftTrimmed = keepComments ? trim(left) : removeCommentLines(trim(left))
const rightTrimmed = keepComments
? trim(right)
: removeCommentLines(trim(right))

return leftTrimmed === rightTrimmed
}
Expand All @@ -208,7 +215,7 @@ const test = (
testFileName: string,
path: string
): TestResult => {
const { ts, expected } = props
const { ts, expected, keepComments } = props

let compiled: ParseNodeType | null = null
let errors: TsGdError[] = []
Expand Down Expand Up @@ -277,7 +284,7 @@ ${errors[0].description}
}

if (typeof expected === "string") {
if (areOutputsEqual(output, expected)) {
if (areOutputsEqual(output, expected, keepComments)) {
return { type: "success" }
}
} else {
Expand All @@ -300,12 +307,18 @@ ${errors[0].description}

for (const actualFile of compiled.files ?? []) {
if (actualFile.filePath === expectedFile.fileName) {
if (!areOutputsEqual(actualFile.body, expectedFile.expected)) {
if (
!areOutputsEqual(
actualFile.body,
expectedFile.expected,
keepComments
)
) {
return {
type: "fail",
fileName: actualFile.filePath,
result: normalize(actualFile.body),
expected: normalize(expectedFile.expected),
result: normalize(actualFile.body, keepComments),
expected: normalize(expectedFile.expected, keepComments),
name,
expectFail: props.expectFail ?? false,
path,
Expand Down Expand Up @@ -337,8 +350,8 @@ ${errors[0].description}

return {
type: "fail",
result: normalize(output),
expected: normalize(expected),
result: normalize(output, keepComments),
expected: normalize(expected, keepComments),
name,
expectFail: props.expectFail ?? false,
path,
Expand Down