Skip to content

Conversation

@franklinsch
Copy link
Contributor

Bug/issue #, if applicable: rdar://91545038

Summary

Resolves an issue when using ConvertService where link resolution fails in doc comments of top-level symbols. For example, in:

/// Link to ``bar()``.
public struct Foo {
  public func bar() {}
}

The link to bar() does not resolve.

The issue started appearing with #74, where DocC started (correctly) percent-encoding : characters in symbol links. The reason is that when ReferenceResolver resolves symbol links, it rewrites them with their full absolute link: ``Foo/bar()`` becomes ``doc://org.swift.example/documentation/Foo/bar()``.

When resolving the link again (in RenderNodeTranslator) during a compilation build (not ConvertService), the URL of the symbol link is looked up in DocumentationContext.referencesIndex, which contains an absolute URL -> resolved reference mapping. This mapping is populated during bundle registration.

However, when using ConvertService, externally-resolved references that are members of the bundle being compiled were not getting added to the referencesIndex mapping. This meant that the reference was getting resolved externally again rather than being looked up in the index.

Then, with #74, ``doc://org.swift.example/documentation/Foo/bar() `` (correctly) became a URL with path doc://org.swift.example/documentation/Foo/bar() rather than a URL with path /documentation/Foo/bar(). Hence, the resolution request being made was for doc://org.swift.example + the path, i.e., doc://org.swift.example/doc://org.swift.example/documentation/Foo/bar(), which is invalid. This was never an issue for regular docc-based compilations, because the reference was looked up in DocumentationContext.referencesIndex and never re-resolved.

This PR aligns the ConvertService and docc-based compilations by registering references resolved by fallback resolvers in the documentation context, so that they can be looked up for subsequent resolutions.

Dependencies

None.

Testing

Build documentation for the Swift example shown above using ConvertService and verify that the link resolves.

Checklist

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@franklinsch
Copy link
Contributor Author

@swift-ci please test

When resolving a link via a fallback resolver (i.e., a resolver that
resolves additional content for the bundle's identifier), register the
reference in the documentation context so that it can be looked up via
its absolute URL string. This is important for subsequent link
resolution requests, in order for the reference to be looked up in the
context rather than getting resolved again.

rdar://91545038
@franklinsch franklinsch force-pushed the convert-service-child-references branch from 1812a24 to 6cb8142 Compare April 11, 2022 14:04
@franklinsch
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

As a follow up it would be nice to rename registerReference(_:) and referenceIndex to be more specific. These have broad sounding names but are used for a more narrow purpose (only for local symbol references from what I could tell).

@jackhl
Copy link
Contributor

jackhl commented Apr 12, 2022

I'm not entirely following what's happening here, but this is concerning:

when ReferenceResolver resolves symbol links, it rewrites them with their full absolute link: Foo/bar() becomes doc://org.swift.example/documentation/Foo/bar().

Using a fully-qualified url including the scheme is invalid within double-backtick symbol links; only paths should be allowed there.

@jackhl
Copy link
Contributor

jackhl commented Apr 12, 2022

The issue started appearing with #74, where DocC started (correctly) percent-encoding : characters in symbol links.

I don't see where percent-encoding is happening that PR. In any case, why is it correct to percent-encode symbol links?

@franklinsch
Copy link
Contributor Author

Yes, having a full URL with a host is odd to have in a symbol link. DocC is correctly percent-encoding the symbol link destination so that's it's a valid URL path. This stems from ReferenceResolver rewriting the SymbolLink markup to include the fully-qualified resolved URL, but it should be writing as a plain Link instead. This is an improvement that we should do in a follow-up PR, but this PR just aligns the ConvertService and docc-based compilations for now. I've been trying to file a bug for this on bugs.swift.org but it seems like there are ongoing issues with JIRA at the moment.

@franklinsch
Copy link
Contributor Author

@swift-ci please test

@franklinsch franklinsch merged commit 90588d4 into swiftlang:main Apr 12, 2022
@franklinsch franklinsch deleted the convert-service-child-references branch April 12, 2022 10:25
franklinsch added a commit to franklinsch/swift-docc that referenced this pull request Apr 12, 2022
When resolving a link via a fallback resolver (i.e., a resolver that
resolves additional content for the bundle's identifier), register the
reference in the documentation context so that it can be looked up via
its absolute URL string. This is important for subsequent link
resolution requests, in order for the reference to be looked up in the
context rather than getting resolved again.

rdar://91545038
franklinsch added a commit to franklinsch/swift-docc that referenced this pull request Apr 13, 2022
When resolving a link via a fallback resolver (i.e., a resolver that
resolves additional content for the bundle's identifier), register the
reference in the documentation context so that it can be looked up via
its absolute URL string. This is important for subsequent link
resolution requests, in order for the reference to be looked up in the
context rather than getting resolved again.

rdar://91545038
franklinsch added a commit that referenced this pull request Apr 13, 2022
When resolving a link via a fallback resolver (i.e., a resolver that
resolves additional content for the bundle's identifier), register the
reference in the documentation context so that it can be looked up via
its absolute URL string. This is important for subsequent link
resolution requests, in order for the reference to be looked up in the
context rather than getting resolved again.

rdar://91545038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants