swiftlang / swiftlang/swift-syntax

`AttributeRemover` outputs a leading newline in some cases

Open
#2,269 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
3.7k
Forks
553
Avg merge
5d 13h
Merged PRs (30d)
16

Description

### Description

#2247 better isolates the `AttributeRemoverTests` to just `AttributeRemover`. Along the way, a new assert helper was created that, unlike `assertMacroExpansion`, doesn't trim newlines before comparing values.

Notably, this revealed a previously undetected behavior in `AttributeRemover`: it outputs an additional leading newline when the removed attribute isn't preceded by any tokens. For instance:

```swift
assertSyntaxRemovingTestAttributes {
"""
@Test
var x: Int
"""
} reduction: {
"var x: Int"
}
//failed - Attribute removal did not produce the expected reduced source
//+
// var x: Int
//
//Actual reduced source:
//
//var x: Int
```

and:

```swift
assertSyntaxRemovingTestAttributes {
"""
@Test
/* comment */
var value: Int
"""
} reduction: {
"""
/* comment */
var value: Int
"""
}
//failed - Attribute removal did not produce the expected reduced source
//+
// /* comment */
// var value: Int
//
//Actual reduced source:
//
///* comment */
//var value: Int
```

The following tests outputs have an extra leading newline:

- [`testEmptyOnOwnLineBeforeVariable`](https://github.com/apple/swift-syntax/blob/main/Tests/SwiftSyntaxMacroExpansionTest/AttributeRemoverTests.swift#L59)
- [`testEmptyTwiceOnOwnLineBeforeVariable`](https://github.com/apple/swift-syntax/blob/main/Tests/SwiftSyntaxMacroExpansionTest/AttributeRemoverTests.swift#L72)
- [`testEmpyNewlineBlockComment`](https://github.com/apple/swift-syntax/blob/main/Tests/SwiftSyntaxMacroExpansionTest/AttributeRemoverTests.swift#L184)
- [`testEmptyAndAttributeOnOwnLinesBeforeVariable`](https://github.com/apple/swift-syntax/blob/main/Tests/SwiftSyntaxMacroExpansionTest/AttributeRemoverTests.swift#L370)
- [`testEmptyOnOwnLineThenEmptyBeforeVariable`](https://github.com/apple/swift-syntax/blob/main/Tests/SwiftSyntaxMacroExpansionTest/AttributeRemoverTests.swift#L403)

Before #2247, this behavior is masked by `assertMacroExpansion`. In #2247, the above tests are "fixed" by including the unwanted leading newlines in the expected output and marking those tests with `FIXME`s (to be properly fixed in a separate PR).

### Steps to Reproduce

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.