Add initial implementation of a consolidated availability logic - #1674
Conversation
patshaughnessy
left a comment
There was a problem hiding this comment.
Amazing work so far - can't wait to see where you go with this!
I assume unit tests will come later? Or will the existing unit test coverage test this automatically when we start using it?
| } | ||
| } | ||
|
|
||
| func preservingBugThatArticlesDoNotDisplayDefaultAvailability() -> Self { |
There was a problem hiding this comment.
Is the idea we will call this later, for articles only?
There was a problem hiding this comment.
Yes, the integration calls this for the common base value for articles.
| for module in unifiedGraph.moduleData.values { | ||
| guard let knownPlatform = KnownPlatform(module.platform) else { | ||
| // We only care about tracking known values for symbol graph's platforms. | ||
| continue |
There was a problem hiding this comment.
Should we add a debug assertion here, for the case we ever receive an unknown platform from the unified symbol graph?
There was a problem hiding this comment.
No. There can be unknown platforms but AFAICT the current implementation doesn't track them.
| // Any platform that's left as `havePlatformForInSymbolGraph` after this loop indicates a platform that the symbol was excluded from using conditionally compilation. | ||
| for selector in unifiedSymbol.allSelectors where selector.interfaceLanguage == languageFilter { | ||
| guard let selectorPlatform = KnownPlatform(selector) else { | ||
| continue |
There was a problem hiding this comment.
Same here: debug assertion?
There was a problem hiding this comment.
I don't think so. There can be unknown platforms but AFAICT the current implementation doesn't track them.
| ) | ||
| } | ||
| } else if let domainName = availability.domain?.rawValue { | ||
| customPlatformsByName[domainName] = if availability.isUnconditionallyUnavailable || availability.obsoletedVersion != nil { |
There was a problem hiding this comment.
Ah so symbol graphs can contain unknown platforms?
There was a problem hiding this comment.
The selector platform and the specified domain name of an in-source attribute are two different things.
|
|
||
| /// Finished computing the combined availability information by applying "fallback" behaviors for iPadOS and Mac Catalyst. | ||
| mutating func finalizePlatformFallbacks() { | ||
| let valueToCopy = knownPlatforms[.iOS] |
There was a problem hiding this comment.
Maybe add "iOS" into the identifier valuetToCopy to make the following code easier to follow.
There was a problem hiding this comment.
I don't know. For me it doesn't matter much to this where the value came from, only that it's the value that's copied for the other fallbacks.
| /// | ||
| /// A page as a whole is considered to be deprecated if the API is deprecated on all the platforms that the API is available for. | ||
| var isDeprecated: Bool { | ||
| return knownPlatforms.allSatisfy { |
There was a problem hiding this comment.
Will this work correctly when there are no platforms? Or if all the platforms are unavailable?
Should we add a guard similar to above in isBeta?
There was a problem hiding this comment.
If it doesn't we don't have any test coverage from that elsewhere in DocC.
There was a problem hiding this comment.
Is there a list of things we'd like to clean up about the availability behavior once this consolidated logic is merged? Maybe it's worth adding a test for this.
Also, fix minor typo in code comment.
Would it be easier or harder to review this new implementation if I also pushed the code that integrates it the same PR (compared to posting a follow up PR with the integration changes)?
I've been adding tests covering the availability logic for months, primarily in
Like I mentioned in that first PR; the goal of those tests was to gain coverage of the observable behaviors of the current implementation so that the planned new implementation could keep the same behaviors. The idea is that by integrating this new code and using it to compute the values that are observable in the rendered output, we can verify that every behavior that those tests cover remain the same. |
mayaepps
left a comment
There was a problem hiding this comment.
This looks good to me. I personally found it easier to review with just these changes in this PR, rather than also including the integration. You could consider opening a second PR targeting this branch with the integration, if you'd prefer to ultimately merge both into main at the same time.
| /// | ||
| /// - Parameter defaultAvailability: The decoded "default" availability, or `nil` if the inputs don't specify any "default" availability. | ||
| init(defaultAvailability: [DefaultAvailability.ModuleAvailability]?) { | ||
| knownPlatforms = [Information](repeating: Information(state: .unavailable, source: .initialValue), count: KnownPlatform.wildcard.rawValue + 1 /* once for all known platforms */) |
There was a problem hiding this comment.
Is space-efficiency the reason you're not using a dictionary here?
There was a problem hiding this comment.
It's part of the reason but I'm also trying to avoid unnecessary work of sorting the platforms when there's a known list of values and the expected order is known beforehand.
| /// | ||
| /// This is its own method because this computation only needs to be performed once per module. | ||
| /// Alternatively, if `addInSourceAvailability(from:languageFilter:)` was responsible for this task, | ||
| /// then the caller couldn't forget to perform this task but it would have to performed once per symbol instead once per module. |
There was a problem hiding this comment.
nit:
| /// then the caller couldn't forget to perform this task but it would have to performed once per symbol instead once per module. | |
| /// then the caller couldn't forget to perform this task but it would have to be performed once per symbol instead once per module. |
There was a problem hiding this comment.
Note also the parameter documentation below has an unfinished sentence.
| /// | ||
| /// - Parameters: | ||
| /// - unifiedSymbol: The symbol to read the unified in-source availability annotations from. | ||
| /// - languageFilter: The string identifier of a source language, that that container used to restrict the container to only add information that matches the provides which information the container reads from the symbol. |
There was a problem hiding this comment.
nit: I'm having a bit of trouble understanding this one, could you re-phrase?
| case "watchos": self = .watchOS | ||
| case "tvos": self = .tvOS | ||
| case "visionos": self = .visionOS | ||
| default: return nil |
There was a problem hiding this comment.
Should we warn if there is a platform in the symbol graph that we don't know about?
There was a problem hiding this comment.
I don't think so. Similar to this question there can be unknown platforms but AFAIC the old platform didn't care about them. (We also have lots of tests that don't specify a platform for the symbol graph.)
| patch = .init(clamping: version.patch) | ||
| } | ||
|
|
||
| /// |
|
@swift-ci please test |
|
The integration can be merged after this. In the mean time, this will be "dead" / unused code. |
Bug/issue #, if applicable: rdar://172280267
Summary
This PR adds an alternate implementation to DocC's availability logic that's consolidated in one place. This type isn't integrated yet, so it is effectively dead code.
Matching behavior
The new implementation tries quite hard to preserve all behaviors of the current implementation, even when those behaviors are inconsistent or are otherwise bugs. The two exceptions to this are (from AvailabilityTests.swift):
// FIXME: Some availability logic only happens when symbols have declarations (rdar://172280267)which were too quirky and inconsistent for me to be able to replicate their fully behaviors.
However, nothing changes until we integrate this new implementation.
No integration yet
With the goal of making this PR smaller and easier to review I haven't included the changes that integrate this new consolidated availability implementation throughout DocC.
However, there's also a risk that by omitting the integration, reviewers may have a harder time seeing the bigger picture.
Let me know if you prefer that I push the integration changes to this PR or to a follow-up PR.
Dependencies
None.
Testing
Nothing in particular unless we prefer to rescope this PR to also include the integration.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded