Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethinking Diagnostic Order in Macro Expansion Assertions #2354

Open
Matejkob opened this issue Nov 13, 2023 · 2 comments
Open

Rethinking Diagnostic Order in Macro Expansion Assertions #2354

Matejkob opened this issue Nov 13, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@Matejkob
Copy link
Contributor

Description

I've been tinkering with macro expansions in Swift and hit an interesting crossroad. It's all about how we handle the order of DiagnosticSpec in assertMacroExpansion. I'm curious to hear your thoughts on this.

Right now, when we assert macro expansions, we're pretty strict about the order of expected diagnostics. Here’s a quick example to show you what I mean:

fileprivate struct TestMacro: DeclarationMacro {
  static func expansion(
    of node: some FreestandingMacroExpansionSyntax,
    in context: some MacroExpansionContext
  ) throws -> [DeclSyntax] {
    let firstErrorMessage = MacroExpansionErrorMessage("First message")
    context.diagnose(Diagnostic(node: node.macroName, message: firstErrorMessage))
    let secondErrorMessage = MacroExpansionErrorMessage("Second message")
    context.diagnose(Diagnostic(node: node.pound, message: secondErrorMessage))
    return []
  }
}

final class MacroDiagnosticsTests: XCTestCase {
  func test_1() {
    assertMacroExpansion(
      "#test",
      expandedSource: "",
      diagnostics: [
        DiagnosticSpec(message: "First message", line: 1, column: 2),
        DiagnosticSpec(message: "Second message", line: 1, column: 1)
      ],
      macros: ["test": TestMacro.self]
    )
  }

  func test_2() {
    assertMacroExpansion(
      "#test",
      expandedSource: "",
      diagnostics: [
        /// failed - message does not match
        /// –Second message
        /// +First message
        ///
        /// XCTAssertEqual failed: ("2") is not equal to ("1") - column does not match
        DiagnosticSpec(message: "Second message", line: 1, column: 1),
        /// failed - message does not match
        /// –First message
        /// +Second message
        ///
        /// XCTAssertEqual failed: ("2") is not equal to ("1") - column does not match
        DiagnosticSpec(message: "First message", line: 1, column: 2)
      ],
      macros: ["test": TestMacro.self]
    )
  }
}

This got me thinking – how much does the order really matter? I mean, would it be better if we tried to match the expected diagnostics to their corresponding emitted equivalents, even if it means complicating the assertion code a bit?

I'm torn between keeping things simple and going for more precision. What's your take? Do you think keeping a strict order is the way to go, or should we be a bit more flexible, especially in more complex codebases?

Would love to hear your experiences or any ideas you might have. Maybe there's an angle I haven't considered yet!

Looking forward to your thoughts!

@Matejkob Matejkob added the enhancement New feature or request label Nov 13, 2023
@ahoppen
Copy link
Collaborator

ahoppen commented Nov 13, 2023

Tracked in Apple’s issue tracker as rdar://118335358

@ahoppen
Copy link
Collaborator

ahoppen commented Nov 13, 2023

The order does not matter. The hard part is just trying to match the actual with the expected diagnostics without generating totally bogus output if e.g. the diagnostic message of a single diagnostic changes.

If you have any ideas of how to do that, I’m all ears. We just need to be very careful to not fall into the category of “this program tried to be a lot smarter than it actually is”. Ie. right now, it’s quite obvious what’s going on. If we try to be smart and essentially start writing a diff algorithm, things might turn out to be more confusing in the end than they are at the moment.

@Matejkob Matejkob changed the title [to discuss] Rethinking Diagnostic Order in Macro Expansion Assertions Rethinking Diagnostic Order in Macro Expansion Assertions Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants