Vexil icon indicating copy to clipboard operation
Vexil copied to clipboard

Add Swift Macro Compatibility Check workflow

Open Matejkob opened this issue 1 year ago • 3 comments

This pull request introduces the Swift Macro Compatibility Check GitHub Action into the CI workflow. This action verifies that the Swift package remains compatible with multiple versions of swift-syntax, which is critical for macro-based functionality.

Motivation for Adding This Check:

  1. swift-syntax Versioning Complexity: As detailed by Point-Free in their article, "Being a good citizen in the land of SwiftSyntax", swift-syntax employs a versioning scheme where minor versions of Swift correspond to major versions of swift-syntax (e.g., SwiftSyntax 509.0 maps to Swift 5.9). This scheme introduces complexity in maintaining compatibility across versions.

  2. Frequent Breaking Changes: Minor releases of swift-syntax have introduced breaking changes, which can cause issues with existing macros and dependencies. This action helps ensure that our macros remain functional across these versions.

  3. Dependency Graph Resolution: By testing against multiple versions, we can prevent dependency conflicts in the broader Swift ecosystem. This is essential, especially as more libraries are adopting macros and using swift-syntax, increasing the likelihood of conflicting dependencies.

  4. Proactive Development: Catching compatibility issues early in the development cycle allows us to resolve them before they become critical, reducing the risk of blocked releases or difficult debugging sessions later.

[!IMPORTANT] To ensure the library can be used alongside others in the Swift packages, it need to support all versions of swift-syntax. Without this compatibility, users may face issues where they can't use several libraries together if they depend on different versions of swift-syntax.

✅ Checklist

  • [ ] I've added at least one test that validates that my change is working, if appropriate
  • [x] I've followed the code style of the rest of the project
  • [x] I've read the Contribution Guidelines
  • [ ] I've updated the documentation if necessary

Matejkob avatar Oct 05 '24 10:10 Matejkob

I agree in principle that Apple have shot themselves in the foot here, but I'm somewhat unconvinced by the current state of your lint:

  1. SwiftPM is clearly broken here; some solution such as allowing "host" products to use separate dependency graphs from "target" products would fix the problem at its source. Where is that evolution proposal / pitch thread / bug / discussion? Maybe the lint is required in the interim, but it's clearly not a solution.
  2. testing all of those swift-syntax versions seems excessive unless people are using "exact" dependencies. Only the highest "patch" version in each minor version should be necessary, even if breaking changes are being introduced in "minor" versions.
  3. the "major-versions-only" flag tests the earliest revision of each major version, where it should probably test the latest revision of each, since that's the one with most features... but as you point out, that doesn't really help the problem since they introduce breaking changes in minor versions. Not sure that flag makes any sense in that situation.
  4. your lint presupposes that every macro can be built with every swift-syntax version. That's unlikely to be true in general; most macros will have a minimum version corresponding to their MSSV.
  5. your lint presupposes that every macro can be built with every swift-syntax version, which corresponds to a kind of version requirement that can't be stated by Package.swift AFAICT? Like, .upToNextMajor(from: "509.0.0") doesn't actually include 510 or 600? How are you proposing the package's swift-syntax dependency be stated? And your lint should probably verify that it takes the correct form.

KeithBauerANZ avatar Oct 07 '24 03:10 KeithBauerANZ

Relevant to my questions, it seems like

  • the dependency on swiftsyntax should be declared as a range, eg. "509.1.0"..<"600.0.1"
    • we can use #if canImport(SwiftSyntax509) within the macro code to determine which version we're actually building against, what a hack <_<;;
  • this lint should respect the range in the Package.swift, and the only option should be which versions within the range to check (suggestion: all, latest_patch_in_each_minor, latest_minor_patch_in_each_major)
  • the lint should probably signal somehow if there is a (new, future) swiftsyntax available which is not included in this package's range, but it shouldn't ever test versions outside the stated range
  • the lint probably needs to defer to us to actually build the macros and run their tests on the supported platforms, IDK how it can hope to do that itself. It basically needs to act as a for loop over swift package resolve?

KeithBauerANZ avatar Oct 09 '24 04:10 KeithBauerANZ

I agree with the sentiment expressed here and believe better testing of swift-syntax compatibility is desirable. But based on the concerns raised above I don't believe this is the correct way to go about it. I'll close this one for now but feel free to comment if you disagree and we can pursue it further.

bok- avatar Dec 10 '24 12:12 bok-