-
Notifications
You must be signed in to change notification settings - Fork 166
Fix regression with duplicated generated implementation sections #321
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.
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
Fix regression with duplicated generated implementation sections #321
Conversation
With the recent fix for supporting generated implementation sections for
symbols that include periods (like `Comparable...<`), we introduced a
regression where it's possible to end up with a duplicate default
implementation section.
This can happen when a protocol has a mix of documentation coverage. For example
```swift
public protocol Foo {
/// This is my great doc.
func fooMemberWithDocComment()
func fooMemberWithoutDocComment()
}
public extension Foo {
func fooMemberWithDocComment() { }
func fooMemberWithoutDocComment() { }
}
public struct Bar: Foo {}
```
Here `Bar` would end up with two "Foo Implementations" sections.
This is because we're able to access the original parent symbol for
`fooMemberWithDocComment()` but not for `fooMemberWithoutDocComment` so
these relationships end up with a different unique identifier but the same
collection name: "Foo Implementations".
The fix is to not rely on the parent relationship being there and instead
look at the origin symbol's parent path components. We can rely on the origin
symbol always being there for protocols defined in the current module.
This also resolves an issue where conforming to compmlicated protocols in a
different module would produce a misnamed collection. For example:
```swift
// FirstTarget
infix operator .<.. : RangeFormationPrecedence
public protocol OtherFancyProtocol {
static func .<.. (lhs: OtherFancyProtocol, rhs: OtherFancyProtocol)
}
public extension OtherFancyProtocol {
static func .<.. (lhs: OtherFancyProtocol, rhs: OtherFancyProtocol) {}
}
```
```swift
// SecondTarget
import FirstTarget
public struct OtherFancyProtocolConformer: OtherFancyProtocol {}
```
Here we would end up with a default collection named "< Implementations"
instead of "OtherFancyProtocol Implementations". The solution is to figure
out the actual name of the symbol based on the source symbol which is always
guaranteed to be there. We can use this to split out the parent symbol's name
from the source origin display name.
Resolves rdar://95808950.
| originParentSymbol = parentOfFunction(originSymbol.reference) | ||
| } | ||
| let originSymbol = context.symbolIndex[origin.identifier]?.symbol | ||
| let sourceSymbol = context.symbolIndex[relationship.source]?.symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checked above as let child = context.symbolIndex[relationship.source] so it should never be nil.
I think this means that the sourceSymbol argument to add(...) could also be non-optional which removes the need for the else case in that implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice catch! Thanks. Addressed here: 976c858.
I think we should still leave the else to reduce risk here in case the child symbol is an unexpected length but this definitely make things clearer.
|
@swift-ci please test |
d-ronnqvist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
QuietMisdreavus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks!
Bug/issue #, if applicable: rdar://95808950
Summary
With the recent fix for supporting generated implementation sections for
symbols that include periods (like
Comparable...<) (#257), we introduced aregression where it's possible to end up with a duplicate default
implementation section.
This can happen when a protocol has a mix of documentation coverage. For example
Here
Barwould end up with two "Foo Implementations" sections.This is because we're able to access the original parent symbol for
fooMemberWithDocComment()but not forfooMemberWithoutDocCommentsothese relationships end up with a different unique identifier but the same
collection name: "Foo Implementations".
The fix is to not rely on the parent relationship being there and instead
look at the origin symbol's parent path components. We can rely on the origin
symbol always being there for protocols defined in the current module.
This also resolves an issue where conforming to compmlicated protocols in a
different module would produce a misnamed collection. For example:
Here we would end up with a default collection named "< Implementations"
instead of "OtherFancyProtocol Implementations". The solution is to figure
out the actual name of the symbol based on the source symbol which is always
guaranteed to be there. We can use this to split out the parent symbol's name
from the source origin display name.
Testing
Build documentation with the examples included in the summary (also included in the attached Swift Package) and confirm that they produce automatic collections as expected.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded