SwiftDocOrg / CommonMark

Create, parse, and render Markdown text according to the CommonMark specification
MIT License
179 stars 10 forks source link

Simplify containers to use only a single container protocol. #14

Closed Lukas-Stuehrk closed 4 years ago

Lukas-Stuehrk commented 4 years ago

Implements #11

Lukas-Stuehrk commented 4 years ago

I don't think that I will make it work with Swift 5.1. It works with Swift 5.2.

On my local, the Swift 5.1 toolchain (macOS) can't even compile the (valid!) source because of a Swift compiler bug, so I have a hard time finding a good approach to fix the errors that happen on CI:

Assertion failed: (hasSelfMetadataParam() && "This method can only be called if the " "SILFunction has a self-metadata parameter"), function getSelfMetadataArgument, file /Users/buildnode/jenkins/workspace/oss-swift-5.1-package-osx/swift/include/swift/SIL/SILFunction.h, line 954.

I see a couple of options that we have now:

  1. Don't proceed with this approach at all and we close this pull request.
  2. Increase the minimum Swift version to 5.2. swift-doc is already on Swift 5.2, so it doesn't screw it up. However, SwiftMarkup is still on 5.1.
  3. We wait until CommonMark is on Swift 5.2 and then we merge this PR.

Let me know what you think, @mattt. Either is fine for me.

mattt commented 4 years ago

@Lukas-Stuehrk I'm sorry that this isn't working. Though, I must admit to feeling a small bit of validation about my hacky implementation (I knew there was a reason for all of that copy-paste!)

But I totally agree that this is a superior approach, and it should be the way things are done going forward. In terms of release planning, how about we schedule this and #13 for a 1.0.0 release targeting Swift 5.2? Those two PRs feel like a major milestone on their own, and making this a new major version mitigates any inconvenience for downstream adoption. Sound like a plan?

Lukas-Stuehrk commented 4 years ago

Sounds like a plan to me!

If this is already the 1.0.0 release (exciting!), then I'd suggest that we also consider adding #15 and #16 to the release. In my point of view, they add the missing functionality which completes a 1.0.

mattt commented 4 years ago

@Lukas-Stuehrk Agreed on all counts. In the interest of avoiding a separate development branch for 1.0 and keeping master green, let's update the requirements to Swift 5.2 in this PR. I've started to add a changelog with #17, and we can add entries for each of the changes there as we go along.

regexident commented 4 years ago

Until we reach 1.0.0 we're good to do breaking changes if necessary. 0.x.y updates are exempt from the semver rules. So we can make the necessary changes one by one without stashing them on a develop branch, no?

mattt commented 4 years ago

@regexident It is true that SemVer permits any changes to the public API before version 1.0.0, but that only contains how subsequent releases are numbered; branching strategies between releases is a separate concern. So long as master stays ✅, I have no objections to working towards the next version there, rather than a separate branch.

regexident commented 4 years ago

SR-12665:

The following code is compiled successfully on macOS, but the compiler crashes on Linux. The code

class C {
  class var i: Int { 0 }
  required init(_: Int) {}
}

protocol P where Self: C {
  init(n: Int)
}

extension P {
  init(n: Int) {
    self.init(type(of: self).i + n)

    // The compiler doesn't crash when you fix it like below:
    //   self.init(Self.i + n)
  }
}
Lukas-Stuehrk commented 4 years ago

I pushed my progress so far. The library itself is compiling, both on macOS and on Linux. There's one crash of the Swift compiler left on Linux when trying to build the tests. The culprits are all the insertions in the manipulation tests, e.g. https://github.com/SwiftDocOrg/CommonMark/blob/master/Tests/CommonMarkTests/ContainerManipulationTests.swift#L22

Lukas-Stuehrk commented 4 years ago

@mattt I tried every possible mitigation which I could think of. I will not make this work on Linux (yet). To unblock things: How about we drop this idea for now and unblock other pull requests? And then I'll have a look again once Swift 5.3 has landed. It still feels right to do it.

mattt commented 4 years ago

@Lukas-Stuehrk I'm really sorry to hear that didn't pan out. Thank you so much for digging into this. I learned a lot from looking through your code and discussion. Let's put a pin in this for now and reassess later this year, when Swift 5.3 is available.

regexident commented 4 years ago

Looking forward to giving this another go with 5.3! I guess this unblocks #13 then?

mattt commented 4 years ago

@regexident Indeed it does! I can take another look at everything soon. Anything left to do there?

regexident commented 4 years ago

No, should be fine.