swiftlang / swift-format

Formatting technology for Swift source code
Apache License 2.0
2.49k stars 229 forks source link

[SR-11798] Scrunching of Range Operators Breaks Code #341

Closed swift-ci closed 4 years ago

swift-ci commented 4 years ago
Previous ID SR-11798
Radar None
Original Reporter SDGGiesbrecht (JIRA User)
Type Bug
Status Resolved
Resolution Done
Environment swift-5.1-branch as of 2019‐11‐17
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | swift-format | |Labels | Bug | |Assignee | @allevato | |Priority | Medium | md5: 0ba030d617ad8d07c00151cd32d99a3e

Issue Description:

Start with this code:

for _ in 1 ... |x| { /* ... */ }
Int.random(in: −10 ... −1)

Run SwiftFormat once and the operators become conjoined, failing to compile:

for _ in 1...|x| { /* ... */ }
Int.random(in: −10...−1)

Run SwiftFormat again and the spaces are restored, but in the wrong places—it’s still broken:

for _ in 1 ...| x| { /* ... */ }
Int.random(in: −10 ...− 1)

I would expect range operators to be spaced with the same uniformity as every other infix operator, but if the scrunched variant is desired, then it should at least be restricted to the situations where it is actually valid code.

swift-ci commented 4 years ago

Comment by Jeremy David Giesbrecht (JIRA)

I think pull request #91 aimed to solve this? Attn: @allevato

I just discovered it can also break if the upperBound starts with a dot, such as an enumeration case:

let range: ClosedRange<SomeEnumeration> = .caseFive ... .caseTen

Did your fix happen to handle that too or not?

allevato commented 4 years ago

The original PR didn't handle that case, because it only considered following tokens that were `prefixOperator`s, whereas the case above is `prefixPeriod` because it's not "technically" an operator in the case of implicit member references.

https://github.com/apple/swift-format/pull/98 addresses this case.

Arguably, a safer way to check this would be to simply check the previous and next characters to see if they're operator code points, but I don't really want to hardcode that list into the formatter—after all, the whole point of using the compiler's syntax parser is to have it do the work so we don't have to duplicate it.

@akyrtzi, maybe the syntax parser library could be extended to have generally useful utility functions, like "is this character an identifier character or operator character"?

allevato commented 4 years ago

https://github.com/apple/swift-format/pull/98 has been merged so this should be fixed now. (It, among other commits, still needs to be cherry-picked into the 5.1 branch.)