swiftdocorg / commonmark Goto Github PK

Hey @mattt!

What do you think about adding support for NSAttributedString?

Seem to be running into an issue when adding a header to a list "* #" or quote "> #", then checking the range for that header:

Fatal error: Can't form Range with upperBound < lowerBound

I've added a test project, to show it happening
test.zip

Hi!

I'm looking into some memory leaks coming from cmark and was wondering if there was a simple explanation as to why CommonMark doesn't directly target commonmark/cmark?

I have no reason to believe updating will fix some of these issues (yet?), but given that apple/swift-cmark is 100+ commits behind (and 20 ahead) I figured it was worth asking.

Thanks!
Gonzalo

The implementation of Document.init(_:options:) currently passes 0 (as in "no options") as third argument of cmark_parse_document(), instead of options.rawValue, effectively ignoring the options.

We fixed a memory leak that existed for a long time, when we merged #22. Unfortunately, this introduced a new issue: When you create a document, we now mark it as managed. When you then reference a child node of the document and no longer keep a reference to the document itself, we will free the node for the document: https://github.com/SwiftDocOrg/CommonMark/blob/main/Sources/CommonMark/Nodes/Node.swift#L35 But this also frees all the child nodes and we suddenly have a use after free when we still have a reference to any child.

Minimal test case to reproduce:

    func testCrashingChild() throws {
        var heading: Heading?
        try autoreleasepool {
            var document: Document? = try Document(Fixtures.udhr)
            heading = document!.children.first! as? Heading
            document = nil
        }

        // Will crash because of use after free.
        XCTAssertEqual("# [Universal Declaration of Human Rights][udhr]", heading?.render(format: .commonmark))
    }

Before, this was not happening, because our document node was always leaking, therefore "preventing" this crash 😄.

I'd say that this is a very common use case of this library. E.g. when you try to upgrade SwiftMarkup to use CommonMark version 0.5.0, then you will run into this crash.

I'd happy to provide a fix for this, but I think we should discuss the approach first. If I'm not mistaken, this is somehow a design flaw in the parent to child relation and a simple managed flag is not enough to represent the relationship.

Hi! I've noticed a memory leak when using CommonMark.Document "standalone" as root node, basically the first example that is given in the README file (just initialize a new Document with a commonmark string).
It seems that for this initializer the internal managed property would not be set to true, and then prevent the cmark nodes from being freed in deinit.
To fix that, I just added an extension that would set the managed property, sth. like this (I'm sure there are better ways...):

public extension Document {
  // Enforce managed flag to be set when the node is used "standalone", otherwise no freeing of cmark nodes will happen on deinit.
  class func root(_ commonmark: String, options: ParsingOptions = []) throws -> Document {
    let document = try Self.init(commonmark, options: options)
    document.managed = true
    return document
  }
}

Can you confirm this or am I missing sth. in terms of the API or intended usage?

Currently, there's no dedicated method to replace a node with another node.

One could already express a replacement with a combination of insert and remove, but I think it makes sense to also provide a wrapper around cmark's cmark_node_replace(). The same like we do for all the other operations on nodes.

I can provide a pull request once #11 is merged.

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.

Direct children of Documents don't have a parent node.

Minimal test case to reproduce the issue:

    func testChildrenShouldHaveParent() throws {
        let markdown = #######"""
        aaa

        bbb

        """#######

        let document = try Document(markdown)

        for child in document.children {
            XCTAssertNotNil(child.parent)
        }
    }

From my understanding, this is not the expected behavior. They should have the document as their parent node.

This is also the reason why most operations on a document are failing, e.g. insert(child:after:) or insert(child:before:), as those operations check for a parent node in cmark.

Cmark has a very misleading naming of its arguments for inserting nodes: The cmark_node_insert_after function takes a node and a sibling. https://github.com/SwiftDocOrg/swift-cmark/blob/master/src/node.c#L719

Unlike what you would expect, sibling is the node that is actually inserted. Unfortunately CommonMark uses the wrong order of arguments for all calls to cmark_node_insert_after, e.g. at https://github.com/SwiftDocOrg/CommonMark/blob/master/Sources/CommonMark/Supporting%20Types/Children.swift#L111

Currently, there's no way to render individual nodes if one doesn't want to render an entire document, but only parts of it. Of course one could import cmark and then use the underlying cmark functions to do so, but I think it makes sense to provide a convenient way to do it via CommonMark.

Therefore I suggest that we simply move the existing render(format:options:width) method from Document to Node. The implementation stays exactly the same.

Document is a Node, so it still provides the render method after the change and the straight-forward use case of the library does not change.

What do you think? Please let me know if this does not make sense at all or if you see better ways in implementing it. Or if I did not find the way to render individual nodes 😄.