FixItApplier crash when generating diagnostics for whole tree but applying them to a subtree

[MahdiBM]

Description

The crash:

Steps to Reproduce

Note that @Enumerator() does not modify the code at all in this case. I'm just sharing the exact code otherwise I think this should be reproducible not that hard. Late night here, if I were to wait till tomorrow I'm pretty sure I would have ended up not reporting this bug, let me know if reproducing this doesn't end up being easy.

The exact code:

func testAppliesFixIts() {
    let unterminatedString = """
    let unterminated = ℹ️"This is unterminated1️⃣
    """
    assertMacroExpansion(
        #"""
        @Enumerator("""
        \#(unterminatedString)
        """)
        enum TestEnum {
            case a(val1: String, Int)
            case b
            case testCase(testValue: String)
        }
        """#,
        expandedSource: #"""
        enum TestEnum {
            case a(val1: String, Int)
            case b
            case testCase(testValue: String)

            let unterminated = "This is unterminated"
        }
        """#,
        macros: EnumeratorMacroEntryPoint.macros
    )
}

How FixIts are applied in the pipeline of the macro:

var statement = statement
var diagnostics = ParseDiagnosticsGenerator.diagnostics(for: statement)

/// Returns if anything changed at all.
func tryApplyFixIts() -> Bool {
    guard diagnostics.contains(where: { !$0.fixIts.isEmpty }) else {
        return false
    }
    let fixedStatement = FixItApplier.applyFixes(
        from: diagnostics,
        filterByMessages: nil,
        to: statement
    )
    var parser = Parser(fixedStatement)
    let newStatement = CodeBlockItemSyntax.parse(from: &parser)
    guard statement != newStatement else {
        return false
    }
    let placeholderDetector = PlaceholderDetector()
    placeholderDetector.walk(newStatement)
    /// One of the FixIts added a placeholder, so the fixes are unacceptable
    /// Known behavior which is fine for now: even if one FixIt is
    /// misbehaving, still none of the FixIts will be applied.
    if placeholderDetector.containedPlaceholder {
        return false
    } else {
        statement = newStatement
        return true
    }
}

final class PlaceholderDetector: SyntaxVisitor {
    var containedPlaceholder = false

    override init(viewMode: SyntaxTreeViewMode = .sourceAccurate) {
        super.init(viewMode: viewMode)
    }

    override func visit(_ node: TokenSyntax) -> SyntaxVisitorContinueKind {
        if node.isEditorPlaceholder {
            self.containedPlaceholder = true
            return .skipChildren
        }
        return .visitChildren
    }
}

Edit: Added PlaceholderDetector for the sake of being complete, although it doesn't matter.

[ahoppen]

Synced to Apple’s issue tracker as rdar://132355564

[ahoppen]

If you could share an implementation of the Enumerator macro that reproduce the issue, that would greatly simplify the issue’s reproduction.

[MahdiBM]

The current main branch with the test that I mentioned in the issue should work out to reproduce the issue.

I'll try to take another look at this in the next few days if you're not in a rush to get this fixed, though.

[MahdiBM]

This repository btw 😅

https://github.com/MahdiBM/enumerator-macro

[ahoppen]

The issue is that ParseDiagnosticsGenerator.diagnostics(for:) generates diagnostics with edits that are relative to the entire source file (instead of the statement’s start) and FixItApplier will apply these edits based on offsets to the statement only, which leads to incompatible string offsets.

There are multiple ways to fix this at its root cause:

• We need to define what it even means to get diagnostics for a subtree
• FixItApplier should maybe verify that the Fix-Its actually apply to the current tree and do edit adjustments as necessary.

I’d like to keep the issue open for that but you should be able to fix your crash by detaching the statement before generating diagnostics

ParseDiagnosticsGenerator.diagnostics(for: statement.detached)

---

Also unrelated, I saw that you’re checking compiler(>=6.0) to check if FixItApplier is available. But that doesn’t have anything to do with the compiler that you’re using but with the version of swift-syntax that you are depending on. The correct check would be #if canImport(SwiftSyntax600). You should be able to also update your package manifest to allow both swift-syntax 510 and 600, independent of the compiler being used. See https://swiftpackageindex.com/swiftlang/swift-syntax/main/documentation/swiftsyntax/macro-versioning for more information.

[MahdiBM]

canImport(SwiftSyntax600) doesn't seem to work because with "510.0.0" ..< "610.0.0" in Package.swift, even Swift 6 beta would pull 510 which i assume is because 600.0.0 is still in beta and SwiftPM is ignoring the beta versions in "510.0.0" ..< "610.0.0".

One reason that I'm to passing all diagnostics to FixItApplier is because i assumed it'd take care of this exact situation. Otherwise passing the diagnostics once by one would have been better since i could slowly but surely confirm that what FixItApplier does, isn't messing up the generated code (e.g. can't contain placeholders).

I think this will be a nice feature, generally. Even the current Xcode behavior is that when you apply one FixIt which moves lines around, the second possible FixIt won't notice that lines have moved and it'll modify the wrong lines (if you don't build again to get fresh diagnostics). I can see it'll be a bit more complicated to get it working for this Xcode situation, but it should be possible. I'm not sure if Xcode is already using these, i'd assume not considering FixItApplier looks new to SwiftSyntax 6.

[MahdiBM]

Soooo.... canImport(SwiftSyntax600) looks like a wrong check in the end?! or something is going unexpectedly wrong here. Because it's not even checking the SwiftSyntax version apparently.

I'm also aware of existence of canImport(SwiftSyntax, _version: 600.0.0) but that also doesn't work out and results in warnings that the declaration can't find the version of SwiftSyntax and is ignoring the condition.

[ahoppen]

With the "510.0.0" ..< "610.0.0" check, you aren’t opting in to prereleases. You should be doing using "510.0.0"..<"601.0.0-latest" if you also want to be able to resolve to swift-syntax 600 prereleases.

And the reason that canImport(SwiftSyntax600) was not true was probably because SwiftPM did not check out the swift-syntax 600 prerelease.

[MahdiBM]

the canImport was actually true. Even though SwiftPM pulled 510, not 6xx.

[ahoppen]

Oh, that was probably because of a stale build directory then that still contained the module for SwiftSyntax600. Cleaning should make it evaluate to false again.

[MahdiBM]

Tested it now, and you're right, just an inconsistency.