regression: overflow evaluating requirement NonNull<Vec<u8>>: Unpin

[Mark-Simulacrum]

https://crater-reports.s3.amazonaws.com/beta-1.74-4/beta-2023-10-21/reg/zvariant-3.15.0/log.txt

[INFO] [stdout] error[E0275]: overflow evaluating the requirement `NonNull<Vec<u8>>: Unpin`
[INFO] [stdout]   |
[INFO] [stdout]   = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`zvariant`)

This seems to recurse into more nested vecs -- not clear why, but doesn't seem like the right behavior.

https://crater-reports.s3.amazonaws.com/beta-1.74-4/beta-2023-10-21/gh/lcnr.binary/log.txt is potentially another example... @lcnr, maybe this means that this is a known bug?

[lcnr]

the root goal seems to be

Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<Vec<u8>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>: Unpin

which feels like it should be incredibly close to the recursion limit. Considering that it hit the recursion limit at NonNull<Vec<u8>>: Unpin it looks like we now increment the recursion depth a bit more somewhere.

The same for https://github.com/lcnr/binary/blob/master/src/main.rs, I apparently set the recursion limit as low as possible, incrementing it to 540 or sth makes this compile again.

#![recursion_limit = "513"]
// We need O(n) levels of recursion, where n is the maximum amount of bits,
// and as we multiply two 256 bit numbers, this is the lowest possible value.

I personally think it's fine to break these crates (maybe opening a Pr to zvariant increasing the recursion limit), but it would still be good to know the root cause.

I wasn't able to test zvariant myself but bisecting https://github.com/lcnr/binary/blob/master/src/main.rs should work :thinking:

[Mark-Simulacrum]

Ah, I missed that these lower the limit. Maybe we can add a note about that to these errors?

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[zeenix]

zvariant author/maintainer here. :wave: I didn't know of this issue until today when our CI started to fail because this is now reproducible with the latest rust release (1.76). The interesting thing is, the failure happens because of our testcase for recursion. :) The D-Bus format has its own limits to container recursions and we need to ensure we're respecting these limits.

Since the recursion limits in Rust can change and they may or may not be compatible with that of D-Bus, I think we should keep this test case. Although it seems counter-productive to set our own recursion limit on the whole crate, just for this test case, since the recursion limit attribute can only be applied to the whole crate, I guess we don't really have much choice(?). :cry:

[lcnr]

Yeah, always setting the recursion limit is probably easiest :thinking: there shouldn't be any downsides to increasing the recursion limit to double the original value. Making sure your crater otherwise compiles even if that's not too useful imo, given that any increased recursion limit also applies when compiling your crate as an dependency. You could only increase the recursion limit with cfg_attr(test, ..) :thinking:

[zeenix]

You could only increase the recursion limit with cfg_attr(test, ..) 🤔

Oh, I forgot about cfg_attr magic. That indeed seems to work and is the best solution. Thanks so much! I owe you a :beer:.