-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add directive for linking symbols as alternate representations #1097
base: main
Are you sure you want to change the base?
Conversation
Before I get into the code, some feedback on this syntax:
|
Sources/SwiftDocC/Semantics/General Purpose Analyses/HasExactlyOne.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Semantics/Metadata/SourceLanguage+Directive.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Semantics/Metadata/SourceLanguage+Directive.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Semantics/Metadata/AlternativeDeclaration.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Semantics/Metadata/AlternativeDeclaration.swift
Outdated
Show resolved
Hide resolved
counterpartReference.sourceLanguages.forEach { | ||
allVariants[$0, default: []].append(counterpartReference) | ||
} |
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.
Has the duplicates/overlap already been filtered out? Otherwise this could result in multiple variants with the same language.
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.
I think that's safe, see comment; only the first symbol reference will be respected by Swift-DocC Render and the rest will be ignored --WDYT?
for alternateDeclaration in entity.metadata?.alternateDeclarations ?? [] { | ||
guard case .resolved(.success(let counterpartReference)) = alternateDeclaration.counterpart, | ||
let counterpartEntity = try? self.entity(with: counterpartReference) else { | ||
continue |
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.
If this is the only place that checks that the link resolved we should emit a warning with the right link source range so that the developer knows that the link failed to resolve.
unresolvedReferenceProblem(...)
can create the same type of diagnostic as for other unresolved links by passing the TopicReferenceResolutionErrorInfo
from the TopicReferenceResolutionResult.failure(_:_:)
.
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 will already emit a diagnostic in the code below so I don't think it's also needed here, WDYT?
swift-docc/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
Lines 614 to 621 in bc6df51
// Also resolve the node's alternate declaration. This isn't part of the node's 'semantic' value (resolved above). | |
documentationNode.metadata?.alternateDeclarations.forEach { synonym in | |
resolver.resolve( | |
synonym: synonym, | |
range: synonym.originalMarkup.range, | |
severity: .warning | |
) | |
} |
Since this ultimately ends up adding this diagnostic:
problems.append(unresolvedReferenceProblem(source: range?.source, range: range, severity: severity, uncuratedArticleMatch: uncuratedArticleMatch, errorInfo: error, fromSymbolLink: false)) |
(I can add a comment to explain why we don't need a diagnostic here)
2638a80
to
abe7160
Compare
The metadata directive was missing some of its child directives in its documentation. These have now been added to the Topics section, in alphabetical order. This ensures that they are curated underneath the `Metadata` symbol, particularly because some of these types are not defined inside `Metadata`.
Adds a new child directive to `@Metadata` which can be used in a symbol extension file by specifying the link to another symbol: ```swift @metadata { @AlternateRepresentation(``MyClass/property``) } ``` External links are also supported, as long as they're quoted: ```swift @metadata { @AlternateRepresentation("doc://com.example/documentation/MyClass/property") } ``` The intent of this directive is to define an alternate language representation for the current symbol, such that both symbols are considered to be alternate representations of the same symbol. Ideally two symbols which are equivalent would have the same USR and be resolved as the same symbol by the compiler, but this is not always the case. For the cases in which it is not feasible to change the source code to have them as one symbol, the `@AlternateRepresentation` directive can be used to manually link them as variants of each other. Discussion: ---------- A mutable topic reference type was chosen as the type for parsing&storing the link so that it can be parsed as an unresolved link at the directive parsing stage, and then later be updated to a resolved link at a later stage when the documentation context is resolving all links. A parsing failure diagnostic is returned if the link is invalid in any way: ``` AlternateRepresentation expects an argument for an unnamed parameter that's convertible to 'TopicReference' --> SynonymSample.docc/Symbol.md:4:31-4:37 2 | 3 | @metadata { 4 + @AlternateRepresentation("doc://") 5 | } 6 | ``` This commit adds the directive itself, but doesn't hook it up to other parts of SwiftDocC. Subsequent commits will add: - link resolution - rendering logic Alternatives considered: ----------------------- Considered other names such as `@Synonym`, `@Counterpart`, `@AlternativeDeclaration` and `@VariantOf`. In the end disqualified these as being confusing, and chose `@AlternateRepresentation` for being the one which strikes the best balance between readable and closeness to the technical term for this concept.
Adds logic in `DocumentationContext` which will resolve the references inside the alternate representation directive. The same logic is used as for resolving all other links. This is done outside the usual ReferenceResolver visit of the semantic object, because `Symbol` objects don't have access to the node metadata, where the unresolved link resides. If the link cannot be resolved, the usual diagnostic is emitted: ``` warning: 'MissingSymbol' doesn't exist at '/Synonyms' --> SynonymSample.docc/SymbolExtension2.md:4:19-4:32 2 | 3 | @metadata { 4 + @AlternateRepresentation(``Synonyms/MissingSymbol``) 5 | } ```
If an `@AlternateRepresentation` clashes with already available source languages, this will now be reported as diagnostics. These diagnostics are performed in the final stage of registering a bundle, during the global analysis of the topic graph, where all nodes are available and all links will have been resolved. This is so that we have all the information we need for detecting duplicates. The following cases are detected: - if the symbol the alternate representation is being defined for (the "original" symbol) was already available in one of the languages the counterpart symbol is available in - if the alternate representations have duplicate source languages in common, i.e. if counterpart1 is available in Objective-C and counterpart2 is **also** available in Objective-C. Suggestions will be provided depending on context: - which languages are duplicate - all the languages the symbol is already available in will be available as part of the diagnostic explanation - if the `@AlternateRepresentation` directive is a duplicate, a suggestion will be made to remove it, with a suitable replacement - if the `@AlternateRepresentation` directive is a duplicate, a note pointing to the original directive will be added Example diagnostics: ``` warning: An alternate representation for Swift already exists This node is already available in Swift and Objective-C. SynonymSample.docc/SymbolExtension2.md:4:5: An alternate representation for Swift has already been defined by an @AlternateRepresentation directive. --> SynonymSample.docc/SymbolExtension2.md:5:5-5:57 3 | @metadata { 4 | @AlternateRepresentation(``Synonyms/Synonym-5zxmc``) 5 + @AlternateRepresentation(``Synonyms/Synonym-5zxmc``) | ╰─suggestion: Remove this alternate representation 6 | } 7 | ``` ``` warning: This node already has a representation in Swift This node is already available in Swift. --> SynonymSample.docc/SynonymExtension.md:5:5-5:56 3 | @metadata { 4 | @AlternateRepresentation(``Synonyms/Synonym-1wqxt``) 5 + @AlternateRepresentation(``Synonyms/OtherSynonym``) | ╰─suggestion: Replace the counterpart link with a node which isn't available in Swift 6 | } 7 | ```
When rendering the variants of a node, use the topic references from the `@AlternateRepresentation` directives to populate more variants. There is no need to report diagnostics as they would have been reported during bundle registration. Link resolution would have already been performed at that point. Unresolved topic references are ignored, but all resolved references are added as variants. If there are multiple symbols per variant, Swift-DocC-Render prefers the first one that was added, which will always be the current node's symbol. There should be no breakage and change of behaviour for anyone not using `@AlternateRepresentation`, and the current symbol's variants will always be preferred over any other.
abe7160
to
14d85bf
Compare
@swift-ci Please test |
@Synonym
directive for linking symbolsThere 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.
The implementation generally looks good but the tests are unrealistic and don't cover a handful of behaviors that should raise diagnostics. Also, I think much of the user-facing documentation and other user-facing text could benefit from some refinements.
let finalElement = languageNames.removeLast() | ||
let commaSeparatedElements = languageNames.joined(separator: ", ") | ||
return "\(commaSeparatedElements) and \(finalElement)" |
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.
FYI: We have a .list(finalConjunction:)
extension to BidirectionalCollection
for this.
argumentValue = String(argumentValue.dropFirst(2).dropLast(2)) | ||
} | ||
guard let url = ValidatedURL(parsingAuthoredLink: argumentValue), !url.components.path.isEmpty else { | ||
return nil |
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.
Is there someplace that surfaces a user-facing diagnostic about this?
moduleName: "unit-test", | ||
symbols: [ | ||
makeSymbol(id: "symbol-id", kind: .class, pathComponents: ["Symbol"]), | ||
makeSymbol(id: "counterpart-symbol-id", language: .objectiveC, kind: .class, pathComponents: ["CounterpartSymbol"]), |
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 unrealistic test data because a single symbol graph file contains symbols of different languages. Use different symbol graphs for each language just like the real data output from the respective compilers would be emitted.
let tempURL = try createTempFolder(content: [exampleDocumentation]) | ||
let (_, bundle, context) = try loadBundle(from: tempURL) |
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.
minor: There's no need to write the bundle to the file system just to read it back
let tempURL = try createTempFolder(content: [exampleDocumentation]) | |
let (_, bundle, context) = try loadBundle(from: tempURL) | |
let (bundle, context) = try loadBundle(catalog: exampleDocumentation) |
""" | ||
} | ||
|
||
XCTAssertTrue(problems.isEmpty, "Found problems: \(problems.joined(separator: ", "))") |
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 "Found problems" text almost looks like that output is expected. I had to read and the full test assertion and think about it to understand that it's unexpected. Also, if this test fails, the output of printing the full problem structures won't be that readable. Many other tests print the diagnostic summaries like this instead:
XCTAssertTrue(problems.isEmpty, "Found problems: \(problems.joined(separator: ", "))") | |
XCTAssertTrue(problems.isEmpty, "Unexpected problems: \(problems.map(\.diagnostic.summary).joined(separator: "\n"))") |
summary: "This node already has a representation in \(duplicateSourceLanguages.diagnosticString)", | ||
explanation: "This node is already available in \(entity.availableSourceLanguages.diagnosticString).", | ||
), | ||
possibleSolutions: [Solution(summary: "Replace the counterpart link with a node which isn't available in \(entity.availableSourceLanguages.diagnosticString)", replacements: [])] |
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 mixing user-facing terminology using "counterpart" instead of "alternate representation". We should aim to be consistent in everything that's user-facing.
} | ||
|
||
return DiagnosticNote(source: source, range: range, message: """ | ||
An alternate representation for \(duplicateCounterpartLanguage.name) has already been defined by an @\(AlternateRepresentation.self) directive. |
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.
Question: what is this note about? Is this telling me that another directive in the same file already adds a representation with the same language? If so, maybe we could say "this" instead since the diagnostic would be associated with the right source location.
An alternate representation for \(duplicateCounterpartLanguage.name) has already been defined by an @\(AlternateRepresentation.self) directive. | |
This directive already specifies an alternate \(duplicateCounterpartLanguage.name) representation. |
continue | ||
} | ||
|
||
// Case where the original symbol already was defined in the languages of the counterpart 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.
// Case where the original symbol already was defined in the languages of the counterpart symbol. | |
// Check if the documented symbol already has alternate representations from in-source annotations. |
@@ -612,6 +612,17 @@ public class DocumentationContext { | |||
resolver.visit(documentationNode.semantic) | |||
} | |||
|
|||
// Also resolve the node's alternate representations. This isn't part of the node's 'semantic' value (resolved above). | |||
documentationNode.metadata?.alternateRepresentations.forEach { alternateRepresentation in |
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.
style: I'll just mention this once but I feel like forEach
is just a worse and less capable version of a regular for loop and there's practically no use for it ever.
problems | ||
.append( | ||
Problem( |
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.
style: I find this style to be a bit too much.
problems | |
.append( | |
Problem( | |
problems.append(Problem( |
Bug/issue #, if applicable: rdar://109417745
Summary
Adds a directive which is meant to be used in markdown extension files for a specific symbol:
Where all variants of
MyApp/MyClass/property
will be added as variants of the current symbol.Its purpose is to be able to define an alternative language representation for a symbol, where the alternative symbol is an unrelated symbol as far as the compiler is aware (i.e. they have different USRs in the symbol graph).
This makes it possible to switch between both symbols through the language switcher:
Whenever possible, the alternative language representations should be defined in-source, by using in-source annotations such as the
@objc
and@_objcImplementation
attributes in Swift, or theNS_SWIFT_NAME
macro in Objective C.However, if this is not possible, this is an alternative to be able to render both symbols as counterparts of each other.
Diagnostics
Links within the directive are resolved, and emit a warning if the link cannot be resolved same as all other markup links:
Duplication of source language availability is also detected:
And suggestions will be provided depending on context:
@AlternateRepresentation
directive is a duplicate, a suggestion will be made to remove it, with a suitable replacement@AlternateRepresentation
directive is a duplicate, a note pointing to the original directive will be addedDependencies
None.
Testing
Tested by building and rendering the following archive locally, which uses the new directive:
SynonymSample.docc.zip
This contains two symbols which have been configured to be counterparts of each other.
You should be able to switch between both symbols using the language picker from either:
http://localhost:8080/documentation/synonyms/synonym-1wqxt?language=objc
or
http://localhost:8080/documentation/synonyms/synonym-5zxmc
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded