Stop propagating generated on macro arguments

[sabiwara]

Close #14676

Alternative to #14677

I've tried a couple approaches here, but eventually I think this is probably the way to go:

• protect macro args before expansion by adding stop_generated: true
• remove these as part of linify and stop marking as generated

Also tried:

1. working with unquotes as originally discussed: I think this is the wrong thing to consider, since macros internally might be building AST by calling quote multiple times and injecting smaller blocks into bigger blocks => what we want is to stop at macro args, no matter how the macro is implemented internally?
2. linify to only mark generated if no line in the meta -> kinda works for real-world cases but not when just trying out quote + Macro.expand in the shell or tests
3. rather than stop_generated, stop_linify: true altogether (since we probably shouldn't add lines of the macro in injected args?) => seems reasonable but breaks with left variables undefined in ExUnit, I could try to fix it but figured this might be too risky of a change

The current stop_generated approach still breaks some tests though, so I'm not 100% sure it's the right approach.

[josevalim]

Thank you! I like this approach and agree with your decisions. ❤️

One suggestion: what if we go with generated: false for those entries then?

Given setting {generated, true} already has to traverse metadata so we don't add it twice, if we do {generated, false}, we only have to traverse metadata once, looking for either true or false, and acting accordingly. Plus one fewer metadata option in general.

Thoughts?

[sabiwara]

we only have to traverse metadata once, looking for either true or false, and acting accordingly

Oh that's a good idea.
Intent-wise, it might be slightly confusing and less "greppable" than the explicit stop_generated, but I think it might be worth the trade-off.
Let me give it a shot!

[sabiwara]

@josevalim I tried but it adds more failing tests, I haven't pinpointed the exact cause but I think it makes it arguably harder to reason about potential side effects, given that several places in the code are using Keyword.take(meta, [:generated]) or equivalents.
So I think it's safer to stick to the specific meta approach, also the new meta doesn't necessarily need to be documented since it is "transient", i.e. it is added before expansion but removed afterwards, WDYT?

[josevalim]

Let's go with this one then!

Also, let's not backport this one, so we have more time to test it in the wild.

[sabiwara]

Also, let's not backport this one, so we have more time to test it in the wild.

I was thinking the same 🤝