-
Notifications
You must be signed in to change notification settings - Fork 166
improve detection for inherited symbol references #257
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
Conversation
|
@swift-ci Please test |
29b16ef to
b558bac
Compare
|
@swift-ci Please test |
| // If we don't have a resolved `sourceOrigin` parent, fall back to parsing its display name | ||
|
|
||
| // Detect the path components of the providing the default implementation. | ||
| let typeComponents = originDisplayName.components(separatedBy: ".").filter({ !$0.isEmpty }) |
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.
Nit: The default behavior of the Swift native String.split actually does this filtering for empty components automatically: https://developer.apple.com/documentation/swift/string/split(separator:maxsplits:omittingemptysubsequences:)
| let typeComponents = originDisplayName.components(separatedBy: ".").filter({ !$0.isEmpty }) | |
| let typeComponents = originDisplayName.split(separator: ".") |
| defer { | ||
| try? FileManager.default.removeItem(at: bundleURL) | ||
| } |
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.
Nit: testBundleAndContext uses a test teardown block to remove any created temp files.
| defer { | |
| try? FileManager.default.removeItem(at: bundleURL) | |
| } |
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.
We should probably clean up all the other tests that use their own defer blocks, then; i was working off the pattern elsewhere in the file! 😅
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.
Yeah, I've started to clean it up as I encounter them. Here are some changes from my link resolution PR. That PR broke a lot of tests in the early stages of the integration 😅
|
I've pushed up changes for the review comments. @swift-ci Please test |
ethan-kusters
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.
Looks great. Thank you!!
e427bd2 to
9fcd568
Compare
|
Rebased onto the latest @swift-ci Please test |
rdar://93203894 Symbols for operator functions that include multiple dots (e.g. the ClosedRange `...` operator) are currently curated into an unnamed "Implementations" section due to improper string splitting behavior. This enhances the detection two ways: 1. For symbols where the "source origin" is available in our given symbol graphs, it loads the name information from the parent symbol. 2. For symbols where the "source origin" is not available, it improved the string splitting behavior by dropping empty-string components before reading names from it. This allows Comparable and Equatable implementations to curate their inherited operators properly.
9fcd568 to
f2d37da
Compare
|
Rebased. @swift-ci Please test |
Bug/issue #, if applicable: rdar://93203894
Summary
Symbols for operator functions that include multiple dots (e.g. the
ClosedRange
...operator) are currently curated into an unnamed"Implementations" section due to improper string splitting behavior.
This enhances the detection two ways:
symbol graphs, it loads the name information from the parent symbol.
the string splitting behavior by dropping empty-string components
before reading names from it.
This allows Comparable and Equatable implementations to curate their
inherited operators properly.
Dependencies
None
Testing
Steps:
....operator in theFancyProtocolsymbol graph added in this PR).Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded