Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Build documentation for extensions on external types. #230

Merged
merged 10 commits into from
Apr 23, 2021

Conversation

Lukas-Stuehrk
Copy link
Member

With this change, swift-doc also generates documentation for symbols declared in extensions on external types. This implements #122.

On the homepage, every external type is listed (in a visually very uninspired way) in a new section "Extensions". The following screenshots shows extensions on two different classes of UIKit. One time the external type is referenced with its full name, including the module. One time the external type is referenced only with its class name.
Screenshot 2021-03-27 at 23 21 58

For every external type, a new page is generated and lists the symbols, similar to the already existing pages for types.
Screenshot 2021-03-27 at 23 21 08

Not changed is the behavior for structs and classes which are declared in extensions on external types. Swift-doc already creates documentation for these types.

Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, @Lukas-Stuehrk.

I found an issue with the current behavior when I was testing this locally. See my comments in Subcommands/Generate.swift.

In other news, I added a commit that improves how the extensions are styled.

Screen Shot 2021-03-29 at 06 06 56

@mattt mattt force-pushed the PublicExtensions branch from 4f3b54a to 89f5e83 Compare April 20, 2021 12:47
@mattt
Copy link
Contributor

mattt commented Apr 20, 2021

@Lukas-Stuehrk I'm interested to hear what you think of my changes in 89f5e83.

Reading through your implementation of isExternalSymbol, I realized that we could generalize this functionality to resolve any qualified name through any typealiases that exist. Currently, we're only using it to determine whether a named symbol is external, in which case the equivalent to isExternalSymbol(name) is now symbols(named: name, resolvingTypealiases: true).isEmpty. Beyond that, I think this could be key to fixing some of the limitations we've had with our hand-rolled parser.

This has all of your original tests passing, with a few additional ones. How does it look for you?

@mattt mattt added this to the 1.0.0-rc1 milestone Apr 20, 2021
@Lukas-Stuehrk
Copy link
Member Author

To be transparent: There are still some shortcomings. One example:

import UIKit 
            
public class SomeClass {
    public struct InnerObject { }
   
    typealias ActuallyExternal = UIView
            
    typealias ActuallyInternal = InnerStruct 
    
    struct InnerStruct {}        
}
            
public class AnotherClass {
    typealias PublicClass = SomeClass            
}
            

In this example, the lookup for AnotherClass.PublicClass.ActuallyInternal does not work.

But nevertheless, I think we cover all of the most common cases, especially for libraries. And this is already a huge improvement. And for me it was really surprising how far we can get with a "cheap" and completely non-semantic symbol lookup.

And I fully agree, this could solve some of the existing problems in the symbol resolution and I like your generalization of the problem. But it also requires to think of what would be the expected result for many cases. One example again:

public class SomeClass {

}

public typealias AnotherClass = SomeClass

public extension AnotherClass {
    func methodOfExtension() {

    }
}

In this example, swift-doc does not include methodOfExtension() anywhere in the generated documentation (and I think fixing this should be outside the scope of this PR). But where should we include it? Simply add it to SomeClass? What was the intention of the author to express it like this? How do we reflect the intention in the generated documentation or do we even need to?

@mattt
Copy link
Contributor

mattt commented Apr 23, 2021

To be transparent: There are still some shortcomings. One example:

import UIKit 
            
public class SomeClass {
    public struct InnerObject { }
   
    typealias ActuallyExternal = UIView
            
    typealias ActuallyInternal = InnerStruct 
    
    struct InnerStruct {}        
}
            
public class AnotherClass {
    typealias PublicClass = SomeClass            
}
            

In this example, the lookup for AnotherClass.PublicClass.ActuallyInternal does not work.

Thanks for sharing that failing test case. I managed to add support for this in 0df8f0a. The basic idea is that, when we encounter a typealias in a key path, we resolve the alias to a matching symbol and then continue with the remaining components from that new reference point.

For example, consider the following:

enum A {
  typealias B = X
}

enum X { enum Y { enum Z { } } }

If we want to resolve "A.B.Y.Z", we walk to A, then A.B and encounter a typealias. The initialized type name "X" is resolved to the top-level X, so from here, the operation is equivalent to resolving "X.Y.Z".

But nevertheless, I think we cover all of the most common cases, especially for libraries. And this is already a huge improvement. And for me it was really surprising how far we can get with a "cheap" and completely non-semantic symbol lookup.

💯. I'm sure this could be more robust, but I'm happy with this for now if it solves our current problem.

And I fully agree, this could solve some of the existing problems in the symbol resolution and I like your generalization of the problem. But it also requires to think of what would be the expected result for many cases. One example again:

public class SomeClass {

}

public typealias AnotherClass = SomeClass

public extension AnotherClass {
    func methodOfExtension() {

    }
}

In this example, swift-doc does not include methodOfExtension() anywhere in the generated documentation (and I think fixing this should be outside the scope of this PR). But where should we include it? Simply add it to SomeClass? What was the intention of the author to express it like this? How do we reflect the intention in the generated documentation or do we even need to?

With 09b9720, this may be fixed. I ran out of time this morning to add a test for this, but I'll circle back to verify later today.

Update: It does!

Regardless, these are all great questions. I'll open a new issue to track this for a future release.

@mattt mattt force-pushed the PublicExtensions branch from bac46a1 to 09b9720 Compare April 23, 2021 13:24
@Lukas-Stuehrk
Copy link
Member Author

I started to write some tests for your changes and found yet another and arguably extreme test case which fails 🙈:

    public func testMembersOfTypealiasedSymbols() throws {
        let source = #"""
        public class SomeClass {
            public typealias IndirectSelfAlias = OtherClass
                
            public func someMethod() { }
        }
                
        public typealias OtherClass = SomeClass
                
        public extension OtherClass {
            func someExtensionMethod() { }
        }
                
        public typealias IndirectSomeClass = OtherClass.IndirectSelfAlias
                
        public extension IndirectSomeClass {
            func anotherExtensionMethod() { }
        }
        """#


        let url = try temporaryFile(contents: source)
        let sourceFile = try SourceFile(file: url, relativeTo: url.deletingLastPathComponent())
        let module = Module(name: "Module", sourceFiles: [sourceFile])

        XCTAssertEqual(module.interface.symbols.count, 7)

        let someClass = module.interface.symbols[0]
        XCTAssertEqual(someClass.name, "SomeClass")
        let members = module.interface.members(of: someClass)
        XCTAssertEqual(4, members.count)
        XCTAssertEqual(members[0].name, "IndirectSelfAlias")
        XCTAssertEqual(members[1].name, "someMethod()")
        XCTAssertEqual(members[2].name, "someExtensionMethod()")
        XCTAssertEqual(members[3].name, "anotherExtensionMethod()")
    }

But maybe we should call it a day and consider this a future problem. Then we could go with passing tests like:

    public func testMembersOfTypealiasedSymbols() throws {
        let source = #"""
        public class SomeClass {
            public func someMethod() { }
        }
                
        public typealias OtherClass = SomeClass
                
        public extension OtherClass {
            func someExtensionMethod() { }
        }
        """#


        let url = try temporaryFile(contents: source)
        let sourceFile = try SourceFile(file: url, relativeTo: url.deletingLastPathComponent())
        let module = Module(name: "Module", sourceFiles: [sourceFile])

        XCTAssertEqual(module.interface.symbols.count, 4)

        let someClass = module.interface.symbols[0]
        XCTAssertEqual(someClass.name, "SomeClass")
        let members = module.interface.members(of: someClass)
        XCTAssertEqual(2, members.count)
        XCTAssertEqual(members[0].name, "someMethod()")
        XCTAssertEqual(members[1].name, "someExtensionMethod()")
    }

@mattt
Copy link
Contributor

mattt commented Apr 23, 2021

@Lukas-Stuehrk 🙈

Agreed. Let's lock in our gains and ship this thing. Do you think this is ready to go in today's beta? Or would you like some extra time for it to bake?

@Lukas-Stuehrk
Copy link
Member Author

I think this is ready! And it's a nice improvement. I pushed the passing tests.

mattt added a commit to Lukas-Stuehrk/swift-doc that referenced this pull request Apr 23, 2021
mattt added a commit to Lukas-Stuehrk/swift-doc that referenced this pull request Apr 23, 2021
@mattt mattt force-pushed the PublicExtensions branch from aefc2ed to e62d777 Compare April 23, 2021 19:40
@mattt mattt merged commit b8a0cd9 into SwiftDocOrg:master Apr 23, 2021
@Lukas-Stuehrk Lukas-Stuehrk deleted the PublicExtensions branch April 23, 2021 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants