Skip to content

Support resolving documentation links to content in other DocC archives #710

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

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Sep 21, 2023

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

Summary

This adds experimental support for resolving documentation links and symbol links to content in other DocC archives.

At a high level this external link support works in two steps:

  • While building the documentation for the first module, DocC emits two files, one that describe the link hierarchy and its possible disambiguations and one that contains minimal amount of content for every linkable page.
  • Later, while building the documentation for the second module, DocC is passed the documentation archive for the first module and reads these file and uses them to resolve links to content in the first archive.

Important

There is more work needed to support this end-to-end. If you run this now you'll notice that the external links are 404s in the browser unless you host both archives on the same server.

Dependencies

None

Testing

To test this you need a project with two documentable targets.

  • Build documentation for one of the targets and pass --enable-experimental-external-link-support to enable linking to this documentation.

  • Verify that the output directory contains a "linkable-entities.json" file and a "link-hierarchy.json" file.

  • In the other target, add links to symbols in the first target starting with a leading slash and the name of the first module, for example /FirstModule/MyClass/myFunction()

  • Build documentation for the other target and pass --dependency /path/to/first-target.doccarchive.

  • The link should resolve to display the documentation comment for myFunction() from the first target.

  • Change the external documentation link to introduce a typo. The external link should raise an error with a fix-it, same as it would for a local link.

  • Remove the "linkable-entities.json" a "link-hierarchy.json" files from the first archive and build documentation for the second target again.

  • There should be an error about missing files in the dependency.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

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

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist marked this pull request as ready for review September 26, 2023 23:31
@d-ronnqvist
Copy link
Contributor Author

I've tried to document how the link resolver gets encoded and decoded but I know that it's not as comprehensive as I would like it to be. Please let me know what pieces are most confusing and I'll try to add additional comments there.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

I realized that I forgot to call out one noteworthy thing about this implementation.

You may notice that this new external resolver doesn't conform to ExternalReferenceResolver or ExternalSymbolResolver and that this new external resolver doesn't return DocumentationNode values for its entities. From trying to use those protocols for this functionality—now and in the past—I've discovered that they cause significant problems, mostly because they return DocumentationNode values.

The issue with that is that DocumentationNode holds not-yet-rendered markup that needs to be rendered again in the local context. This means that links, formatting, and images in the original external content needs to be transformed into markup and be rendered again meaning that external references and external media need to be added to local context which introduce a risk for collisions between local content and external content. #468 goes into more detail about these issues.

Since those are public protocols it would be a breaking change to modify their requirements. Instead I opted to create concrete types that are internal only. With the exception of LinkResolver.dependencyArchives I've kept all the new API internal only so that we are free to iterate on it without public breaking changes.

In the future we should deprecate OutOfProcessReferenceResolver, ExternalReferenceResolver, and ExternalSymbolResolver.

This also corrects how the declaration is saved in the linkable entity

rdar://116086280
Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

Looks great! I just had some question but no serious concerns. Looking forward to landing this. 🚀

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist force-pushed the path-hierarchy-serialization branch from 1b91be6 to 80a983c Compare October 5, 2023 01:10
The external resolver can't diagnose a mismatch in the module name
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

identifierMap[identifier] = index
}

let nodes = [Node](unsafeUninitializedCapacity: lookup.count) { buffer, initializedCount in
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry – I thought I asked this on the first review but it looks like I didn't. Is there a need for the unsafe initializer here? Or should we consider initially landing without it and then adopting it if there's a noticeable performance gain?

(I'm not familiar with the risk/reward of this particular initializer – the unsafe identifier is just raising alarm bells for me.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This initializer is described in SE-0245 and was added in Swift 5.1. It allows the caller to initialize an array of a known size and initialize the elements out of order by writing into the array's buffer. The alternatives involve extra allocations and extra copying of data.

Even if each node is small, the list is long (one node for each page, landmark, and anchor). This initialization is still probably just a rounding error in the scope of the overall build but this API was added for specifically this use case (initializing an arrays elements out of order).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Since this is the exact intent of this API – I think it makes sense to use it here. Thank you for the explanation! It might be nice to add a comment calling that out since we don't have a lot of unsafe API usage in the codebase generally, but non-blocking.

Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

🚀

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.

2 participants