[SR-6605] Doc comment merging issues

johnfairh commented 6 years ago


Previous ID	SR-6605
Radar	rdar://82414210
Original Reporter	@johnfairh
Type	Bug
Status	Resolved
Resolution	Done

Environment

Xcode 9.2

Additional Detail from JIRA

| | | |------------------|-----------------| |Votes | 0 | |Component/s | Source Tooling | |Labels | Bug | |Assignee | @bnbarham | |Priority | Medium | md5: 0e37b8ffa8a381dbc5e9566da25b0955

Issue Description:

Handling of doc comments that should not be merged is a bit wrong.

First problem:

/** one */

/** two */
let v = 1

...gives an empty doc comment. Per the intent of the code it should give ‘two’. (Maybe someone wants to argue for changing to ‘one two’ but empty has to be wrong.)

Second problem:

/// one

/// two
let w =1

...gives a doc comment of ‘one two’. Changing the spacing to two blank lines gives an empty doc comment again.

I’ll try a PR to fix both parts. I’m conscious that code might be using the second behavior today - the two instances I found in swift though look as though they would be better without merging:
swiftpm swift/stdlib

krilnon commented 6 years ago

@natecook1000 probably has thoughts on this. How did you search/grep for the potential impact of this change?

johnfairh commented 6 years ago

How did you search?

'poorly' basically – with a better regex today:

find . \( -name '*.swift' -or -name '*.swift.gyb' \) -type f -exec pcregrep -HM ^\\s*///.*?\\n\\s*\\n\\s*/// {} \+

Real list in the swift project is:

llbuild - typo on MARK comment
stdlib/StringBridge.swift as above - typo, not a doc comment for decl
SwiftSyntax/Syntax.swift - MARK typo
corelibs-foundation/FileHandle.swift - not doc comments
corelibs-foundation/NSError.swift - not doc comments (category names)
corelibs-foundation/Progress.swift - MARK typo
corelibs-foundation/EasyHandle.swift - unintended doc comment merging
swiftpm/Path.swift - not doc comments
swiftpm/PathShims.swift as above - not doc comments
swiftpm/XcodeProjectModelSerialization.swift - partially not a doc comment + partially a doc comment in the wrong place

These are all marginally improved by fixing /// merging; there are no places intentionally taking advantage of it. Scanning a handful of open source projects turned up one hit, another 'MARK' typo.

bnbarham commented 3 years ago

I had a look at the source compatibility suite and there's no case where we want to merge:

Block comments together (mixed or otherwise)
Comments with a newline in-between

There's a few cases where not merging is better though, so I've fixed the empty case and prevented block comments merging altogether in https://github.com/apple/swift/pull/39086

EDIT: I forgot to look for `/// ... /**` - there are a few cases of that (all in `RxSwift`). Guess I'll keep the merging of mixed comments!

bnbarham commented 3 years ago

Resolved in https://github.com/apple/swift/pull/39086

swiftlang / swift

[SR-6605] Doc comment merging issues #49155