Detect circular macro expansion

[AppAppWorks]

fixes #2018

[ahoppen]

Wouldn’t it be simpler to just maintain a stack of expanded macros, add a macro’s name to it whenever we are inside its expansion and then pop it again afterwards? And if we try to expand a macro that’s in that list, emit an error?

[CodaFi]

Do you consider a tail-recursive macro whose expansion eventually terminates to be bannable? I think we have to be very careful about what counts as non-productive recursion.

[CodaFi]

Wouldn’t it be simpler to just maintain a stack of expanded macros, add a macro’s name to it whenever we are inside its expansion and then pop it again afterwards?

Now you're approaching the territory of a termination checker. I suggest tracking expansions with a stack as suggested, but rather than query the stack let's use it to install a reasonable depth limit and call it a day - dumping the stack for diagnostics if need be. The full scope of efficiently rejecting general recursion in programs is an open area of research 😓

[CodaFi]

a reasonable depth limit

Unfortunately, I'm not sure what this number should be, and it seems neither do our peer compilers.

[AppAppWorks]

Do you consider a tail-recursive macro whose expansion eventually terminates to be bannable? I think we have to be very careful about what counts as non-productive recursion.

@ahoppen and I have discussed in #2018 that recursive macro expansion is currently prohibited by the compiler. Notwithstanding, I think determining whether a tail-recursive macro will eventually terminate is akin to the halting problem?

[CodaFi]

That’s because #infiniteRecursion isn’t allowed to expand to #infiniteRecursion again, even if it contains different argument.

I see, well then that is really quite restrictive and I do disagree with it but I defer to Alex on this one.

I think determining whether a tail-recursive macro will eventually terminate is akin to the halting problem?

Absolutely, but there is a lot of research in restricted forms of termination checking, mostly in service of proof assistants. There, you're absolutely willing to sacrifice a certain class of (often, ill-typed anyways) terms that will happen to halt but that your termination checker can't really prove. There's heuristics like syntactic decrease all the way up to stuff like Andreas Abel's research on termination checking for Agda which reduces inter-procedural analysis to a series of matrix multiplies. It's really neat stuff!

That said, if you peek around production compilers with different forms of macro evaluation, you'll see they all implement some kind of arbitrary expansion depth limit and usually some kind of compiler flag to allow you to raise or lower it as needed. It's all kinda gnarly.

[ahoppen]

To be clear, we currently have the restriction that a macro can’t expand to itself in the compiler. And this PR is just implementing the same behavior in MacroSystem, which is used for assertMacroExpansion. The goal here is to make the compiler and MacroSystem match, not to change any limitations on recursive macro expansion.

Wouldn’t it be simpler to just maintain a stack of expanded macros, add a macro’s name to it whenever we are inside its expansion and then pop it again afterwards? And if we try to expand a macro that’s in that list, emit an error?

Did you address this comment yet, @AppAppWorks? I couldn’t find anything related to it in your latest commits. Just checking that we’re not mutually waiting for each other.

[AppAppWorks]

Did you address this comment yet, @AppAppWorks? I couldn’t find anything related to it in your latest commits. Just checking that we’re not mutually waiting for each other.

I've just reverted to a previous commit as well as corrected the sites of stack popping, more precisely we now only pop a macro type name from the stack whenever such freestanding macro has been fully expanded.

[ahoppen]

• I would really prefer if the place that adds elements to expandingFreestandingMacros is connected to the place that removes them. It's just too easy to forget to remove them otherwise.

[AppAppWorks]

• I would really prefer if the place that adds elements to expandingFreestandingMacros is connected to the place that removes them. It's just too easy to forget to remove them otherwise.

True, I'll put some more thought into refactoring.

[ahoppen]

FWIW the compiler also emits an error if an attached macro is recursively expanded in the same role. https://github.com/swiftlang/swift/blob/454dfb9a0b19917a64e2ecda31e714203a05231e/lib/Sema/TypeCheckMacros.cpp#L1364-L1369

I think it would be good to implement that in MacroSystem as well but that doesn’t have to be this PR.

[AppAppWorks]

FWIW the compiler also emits an error if an attached macro is recursively expanded in the same role. https://github.com/swiftlang/swift/blob/454dfb9a0b19917a64e2ecda31e714203a05231e/lib/Sema/TypeCheckMacros.cpp#L1364-L1369

I think it would be good to implement that in MacroSystem as well but that doesn’t have to be this PR.

I wrote this in #2018,

As reported in #1805, even though SyntaxProtocol.expand() isn't yet a "full" expansion system, the expansion behaviours of different subtypes of AttachedMacro vary as some of them will expand macros recursively to some extent.

Should the upcoming PR also solve this incoherent behaviour across different types of attached macros?

[ahoppen]

Should the upcoming PR also solve this incoherent behaviour across different types of attached macros?

Yes, let’s do a separate PR for that.

[ahoppen]

Looking great

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[ahoppen]

Could you run swift-format? https://github.com/swiftlang/swift-syntax/blob/main/CONTRIBUTING.md#formatting

[AppAppWorks]

Formatted, please test again.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows