Detect deep recursion when expanding macros and error out

[dburgener]

No description provided.

[dburgener]

Fixes #884, as per @Pat-Lafon's suggestion to just cap recursion depth.

Two things I'm not really sure about and would like feedback on:

1. The 200 recursion depth is pretty arbitrary. When I was experimenting, I was noticing a delay and hitting ctrl+c around 8000 depth, give or take, and I imagine 200 should be plenty more than intentional recursion would hit?
2. I'd like it if the error message was a little more explicit about the problem and suggested a fix, but I'm not really sure about what suggestion to make. Any thoughts on a better error message?

[Pat-Lafon]

One thing to be mindful of is that this is technically a breaking change no? I would imagine that nobody would hit it(But if we knew that for sure, we wouldn't be considering the right value for loop iterations).

For the error message, I'm not 100% sure but if we did provide a form of Configuration for this, then we could say, "You have hit the macro expansion limit set in Lalrpop to conservatively avoid an infinite loop. If you believe that the macro expansion process will terminate, set a higher limit via Configuration::set_macro_expansion_limit.

[dburgener]

One thing to be mindful of is that this is technically a breaking change no?

That's frustrating, but you may be right. Browsing this Rust RFC around standard library breaking changes, I think we mostly fit in the "behavioral changes" section. It's a slightly weird spot, since lalrpop runs during build.rs, so lalrpop runtime is user compile time, and it's a behavioral change that can break compilation by virtue of running in the build.rs. According to that, we're supposed to be bound to our own documentation. Since our documentation doesn't specify a limit on recursive macro use, I guess this is sort of breaking?

Another angle that one could look at it is that the Linux kernel "we don't break userspace" rule is applied as breaking userspace in practice, rather than theory. If one person with a wonky setup is broken, it doesn't matter what theory says, the kernel broke userspace. And conversely, if it's clearly an API change, but there are actually zero API users, that's not a break. In that case, if we just set the limit high enough that no one hits it in practice, we're good. I don't think that turning an infinite loop into an error is an actual breaking change, so we're only worried about users who have legitimately nested macros enough to hit the limit.

If we want to go that second route, I worry 200 might be a bit low to have confidence. The main way I think this can happen is when a user defines many macros that call each other (in non-looping fashion). At some point, that could get 200 deep on pretty excessive macro use. The cut off is arbitrary, and there's no theoretical max to what someone might want to do, but in practice, I bet real numbers are typically 3 or 4, and we could still define a reasonable limit that seems very likely to cover any actual real world use case.

Of course, just calling it breaking and moving on is the less risky approach. If we want to go that way, we should consider if there's anything else breaking we are thinking about that would make sense to get in at the same time?

[dburgener]

In terms of the default, I poked around a bit at some of our top downloaded dependencies, and I found this grammar which makes pretty extensive use of macros and nesting macros. So I instrumented a build to count the loops in macro_expand(), and it took 10 loops to resolve all the macros. That seems to me like a lot, I imagine most grammars only need 2-3 at most.

All that said, I don't think there's much downside to a higher value. The major downside of continuing to run is growing ram usage. I can do some more experiments, but I have a feeling based on my earlier experimentation like we can safely go up at least into the 1000s (or higher) before having noticeable ram consumption issues on modern machines. So maybe a higher default value gives a little more wiggle room in case someone has extremely deeply nested macros, with the only downside being mildly worse performance in an error path.

I can do some more performance measurements and experimenting next week probably if we want to understand the tradeoffs on the default value better.

[Pat-Lafon]

Thanks for all the details. For sure I am interested in the other's input(@yannham @youknowone) since it is possibly breaking.

My initial thoughts are that we don't want to unnecessarily break people. This actually seems like a change that I wish we could leverage Crater for but I am having a hard time thinking of how someone would go over the 200 macro expansion limit. Hopefully for 99%+ of users this limit doesn't matter anyways.

Lalrpop is pre-1.0 so I think we should be more comfortable with listing these kinds of changes as possibly breaking even if it isn't in practice. I don't think the social cost is too high and we can take the opportunity to give users a heads up. (A random thought is to include a request for grammars if this change does break a user, I think we would want to add it to the test suite as a higher complexity use case.)

So lets bump the version number and I am also happy to raise the limit if there is a desire to do so.

[Pat-Lafon]

Of course, just calling it breaking and moving on is the less risky approach. If we want to go that way, we should consider if there's anything else breaking we are thinking about that would make sense to get in at the same time?

Is there something you had in mind? I'm not opposed, but given some of my reasoning above I'm pretty happy to just ship this. Being pre-1.0 means there is a little less pressure to package breaking changes together and the change we are making is probably barely breaking to begin with.

[yannham]

Thanks for taking care of this, @dburgener. My personal stance on backward compatibility is that I usually lean on the side of "we don't break backward compatibility if we fix a behavior that was entirely accidental and that nobody should sanely rely on (and wasn't advertised in the documentation)". Of course the line isn't always entirely clear (insert obligatory XKCD here), but otherwise our hands are too tied and the price to pay is to carry technical debt for no clear benefits.

If we would have been post-1.0, I think I would be ok with not incrementing the major version for this change (in balance with the other too costly alternatives: do nothing and preserve the old behavior or increment the major version for something that won't actually break any real world program).

However, as @Pat-Lafon says, we don't have this issue here - it's cheap to increment the version in pre-1.0, so I'm fine with bumping the minor (but thus making it a breaking change in pre-1.0) version.

[dburgener]

Regarding the breaking aspect, yeah, I think the "we're pre-1.0" case is convincing enough. I'm fine with shipping this as breaking just in case.

I didn't have in mind anything else breaking we're looking to do, no. I just like batching breaking changes if we can/it makes sense, so wanted to flag the option if anyone else had anything in mind.