Skip to content
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

Prevent a manual technology root from overriding the module node with the same name #770

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Dec 13, 2023

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

Summary

Fix a crash where if a developer added a technology root with the same file name as a module, the technology root would replace the module node and effectively drop all the symbols in the project.

Dependencies

None.

Testing

In any project with documentation for a module; add a manual @TechnologyRoot and name the file the same as the module name (with a ".md" extension).

Build documentation for the project. DocC shouldn't crash but may still run into #593

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

@sofiaromorales
Copy link
Member

@d-ronnqvist could you complete the Testing section of the PR description with the expected behaviour ?

# Conflicts:
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Find.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@d-ronnqvist could you complete the Testing section of the PR description with the expected behaviour ?

Added

@@ -352,7 +352,7 @@ struct PathHierarchy {
newNode.identifier = newReference
self.lookup[newReference] = newNode

modules[name] = newNode
modules.append(newNode)
Copy link
Member

Choose a reason for hiding this comment

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

Hi @d-ronnqvist. It seems that this is what fixes the bug, and the rest is refactoring as a consequence of changing modules from a dictionary to an array.

However, I'm confused about some other changes that don't appear to have any impact on resolving the bug. I'll add a comment to each of the things I believe are unnecessary in this PR, so you can explain me why these changes were made.

@@ -174,7 +174,7 @@ extension PathHierarchyBasedLinkResolver {
}

return SerializableLinkResolutionInformation(
version: .init(major: 0, minor: 0, patch: 1), // This is still in development
version: .init(major: 0, minor: 1, patch: 0), // This is still in development
Copy link
Member

Choose a reason for hiding this comment

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

How this is related to the crash?

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 is a consequence of changing the modules from a dictionary to an array which also changed the file format of the serialized path hierarchy. Since that file format change isn't backwards compatible I bumped the version number.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 18561d4 into apple:main Dec 20, 2023
2 checks passed
@d-ronnqvist d-ronnqvist deleted the technology-root-overriding-module branch December 20, 2023 18:16
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.

None yet

2 participants