Prepend `SyntaxStringInterpolationError` errors in macro expansion with `Internal macro error:` #2176

natikgadzhi · 2023-09-11T06:13:17Z

Summary

This pull request prepends SyntaxStringInterpolationError message with Internal macro error:. Closes #2039.

Changes

In MacroExpansionContext.addDiagnostics, if the passed error as? SyntaxStringInterpolationError worked, we prepend it with the Internal macro error and grab it's description. Otherwise, we just use String(describing:error) as the diagnostic message.
Adds a basic test for addDiagnostics for that behavior.

Next steps

@DougGregor, does this look like the right direction to you? I'm new to Macros, and I'm here to learn — no hard feelings if my attempt is naive / plain wrong.
@Matejkob, sounds like your area of interest! Could you give me some hints to how I could test this end to end, and construct a test macro that would throw syntax string interpolation error?

natikgadzhi · 2023-09-11T06:16:03Z

Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift

+// Used in `MacroExpansionContext` to make a nicer error message when a
+// macro expansion fails with SwiftSyntaxBuilder making a syntax node
+// with string interpolation.
+@_spi(Diagnostics) public enum SyntaxStringInterpolationError: Error, CustomStringConvertible {


Don't know if it's okay to @_spi this, but that seemed like a better option than to make it public outright.

I just checked and SyntaxStringInterpolationError currently serves two distinct use cases:

The .diagnostics case in SyntaxProtocol(validating:)

The .producedInvalidNodeType case in SyntaxNodeWithBody and DeclSyntaxProtocol.init

I think what we should do, is split the error into two distinct structs and make the one that represents the producedInvalidNodeType case public. That way, we minimize the public surface are we are introducing while also allowing other clients to catch that case.

Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift

Tests/SwiftSyntaxMacroExpansionTest/MacroExpansionContextTests.swift

ahoppen · 2023-09-11T18:23:04Z

Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift

+// Used in `MacroExpansionContext` to make a nicer error message when a
+// macro expansion fails with SwiftSyntaxBuilder making a syntax node
+// with string interpolation.
+@_spi(Diagnostics) public enum SyntaxStringInterpolationError: Error, CustomStringConvertible {


I just checked and SyntaxStringInterpolationError currently serves two distinct use cases:

The .diagnostics case in SyntaxProtocol(validating:)

The .producedInvalidNodeType case in SyntaxNodeWithBody and DeclSyntaxProtocol.init

I think what we should do, is split the error into two distinct structs and make the one that represents the producedInvalidNodeType case public. That way, we minimize the public surface are we are introducing while also allowing other clients to catch that case.

ahoppen · 2023-09-11T18:43:09Z

Tests/SwiftSyntaxMacroExpansionTest/MacroExpansionContextTests.swift

+    let context = BasicMacroExpansionContext()
+    let error = SyntaxStringInterpolationError.producedInvalidNodeType(expectedType: DeclSyntax.self, actualType: ExprSyntax.self)
+    // Since we only care about the error switch inside of addDagnostics, we don't care about the particular node we're passing in.
+    context.addDiagnostics(from: error, node: ExprSyntax("1"))
+
+    XCTAssertEqual(context.diagnostics.count, 1)
+    XCTAssertTrue(context.diagnostics.first!.message.starts(with: "Internal macro error:"))


I think you should be able to test the real-case scenario when this happens by defining an extension macro that returns

try ExtensionDeclSyntax("var x: Int")

And then use assertMacroExpansion to check that the emitted error contains Internal macro error:

@ahoppen

- This commit prepends `SyntaxStringInterpolationError` message with `Internal macro error:` when adding it as a diagnostic in `MacroExpansionContext.addDiagnostics`, so that macro expansion clarifies the error as internal to the macro, not to how it was applied. - Any unrecognized errors raised and sent to `addDiagnostics` will be added as is, without prepending them with a message. That's consistent with how it works now. - Adds a basic test for `addDiagnostics` for that behavior. - Adds end to end test with `assertMacroExpansion`, thanks @ahoppen for the hint!

natikgadzhi · 2023-09-11T22:03:51Z

@ahoppen, thank you for the review!

I've cleaned up the code and added an end-to-end test for now. The next part is splitting the error enum into two pieces. That'll be a separate commit.

natikgadzhi · 2023-09-11T22:30:36Z

@ahoppen ready for review!

ahoppen

A couple of minor comments, otherwise LGTM.

ahoppen · 2023-09-12T14:53:25Z

Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift

-      // Start the diagnostic on a new line so it isn't prefixed with the file, which messes up the
-      // column-aligned message from ``DiagnosticsFormatter``.


Could we keep this comment?

Yep, added that comment back <3

ahoppen · 2023-09-12T14:54:08Z

Sources/SwiftSyntaxMacros/MacroExpansionContext.swift

@@ -12,6 +12,7 @@

 import SwiftDiagnostics
 import SwiftSyntax
+@_spi(Diagnostics) import SwiftSyntaxBuilder


I don’t think we need @_spi(Diagnostics) anymore.

Good catch! Sorry I missed it.

ahoppen · 2023-09-12T14:55:36Z

Tests/SwiftSyntaxMacroExpansionTest/MacroExpansionContextTests.swift

+  }
+}
+
+final class MacroExpansionContextTests: XCTestCase {


I think a better name for this test would now be something like StringInterpolationErrorTests.

natikgadzhi · 2023-09-12T15:13:17Z

Oooops, those are embarrassing. I’ll clean it up.

natikgadzhi · 2023-09-14T22:55:43Z

@ahoppen done!

ahoppen

One minor comment, otherwise LGTM.

ahoppen · 2023-09-14T23:12:32Z

Tests/SwiftSyntaxMacroExpansionTest/StringInterpolationErrorTests.swift

+    // Since we only care about the error switch inside of addDagnostics, we don't care about the particular node we're passing in
+    context.addDiagnostics(from: error, node: ExprSyntax("1"))
+    XCTAssertEqual(context.diagnostics.count, 1)
+    XCTAssertTrue(context.diagnostics.first!.message.starts(with: "Internal macro error:"))


Can you do the following? That way the test doesn’t crash if there is no diagnostic.

Suggested change

XCTAssertTrue(context.diagnostics.first!.message.starts(with: "Internal macro error:"))

let diagnostic = try XCTUnwrap(context.diagnostics.first)

XCTAssertTrue(diagnostic.message.starts(with: "Internal macro error:"))

Same for the test below

Yep, good call. Fixed.

This commit splits `SyntaxStringInterpolationError` into two error types: - `SyntaxStringInterpolationInvalidTokenTypeError` for situations when interpolation resulted in a wrong token type, and - `SyntaxStringInterpolationDiagnosticError` when another error occured, and the user provides additional diagnostics with the error. Additionally: - The `SyntaxStringInterpolationInvalidTokenTypeError` is now `public` so other clients can catch it. - Both error types are structs instead of enums. - All invocation spots are updated to match the API.

ahoppen · 2023-09-15T21:30:07Z

@swift-ci Please test

kimdv · 2023-09-16T20:07:43Z

@swift-ci please test windows

kimdv · 2023-09-17T07:43:37Z

@swift-ci please test windows

kimdv · 2023-09-17T07:45:54Z

@swift-ci please test Windows

kimdv · 2023-09-17T14:59:02Z

@swift-ci please test Windows

natikgadzhi requested review from DougGregor and ahoppen as code owners September 11, 2023 06:13

natikgadzhi commented Sep 11, 2023

View reviewed changes

ahoppen reviewed Sep 11, 2023

View reviewed changes

natikgadzhi force-pushed the macro-expansion/syntax-interpolation-errors branch from 716ed0f to 34a634a Compare September 11, 2023 22:01

natikgadzhi force-pushed the macro-expansion/syntax-interpolation-errors branch from 34a634a to 0df36e7 Compare September 11, 2023 22:03

natikgadzhi requested a review from ahoppen September 11, 2023 22:30

ahoppen reviewed Sep 12, 2023

View reviewed changes

natikgadzhi force-pushed the macro-expansion/syntax-interpolation-errors branch from 21e2acc to 02517bc Compare September 14, 2023 22:55

ahoppen reviewed Sep 14, 2023

View reviewed changes

natikgadzhi force-pushed the macro-expansion/syntax-interpolation-errors branch from 02517bc to ec8265e Compare September 15, 2023 00:51

natikgadzhi requested a review from ahoppen September 15, 2023 00:51

ahoppen approved these changes Sep 15, 2023

View reviewed changes

ahoppen enabled auto-merge September 15, 2023 21:30

ahoppen merged commit 636a911 into swiftlang:main Sep 17, 2023

		// Start the diagnostic on a new line so it isn't prefixed with the file, which messes up the
		// column-aligned message from ``DiagnosticsFormatter``.

	XCTAssertTrue(context.diagnostics.first!.message.starts(with: "Internal macro error:"))
	let diagnostic = try XCTUnwrap(context.diagnostics.first)
	XCTAssertTrue(diagnostic.message.starts(with: "Internal macro error:"))

Prepend SyntaxStringInterpolationError errors in macro expansion with Internal macro error: #2176

Prepend SyntaxStringInterpolationError errors in macro expansion with Internal macro error: #2176

Uh oh!

Conversation

natikgadzhi commented Sep 11, 2023

Summary

Changes

Next steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natikgadzhi commented Sep 11, 2023

Uh oh!

natikgadzhi commented Sep 11, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natikgadzhi commented Sep 12, 2023 via email

Uh oh!

natikgadzhi commented Sep 14, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Sep 15, 2023

Uh oh!

kimdv commented Sep 16, 2023

Uh oh!

kimdv commented Sep 17, 2023

Uh oh!

kimdv commented Sep 17, 2023

Uh oh!

kimdv commented Sep 17, 2023

Uh oh!

Uh oh!

Prepend `SyntaxStringInterpolationError` errors in macro expansion with `Internal macro error:` #2176

Prepend `SyntaxStringInterpolationError` errors in macro expansion with `Internal macro error:` #2176