Fix bugs with symbol links to `/(_:_:)` operators #717

d-ronnqvist · 2023-09-26T20:02:52Z

Bug/issue #, if applicable: rdar://112555102 #714

Summary

This fixes two bugs where symbols with "/" in the name couldn't be linked to and where their data files in the output archive could collide with other pages's data files.

To avoid needing to escape slashes this fix relies some rules about

the allowed characters in module names in Swift, C, Objective-C
the allowed characters in Swift operators (C and Objective-C can't define custom operators).

These rules means that ``/Something`` can unambiguously be parsed as an absolute link to the "Something" module (excluding the slash from the path component) whereas ``/(_:_:)`` can unambiguously be parsed as a relative symbol link to the /(_:_:) operator (including the slash in the path component).

It also means that ``+/-(_:_:)`` can unambiguously be parsed as a single path component because operators need at least argument.

Dependencies

None

Testing

Define a public type with /(_:_:) operators, for example

public struct MyNumber: SignedNumeric, Comparable, Equatable, Hashable {
    public static func / (lhs: MyNumber, rhs: MyNumber) -> MyNumber { fatalError("This only needs to compile for a unit test") }
    public static func /= (lhs: inout MyNumber, rhs: MyNumber) -> MyNumber { fatalError("This only needs to compile for a unit test") }
    public static func *= (lhs: inout MyNumber, rhs: MyNumber) { fatalError("This only needs to compile for a unit test") }
    public var magnitude: Int.Magnitude
    public static func - (lhs: MyNumber, rhs: MyNumber) -> MyNumber { fatalError("This only needs to compile for a unit test") }
    public init(integerLiteral value: Int) { fatalError("This only needs to compile for a unit test") }
    public static func < (lhs: MyNumber, rhs: MyNumber) -> Bool { fatalError("This only needs to compile for a unit test") }
    public init?<T>(exactly source: T) where T : BinaryInteger { fatalError("This only needs to compile for a unit test") }
    public static func * (lhs: MyNumber, rhs: MyNumber) -> MyNumber { fatalError("This only needs to compile for a unit test") }
    public static func + (lhs: MyNumber, rhs: MyNumber) -> MyNumber { fatalError("This only needs to compile for a unit test") }
}

Link to ``MyNumber//(_:_:)`` and ``MyNumber//=(_:_:)`` from anywhere in the project
and to ``/(_:_:)`` and ``/=(_:_:)`` from MyNumber.
Build documentation for the project. Both links above should resolve successfully.
Inspect the "data" directory in the .doccarchive. Both division operators should have file names that start with "_". You can verify this by not seeing files named "(_:_:).json" and "=(_:_:).json" (as those files names would be if the leading slash was dropped from the file name)

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 · 2023-09-26T20:03:03Z

@swift-ci please test

rdar://112555102

# Conflicts: # Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift # Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

rdar://112555102

d-ronnqvist · 2023-11-16T16:46:53Z

There's no measurable performance impact from these changes

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Metric                                   │ Change          │ main                 │ current              │
├──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Duration for 'bundle-registration'       │ no change¹      │ 2.4 sec              │ 2.401 sec            │
│ Duration for 'convert-total-time'        │ no change²      │ 7.414 sec            │ 7.494 sec            │
│ Duration for 'documentation-processing'  │ no change³      │ 4.879 sec            │ 4.957 sec            │
│ Duration for 'finalize-navigation-index' │ no change⁴      │ 0.058 sec            │ 0.059 sec            │
│ Peak memory footprint                    │ no change⁵      │ 538.2 MB             │ 537 MB               │
│ Data subdirectory size                   │ no change       │ 168.5 MB             │ 168.5 MB             │
│ Index subdirectory size                  │ no change       │ 1.5 MB               │ 1.5 MB               │
│ Total DocC archive size                  │ no change       │ 223.7 MB             │ 223.7 MB             │
│ Topic Anchor Checksum                    │ no change       │ 9ca210cb9901eb38d157 │ 9ca210cb9901eb38d157 │
│ Topic Graph Checksum                     │ no change       │ 311f4d32005c9bd7dd72 │ 311f4d32005c9bd7dd72 │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘

 1: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : -0.170          degrees of freedom : 28       95% confidence critical value : +2.042

 2: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : -0.939          degrees of freedom : 28       95% confidence critical value : +2.042

 3: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : -0.894          degrees of freedom : 28       95% confidence critical value : +2.042

 4: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : -0.320          degrees of freedom : 28       95% confidence critical value : +2.042

 5: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : +0.946          degrees of freedom : 28       95% confidence critical value : +2.042

d-ronnqvist · 2023-11-16T16:55:16Z

@swift-ci please test

d-ronnqvist · 2023-11-17T07:25:32Z

@swift-ci please test

patshaughnessy

Great work. I love the use of Substring slices my guess is this will be faster than what we had before, in addition to fixing bugs.

patshaughnessy · 2023-12-12T15:26:04Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

    }

+    enum PathParser {


What's the advantage of this Swift idiom, which I've seen a few times now: enum EmptyEnum { } and then later extension EmptyEnum { ...functions defined here... }

Why not just create a normal class or struct and directly add functions to it?

Here I mainly did it to avoid the extra indentation. By using extension OuterType.InnerType { ... } the functions defined on InnerType are only indented once instead of twice. This mostly doesn't matter unless the API is added to a deeply nested type.

However, since I was moving API from PathHierarchy into PathHierarchy.PathParser while also modifying it, I wanted to preserve the unchanged lines in the diff which wouldn't have worked if the whole code block was intended more than it was before.

patshaughnessy · 2023-12-12T15:31:34Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+
+    mutating func scanPathComponent() -> Substring {
+        // If the next component is an operator, parse the full operator before splitting on "/" ("/" may appear in the operator name)
+        if remaining.unicodeScalars.prefix(3).allSatisfy(\.isValidOperatorHead) {


Where does the value 3 come from? Can you calculate this constant at compile time somehow? (I'm guessing it's the maximum length of the string literal cases in isValidOperatorHead?)

It's meant to be the shortest possible¹ operator name²; one operator head character followed by () for no arguments.

We could shorten it to just two characters because we only need to find if the next component isn't a module with a "/" prefix.

Footnotes

Having no arguments doesn't really make sense for a real operator since there's no value to operate on. Even ! has one argument. ↩

It's really too permissive for real operator names since the operator head supports more characters than the rest of the operator name does. ↩

Maybe add a comment to this effect (someday... not urgent) - or define a let constant with a descriptive name, so the number is less magical.

I'll probably revisit this piece of code for rdar://119616317

patshaughnessy · 2023-12-13T00:19:12Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+        let isAbsolute: Bool
+        if path.first == PathComponentScanner.separator {
+            guard let maybeModuleName = components.first?.dropFirst(), !maybeModuleName.isEmpty else {
+                return ([], true)


When would this line be reached? for a path that begins with // ?

This is reached if the path just a single /. If that path was // there would still be a non-empty string after dropping the leading slash.

patshaughnessy · 2023-12-13T00:21:08Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+                return ([], true)
+            }
+            isAbsolute = maybeModuleName.isValidModuleName()
+            assert(NodeURLGenerator.Path.documentationFolderName.isValidModuleName())


What do these assert calls achieve? Just a sanity check that the logic in isValidModuleName is not broken somehow?

It's more to check that we don't change NodeURLGenerator.Path.documentationFolderName and add some unexpected characters.

patshaughnessy · 2023-12-13T00:24:37Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+            guard let maybeModuleName = components.first?.dropFirst(), !maybeModuleName.isEmpty else {
+                return ([], true)
+            }
+            isAbsolute = maybeModuleName.isValidModuleName()


Does this rely on isValidModuleName distinguishing between operators and modules just based on the type of characters in the path component?

Yes, the path splitting will return ["/ModuleName"] if the path was /ModuleName and ["/(_:_:)"] if the path was /(_:_:). To distinguish between an absolute (starting with a leading slash) reference the module and a relative to the division operator we check if the rest of the path component is a valid module name.

patshaughnessy · 2023-12-13T00:26:09Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+        // If the next component is an operator, parse the full operator before splitting on "/" ("/" may appear in the operator name)
+        if remaining.unicodeScalars.prefix(3).allSatisfy(\.isValidOperatorHead) {
+            guard let operatorEndIndex = remaining.firstIndex(of: Self.operatorEnd) else {
+                defer { remaining.removeAll() }


This is very clever how you call different defer closures depending on the contents of the string and the logic here and how you update the remaining value after determining the return value (if I'm reading this correctly). Nice!

patshaughnessy · 2023-12-13T00:30:14Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift

+        switch value {
+        case
+            // A-Z
+            0x0041...0x005A,


Any way we can save these unicode values/characters into a config file elsewhere? That would simplify this source code file a lot.

Or would the overhead of opening that config file be too much?

In my opinion a separate config file would be more complicated and likely less readable.

The biggest complication would be that we'd have a resource that we need to ship alongside the docc executable. This is a fairly minor thing but it does mean that we need to update build-script-helper.py to copy this config file into the install directory and DocC needs to handle the cases when this config file is missing or not formatted correctly.

There's a tradeoff between readability and ease-of-parsing in a separate file. The easiest to parse would probably be to just write a stream of all the padded bytes for each character but that wouldn't be human readable at all. We could print them as characters which makes it easier to see what characters are supported but it also makes it look like entire Unicode code blocks are supported because most characters in that block are supported. For example, 98 characters in the "Greek" Unicode block are supported C99 identifiers (such as "Ϛ") but 12 characters aren't supported (such as "ϛ").

I feel that to make something that's more readable than in-source character ranges with comments we need to write a small one-off parser and tests for that parser

Yes you're right a runtime config file would be a bad idea for all the reasons you have here. Maybe just moving these large switch statements into a separate Swift file called something like PathHierarchy+ValidC99ExtendedIdentifier or whatever would remove all the verbose code from here.

The downside of moving it to a separate file is that it would need to be internal making it available everywhere in DocC whereas now it's only available in this file. That's a very small thing but I like to keep very specialized single use API private in the same file and only move it out if there's other places that need that functionality.

Maybe one day I'll find the time to break up SwiftDocC into multiple smaller modules and then each piece would have better control over what API it exposes to other pieces 🤞

daniel-grumberg · 2023-12-13T10:22:13Z

the allowed characters in Swift operators (C and Objective-C can't define custom operators).

This is not entirely true anymore as we lump C++/Objective-C++ together with Objective-C variants and those languages support operator overloading including the division operator.

d-ronnqvist · 2023-12-13T10:25:12Z

the allowed characters in Swift operators (C and Objective-C can't define custom operators).

This is not entirely true anymore as we lump C++/Objective-C++ together with Objective-C variants and those languages support operator overloading including the division operator.

Do you know what the allowed characters are for operators in C++/Objective-C++ operators? Are there characters that C++/Objective-C++ supports in operators that Swift doesn't?

daniel-grumberg · 2023-12-13T10:24:54Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+DisambiguatedPaths.swift

@@ -10,7 +10,7 @@

 import Foundation

-private let nonAllowedPathCharacters = CharacterSet.urlPathAllowed.inverted
+private let nonAllowedPathCharacters = CharacterSet.urlPathAllowed.inverted.union(["/"])


C++ supports overloading the following operators:
+ - * / % ^ & | ~ ! = < > += -= *= /= %= ^= &= |= << >> >>= <<= == != <= >= <=> && || ++ -- , ->* -> ( ) [ ]

Did you mean to comment on another line? This comment doesn't seem to be about this line.

Regardless, assuming that the names for those operators include at least (), all of them parse as expected with the current code in this PR.

I'm making that assumption because operator definitions in C++ has parameters, for example:

void operator ++ () { ... } MyNumber operator + (const MyNumber& obj) { ... } MyNumber operator - () { ... } MyNumber operator - (const MyNumber& obj) { ... }

so I would expect the parameters to be included in the symbol name just like they'd be for parameters to functions.

Daniel and I talked about this some more offline and agreed to make further changes for C++ operators in a follow up PR. I filed rdar://119616317 to track doing this.

d-ronnqvist · 2023-12-13T17:53:47Z

@swift-ci please test

* Support escaping forward slash in symbol links rdar://112555102 * Support operators with "/" without escaping rdar://112555102 * Remove need to pass original link string to create error info * Replace "/" in symbol names with "_" in resolved topic references

d-ronnqvist linked an issue Sep 26, 2023 that may be closed by this pull request

Can't reference forward-slash operator functions #714

Closed

2 tasks

d-ronnqvist mentioned this pull request Sep 26, 2023

Can't reference forward-slash operator functions #714

Closed

2 tasks

This comment was marked as outdated.

Sign in to view

Support escaping forward slash in symbol links

209112f

rdar://112555102

d-ronnqvist force-pushed the forward-slash-in-symbol-link branch from 22ea004 to 209112f Compare October 14, 2023 00:30

This comment was marked as resolved.

Sign in to view

d-ronnqvist marked this pull request as draft October 14, 2023 00:33

d-ronnqvist changed the title ~~Fix two bugs with symbol links to /(_:_:) and -(_:_:) operators~~ Fix bug with symbol links to /(_:_:) operators Oct 14, 2023

d-ronnqvist added 3 commits November 10, 2023 10:40

Merge branch 'main' into forward-slash-in-symbol-link

4166751

# Conflicts: # Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift # Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

Support operators with "/" without escaping

b147ec2

rdar://112555102

Remove need to pass original link string to create error info

b92549a

d-ronnqvist marked this pull request as ready for review November 16, 2023 16:54

Replace "/" in symbol names with "_" in resolved topic references

a80f56f

d-ronnqvist force-pushed the forward-slash-in-symbol-link branch from d6140ff to a80f56f Compare November 17, 2023 07:25

d-ronnqvist requested a review from daniel-grumberg November 17, 2023 09:46

Merge branch 'main' into forward-slash-in-symbol-link

9248ceb

d-ronnqvist requested review from patshaughnessy, sofiaromorales and mayaepps December 11, 2023 08:50

d-ronnqvist changed the title ~~Fix bug with symbol links to /(_:_:) operators~~ Fix bugs with symbol links to /(_:_:) operators Dec 11, 2023

Merge branch 'main' into forward-slash-in-symbol-link

1d3ac19

patshaughnessy approved these changes Dec 13, 2023

View reviewed changes

daniel-grumberg reviewed Dec 13, 2023

View reviewed changes

d-ronnqvist merged commit efcc793 into swiftlang:main Dec 13, 2023
2 checks passed

d-ronnqvist mentioned this pull request Dec 14, 2023

[5.10] Fix bugs with symbol links to /(_:_:) operators (#717) #772

Merged

d-ronnqvist mentioned this pull request Jan 22, 2024

Can't document - -= / /= operators #808

Closed

2 tasks

d-ronnqvist deleted the forward-slash-in-symbol-link branch February 19, 2024 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs with symbol links to `/(_:_:)` operators #717

Fix bugs with symbol links to `/(_:_:)` operators #717

d-ronnqvist commented Sep 26, 2023 •

edited

Loading

d-ronnqvist commented Sep 26, 2023

This comment was marked as outdated.

This comment was marked as resolved.

d-ronnqvist commented Nov 16, 2023

d-ronnqvist commented Nov 16, 2023

d-ronnqvist commented Nov 17, 2023

patshaughnessy left a comment

patshaughnessy Dec 12, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 12, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

patshaughnessy Dec 13, 2023

d-ronnqvist Dec 13, 2023

d-ronnqvist Dec 13, 2023

daniel-grumberg commented Dec 13, 2023

d-ronnqvist commented Dec 13, 2023

daniel-grumberg Dec 13, 2023

d-ronnqvist Dec 13, 2023

d-ronnqvist Dec 13, 2023

d-ronnqvist Dec 13, 2023

d-ronnqvist commented Dec 13, 2023

Fix bugs with symbol links to /(_:_:) operators #717

Fix bugs with symbol links to /(_:_:) operators #717

Conversation

d-ronnqvist commented Sep 26, 2023 • edited Loading

Summary

Dependencies

Testing

Checklist

d-ronnqvist commented Sep 26, 2023

This comment was marked as outdated.

This comment was marked as resolved.

d-ronnqvist commented Nov 16, 2023

d-ronnqvist commented Nov 16, 2023

d-ronnqvist commented Nov 17, 2023

patshaughnessy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-grumberg commented Dec 13, 2023

d-ronnqvist commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist commented Dec 13, 2023

Fix bugs with symbol links to `/(_:_:)` operators #717

Fix bugs with symbol links to `/(_:_:)` operators #717

d-ronnqvist commented Sep 26, 2023 •

edited

Loading