Fixing macros that expand to include other macros

Development Compiler
1

I was experimenting with creating my own peer macro that expands to include @Test and ran into some compiler bugs, so I was motivated to fix them. However, fixing the bugs includes some subtle user-facing changes, so I wanted to gauge other folks' opinions about whether this is just a bug fix or something that we need to be more deliberate about.

The problem is this: let's say I have a peer macro called @MyTest:

@MyTest func global() {}

struct S {
  @MyTest func member() {}
}

and this is meant to expand to:

func global() {}
@Test func global_myTest() {}

struct S {
  func member() {}
  @Test func member_myTest() {}
}

When this is compiled, @MyTest expands, and then @Test expands to its various parts. But the build still fails for a few reasons:

  • Lookup doesn't always include auxiliary decls that are themselves produced by auxiliary decls, so the code generated by the inner @Test can't reference other decls in its own expansion ("such and such is not found in scope" errors).

  • Even after fixing the lookup bug, SILGen has a similar issue of not recursing through auxiliary decls of auxiliary decls. This is a particularly sinister failure mode for Swift Testing because the generated decls are just small decl graphs rooted by @section data. So nothing actually fails to compile/link; the tests just don't get discovered at runtime.

  • The lexical context passed to the inner @Test macro is incomplete—it stops at the root of its own macro expansion buffer—so in the member case it can't see that it's inside a type. (For Swift Testing, this means that it doesn't emit the static keyword that it needs for some of its helpers.)

I've got a pull request that fixes these issues, but doing the right thing changes behavior in a couple ways:

First, user macros that are inside other macros today might be inadvertently relying on the fact that they don't get a complete lexical context, and now passing them a correct context could change their behavior. It's hard to imagine a situation where relying on the incomplete context is the right thing to do, but it could surface latent bugs in macros that haven't been there.

Second, one of the SourceKit tests that previously passed began to fail because it was relying on the broken lookup behavior.

How the test previously works today without my fix:

  1. @AddFuncPeer is attached to the #AddFunc freestanding macro.
  2. @AddFuncPeer is expanded, emitting func foo(_ x: Int).
  3. #AddFunc is expanded. Since the default behavior is to propagate attributes to the declarations produced by this macro, @AddFuncPeer is copied to the decls in that expansion. This emits @AddFuncPeer(x: 0) func foo(_ x: String).
  4. The @AddFuncPeer in this new expansion is expanded, emitting another copy of func foo(_ x: Int) at the same (global) scope.
  5. However, because of the lookup bug, the innermost foo(_ x: Int) is invisible to the compiler and the conflict is not diagnosed.

Fixing the lookup bug causes the file to (correctly) fail to compile because now there are two copies of foo(_ x: Int) visible in scope.

This seems like a trivially fixed bug in the macro implementation: either the freestanding macro should override propagateFreestandingMacroAttributes to false or the attached macro should be a no-op when attached to a MacroExpansionDecl.

But in any case, it's still an example of code that compiles successfully today (whether or not it's honoring the intent of the author) and that would fail to compile with the proposed fix.

What say folks? Would we consider these to be safe changes or do they need to be staged in somehow?

5 Likes