Support for comments modifiers #385

iMostfa · 2022-04-20T10:33:06Z

Hi everyone!
In this PR I'm discussing how we can add comments support in SwiftSyntaxBuilder based on my discussion here,
and as part of my interest in this project.

Goal

the goal is to be able to add comments to `BuildableNodes` in a declarative way, much like this:

SourceFile {
    ImportDecl(path: "Foundation")
      .withLineComment("Foundation is important")
    ImportDecl(path: "UIKit")
    ClassDecl(classOrActorKeyword: .class, identifier: "SomeViewController", membersBuilder: {
      VariableDecl(.let, name: "tableView", type: "UITableView")
    })
    .withDocBlokComment("Used as login screen")

//output:- 

// Foundation is important
import Foundation
import UIKit
/** Used as login screen */
class SomeViewController{
    let tableView: UITableView
}
  }

Steps

to achieve this goal, I did the following

1- Added itemLeadingTrivia for each BuildableNode
2- when the build function for a base type is called, i added the following(e.g BuildDecl)

  public func buildDecl(format: Format, leadingTrivia: Trivia? = nil) -> DeclSyntax {
    let result = buildFunctionDecl(format: format, leadingTrivia: leadingTrivia)
    return DeclSyntax(result).withLeadingTrivia(itemLeadingTrivia ?? .zero)
  }

3- added withLineComment, and withDocBlokComment on each BuildableNode, when called, they would return a new modified value of Self( to avoid mutating)

Output

doing the following was enough to get our desired output 🤩:

 SourceFile {
    
    ImportDecl(path: "Foundation")
    ImportDecl(path: "SceneKit")
      .withDocBlokComment("Comment 1")
    
    FunctionDecl(funcKeyword: .func,
                 identifier: .identifier("visit"),
                 signature: functionSignature(nodeType: "node",
                                              output: "Output")) {
    } modifiersBuilder: {
      TokenSyntax.public
    } bodyBuilder: {
      
    }.withLineComment("comment 2")
      .withDocBlokComment("comment 3")
    
  }.withLineComment("Comment 0")

// Output: -

// Comment 0
import Foundation
/** Comment 1 */
import SceneKit
// comment 2
/** comment 3 */
public func visit(_ node: Output)-> Output{
}

Bugs and Questions 🐞

while everything work as expected, i noticed that the comments doesn't get genrated for first CodeBlockItem inside a SourceFile {} , as an example:

  SourceFile {
    
    ImportDecl(path: "Foundation")
      .withDocBlokComment("IMPORTANT COMMENT")

    ImportDecl(path: "SceneKit")
      .withDocBlokComment("Comment 1")
    
    FunctionDecl(funcKeyword: .func,
                 identifier: .identifier("visit"),
                 signature: functionSignature(nodeType: "node",
                                              output: "Output")) {
    } modifiersBuilder: {
      TokenSyntax.public
    } bodyBuilder: {
      
    }.withLineComment("comment 2")
      .withDocBlokComment("comment 3")
    
  }.withLineComment("Comment 0")


// OUTPUT: - 

// Comment 0
import Foundation //<- Notice, no IMPORTANT COMMENT was generated
/** Comment 1 */
import SceneKit
// comment 2
/** comment 3 */
public func visit(_ node: Output)-> Output{
}

also if there's only one Declaration/CodeBlockItem inside the SourceFile, the same problem happens

  SourceFile {

    FunctionDecl(funcKeyword: .func,
                 identifier: .identifier("visit"),
                 signature: functionSignature(nodeType: "node",
                                              output: "Output")) {
    } modifiersBuilder: {
      TokenSyntax.public
    } bodyBuilder: {
      
    }.withLineComment("comment 2")
      .withDocBlokComment("comment 3")
    
  }

I'm still investigating it, I think it might be related to the way ResultBuilders work? would need your opinions about it.

I have checked Swift AST Explorer and found that it's expected for a first CodeBlockItem to have a comment as Leading Trivia,

also, what do you think about the direction I'm taking to solve and implement the comments support? should I continue with the rest of the comments type ? or there's something better I should do?

Thank you for your time reading this 🧑🏻‍💻

ahoppen

This is looking good overall. I added a few comments inline (it looks worse on first sight than it really is 🙈). Could you also add a few test cases for the new functionality?

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

ahoppen · 2022-04-21T14:27:56Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+  }
+
+   public func withLineComment(_ text: String) -> Self {
+     let piece = TriviaPiece.lineComment("// \(text)")


This will behave surprisingly if text contains a newline because the second line will not have a //. I think we should insert // in front of every line. As an added bonus, you could make sure that these lines have the same indentation as the current one, by taking a look at the leading trivia after the last .newline in the node’s current leadingTrivia.

Working on this one 🤝

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

iMostfa · 2022-04-22T04:56:07Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+
+     if let itemLeadingTrivia = itemLeadingTrivia
+    {
+      let formattedLeadingTrivia = [itemLeadingTrivia, format._makeIndent()].reduce(.zero, +)


Thanks, @ahoppen for pointing me out to

If you now stick that ImportDecl into a SourceFile and add a comment to the SourceFile

I've solved it by doing the following, what do you think?،
also, this should support comments indiation

I think this will solve the indentation problem but not your problem with dropping IMPORTANT COMMENT as you described in the PR description.

I would try one of the following (haven’t tested myself):

if let itemLeadingTrivia = itemLeadingTrivia { leadingTrivia = itemLeadingTrivia + leadingTrivia // Alternative 1 leadingTrivia = format._makeIndent() + itemLeadingTrivia + leadingTrivia // Alternative 2 } let result = build${type.base_name()}(format: format, leadingTrivia: leadingTrivia)

and dropping the withLeadingTrivia down here.

actually, I think it solved the dropping of the first Comment In SourceFile {}, for example:
the output is:

// IMPORTANT COMMENT import Foundation // lineComment // hello /* This's the new block comment with new lines support */ import UIKit class SomeViewController{ /** This's the first nested comment */ let tableView: UITableView class SomeViewController{ // this's a also a nested comment let mainNode: SCNNode } }

for the following SourceFile

SourceFile { ImportDecl(path: "Foundation") .lineComment("IMPORTANT COMMENT") ImportDecl(path: "UIKit") .lineComment( """ lineComment hello """ ) .blockComment("This's the new block comment\nwith new lines support") ClassDecl(classOrActorKeyword: .class, identifier: "SomeViewController", membersBuilder: { VariableDecl(.let, name: "tableView", type: "UITableView") .docBlockComment("This's the first nested comment") ClassDecl(classOrActorKeyword: .class, identifier: "SomeViewController", membersBuilder: { VariableDecl(.let, name: "mainNode", type: "SCNNode") .lineComment("this's a also a nested comment") }) }) }//.lineComment("This comment will override important comment if placed")

(if a comment on SourceFile is placed, it will override the one on the first item, should we handle it in a special way, which adds both of the comments instead of overriding) ?

is the current output enough for our needs?

please let me know if I'm missing something, also I will check your snippets and see how would they differ :-)

Thank you for the review and for helping me.

(if a comment on SourceFile is placed, it will override the one on the first item, should we handle it in a special way, which adds both of the comments instead of overriding) ?

Yes, definitely. I don’t think we should be dropping comments if the user explicitly asked for them.

Could you add this test case to Tests/SwiftSyntaxBuilderTest?

ahoppen · 2022-04-22T06:39:28Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+     if let itemLeadingTrivia = itemLeadingTrivia
+    {


Nitpick

Suggested change

if let itemLeadingTrivia = itemLeadingTrivia

{

if let itemLeadingTrivia = itemLeadingTrivia {

ahoppen · 2022-04-22T06:43:20Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+
+     if let itemLeadingTrivia = itemLeadingTrivia
+    {
+      let formattedLeadingTrivia = [itemLeadingTrivia, format._makeIndent()].reduce(.zero, +)


I think this will solve the indentation problem but not your problem with dropping IMPORTANT COMMENT as you described in the PR description.

I would try one of the following (haven’t tested myself):

if let itemLeadingTrivia = itemLeadingTrivia { leadingTrivia = itemLeadingTrivia + leadingTrivia // Alternative 1 leadingTrivia = format._makeIndent() + itemLeadingTrivia + leadingTrivia // Alternative 2 } let result = build${type.base_name()}(format: format, leadingTrivia: leadingTrivia)

and dropping the withLeadingTrivia down here.

iMostfa · 2022-04-25T00:55:00Z

I added some tests related to comments behaviors.
I've found that extra work has to be done to support multiple lines indentation, and I'm working on it.

iMostfa · 2022-04-25T05:01:42Z

I've added 11 test cases, which handles most of the cases:

4 Test Cases for comments types

testDocLineComment
testLineComment
testBlockComment
testDocBlockComment

2 Tests for multiple line Support

testMultipleLineComments
testSourceFileMultipleLineComments

3 Tests for SoureFile Comments

testSourceFileCommentsWhenFirstNodeHasComment
testSourceFileSingleLineComments
testFirstNodeComment

2 Indentation Tests

testNestedCommentsIndentation
testMultipleLineCommentsIndentation

Thank you for encouraging me to write tests, it helped me discover more cases that weren't handled.
and I will continue working on enhancing the current implementation

ahoppen · 2022-04-25T08:33:50Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+    %   if type.buildable() != 'SourceFile':
    let result = build${type.base_name()}(format: format, leadingTrivia: leadingTrivia)
+    % end
+    %   if type.buildable() == 'SourceFile':
+    let indentedLines = itemLeadingTriviaPieces
+      .map { Trivia(pieces: [$0, .newlines(1)]) }
+      .reduce(.zero, +)
+
+    let combinedTrivia = [format._makeIndent(),
+                          leadingTrivia,
+                          indentedLines]
+      .compactMap { $0 }
+      .reduce(.zero, +)
+    %   if type.buildable() == 'SourceFile':
+    let result = buildSourceFile(format: format, leadingTrivia: combinedTrivia)
+    % end
+    % else:
+
+      if !itemLeadingTriviaPieces.isEmpty {
+        let indentedLeadingTrivia = itemLeadingTriviaPieces
+          .compactMap { Trivia(pieces:[ $0, .newlines(1)]) }
+          .map { $0 + format._makeIndent() }
+          .reduce(.zero, +)
+        return ${base_type.syntax()}(result).withLeadingTrivia(indentedLeadingTrivia)
+      }
+    %   end


I don’t think the special handling for SourceFile is sufficient here. The same issue will arise for any syntax node that doesn’t have a token before its first child node (I’m happy to be proven wrong by a test case). For example we should see the same issue if you try to create a TypealiasDecl like the following where both the private attribute and the TypealiasDecl itself have comments attached to it.

/// This comment is added to the TypealiasDecl /// This comment is added to the `private` keyword private typealias MyInt = Int

Is there any issue with applying the solution you have implemented for SourceFile to all syntax nodes (I haven’t checked).

Also, I think there’s a few end missing and the ones that do exist have the wrong indentation level sometimes. Could you make sure that every if-statement has a matching end on the same indentation level?

ahoppen · 2022-04-25T08:36:30Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+  /// Extracts Lines from a given text, and add a prefix to it
+  /// - Parameters:
+  ///   - text: Text which its lines will be prefixed
+  ///   - prefix: prefix for each line
+  /// - Returns: an array of new string, which has prefixed lines
+  private func prefixLines(of text: String, with prefix: String) -> [String] {
+
+    let prefixedLines = text
+      .split(whereSeparator: \.isNewline)
+      .map {"\(prefix) \($0)"}
+
+    return prefixedLines
+  }


A little bit of a nitpick but I think this is a general helper method and not something that needs to access instance members of *Buildable. Thus, I think it would make sense to make it a free function instead of an instance function.

ahoppen · 2022-04-25T08:39:20Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+  /// - Parameter text: comment to be inserted after ///
+  /// - Returns: a `${type.buildable()}` with the added documentation line comment.
+  public func docLineComment(_ text: String) -> Self {
+    let textWithSlashesPieces = prefixLines(of: text, with: "//")


This should probably be

Suggested change

let textWithSlashesPieces = prefixLines(of: text, with: "//")

let textWithSlashesPieces = prefixLines(of: text, with: "///")

Could you also add a test case for it?

ahoppen · 2022-04-25T08:41:37Z

Sources/SwiftSyntaxBuilder/BuildableNodes.swift.gyb

+    return addCommentPiece(commentPieces: [piece])
+  }
+
+  private func addCommentPiece(commentPieces: [TriviaPiece]) -> Self {


Per the Swift Naming guidelines this should be named addingCommentPiece because it returns a modified version of self instead of modifying self in-place.

ahoppen · 2022-04-25T08:42:59Z

Tests/SwiftSyntaxBuilderTest/CommentsTests.swift

+          // Plane node used to represent in a scene.
+          // Each scene is supposed to have only one plane.


It’s great that this works! Could you also add a similar test case for block comments?

ahoppen · 2022-07-07T08:22:54Z

Superseded by #475

Update for `async` being a contextual keyword.

iMostfa requested a review from ahoppen as a code owner April 20, 2022 10:33

iMostfa force-pushed the CommentsSupport branch from adc3491 to 8ae73f4 Compare April 20, 2022 10:39

ahoppen reviewed Apr 21, 2022

View reviewed changes

iMostfa force-pushed the CommentsSupport branch from 9302de1 to 7e2f6bc Compare April 22, 2022 04:54

iMostfa commented Apr 22, 2022

View reviewed changes

ahoppen reviewed Apr 22, 2022

View reviewed changes

Supported comments for more types

bc6c721

iMostfa force-pushed the CommentsSupport branch from 7e2f6bc to bc6c721 Compare April 22, 2022 07:29

Supported Comments in SourceFile

b665b8e

iMostfa force-pushed the CommentsSupport branch from 635b925 to b665b8e Compare April 22, 2022 22:02

iMostfa added 2 commits April 25, 2022 02:12

Added Comments Tests.

fa82ee1

Adds SourceFile comments test

a8dbca1

Better handling form comments with multi-lines, and more test cases.

10993f5

ahoppen reviewed Apr 25, 2022

View reviewed changes

ahoppen mentioned this pull request Jun 8, 2022

Take attribute and modifier lists directly in declaration convenience initializers #460

Closed

ahoppen closed this Jul 7, 2022

adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024

Merge pull request swiftlang#385 from allevato/async-contextual-keyword

f87ecde

Update for `async` being a contextual keyword.

	if let itemLeadingTrivia = itemLeadingTrivia
	{
	if let itemLeadingTrivia = itemLeadingTrivia {

	let textWithSlashesPieces = prefixLines(of: text, with: "//")
	let textWithSlashesPieces = prefixLines(of: text, with: "///")

		// Plane node used to represent in a scene.
		// Each scene is supposed to have only one plane.

Support for comments modifiers #385

Support for comments modifiers #385

Uh oh!

Conversation

iMostfa commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Steps

Output

Bugs and Questions 🐞

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iMostfa commented Apr 25, 2022

Uh oh!

iMostfa commented Apr 25, 2022

4 Test Cases for comments types

2 Tests for multiple line Support

3 Tests for SoureFile Comments

2 Indentation Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 7, 2022

Uh oh!

Uh oh!

iMostfa commented Apr 20, 2022 •

edited

Loading