Proposal: Simplify container protocols

[Lukas-Stuehrk]

There are two different protocols for container elements currently:

• ContainerOfBlocks
• ContainerOfInlineElements

Both protocols have extensions which implement exactly the same logic, except that the return and input types differ (Block & Node vs. Inline & Node).

On top of that, both List and List.Item have the same extensions, again with different types. So there are basically four times the same implementations. Fixing bugs or adding functionality needs to be replicated four times.

We could simplify this to use one single container protocol instead. The unified protocol would look like:

public protocol Container: Node {
    associatedtype ChildNode
}

The different container elements would declare conformance with an implementation like:

extension Document: Container {
    public typealias ChildNode = Block & Node
}

The methods in an extension of the protocol would then look like this example for insert(child:before:):

public func insert(child: ChildNode, before sibling: ChildNode) -> Bool {
    return add(child) { cmark_node_insert_before(sibling.cmark_node, child.cmark_node) }
}

What do you think? I can readily provide a pull request implementing the concept.

[mattt]

Thanks for thinking on this, @Lukas-Stuehrk. All of that repetition in the implementation is annoying and conspicuous, but it serves a specific purpose (at least I think it does).

When I first approached the problem, a Container<Child> seemed like an obvious choice. That's a textbook example of when to use generics in Swift. But I soon ran into the limitation that having an associated type requirement requires you to specify that generic type in declarations. For example, if Container has an associated Child type requirement, you can't declare a variable with type Container or use that as the return value for a function or property — you'd have to be explicit: Container<Block & Node> or Container<Inline & Node>.

IIRC, what I eventually found was that Container<Child> caused the associated type requirement to bubble all the way up to Node, which compromised my design goals for the library. Hence the ugly workaround — all of that copy-paste is for the benefit of convenience at the call site.

It could very well be that I missed something obvious in my first implementation, or wasn't clever enough to find a better solution (as you saw, managing child nodes wasn't my primary use case, and thus wasn't the focus of my efforts). If you found a solution that simplified the implementation without degrading API usability, I'd be glad to have it. But I just wanted to offer fair warning that I tried the same thing before, and wasn't able to make it work.

[Lukas-Stuehrk]

I did a quick prototype in the Container-Simplification branch in my fork.

The first commit 7101648a831a71ae2b84dbfa57f60f8838e13eec removes all protocols and comes with a single container protocol. Type safety is still given, so it's not possible to add a block as a child of a container that only supports inline elements.

If we still need to be able to have the ContainerOfBlocks and the ContainerOfInlineElements protocols, and to be able to use it as a type (so they should not have associated types or Self requirements), then 1d89b5028d4751e8d5c65a84a3916ad2d6db34f8 demonstrates that this is still possible.

All changes happen in Children.swift only, so none of the complexity bubbles up to any other type.

However, I also understand that this implementation might use a certain level of "cleverness" which you might not want to have in a codebase, as it might be not obvious enough for people who are not familiar with the codebase.

[mattt]

I stand corrected! 7101648 is exactly how this should be done. If you submit that in a PR, I'd be very happy to incorporate it for the next release.

As for 1d89b50, I'm having trouble wrapping my head around why the typealias declaration in an extension works here. Assuming we can do without ContainerOfBlocks and ContainerOfInlineElements (since now you can say Container where Child: Inline), I say let's get rid of them outright.

[Lukas-Stuehrk]

The typealias declaration in 1d89b5028d4751e8d5c65a84a3916ad2d6db34f8 works because I kind of cheated: ContainerOfBlock and ContainerOfInlineElements do not need to conform to Container.

As for 7101648a831a71ae2b84dbfa57f60f8838e13eec, I need to disclose one little problem: Please notice the ugly cast in line 60: https://github.com/SwiftDocOrg/CommonMark/commit/7101648a831a71ae2b84dbfa57f60f8838e13eec#diff-6ec4af82405deaa118bdbc9885526abbR60

I'd prefer to modify the existing approach and use Children(of: self).compactMap { $0 as? ChildNode } because this would only drop a single child if there was a problem with a wrong child, instead of dropping all children like my solution. But this approach always creates an empty array now (ironically only caught by the tests I added in #10). I'm 95% sure that this is a Swift bug, because if I debug it and execute the code in lldb everything works and all types are correct. But I also did not spent too much time investigating it as this was only a quick prototype.

However, this cast should always work as it's impossible to add the wrong child.

I will open a pull request for the simplification, but just wanted to be fully transparent with this problem and not sneak it in.

[Lukas-Stuehrk]

I managed to find a more elegant solution for the problem.

[mattt]

@Lukas-Stuehrk A fair amount of code has changed with #22 and more stands to change with #25, so this may no longer be a concern. If it is, please let me know and I'll reopen for further discussion. Thanks!