Please considering removing non-determinism from bon's macros

[davidbarsky]

Hello! Would you be willing to remove the non-determinism in the proc macros? Part of my job is supporting Rust at Facebook, which builds the code using Buck. Buck strongly depends on deterministic builds in order to cache intermediate artifacts and produce correct binaries, but this non-determinism blocks my colleagues and I from upgrading to bon 3.1.

(As a previous example where non-determinism is problematic for Buck—and I assume Bazel—see: yaahc/displaydoc#52.)

As an alternative, I think a #[doc_hidden] could be supplant rust's lacking macro hygiene—rust-analyzer will not complete #[doc_hidden] items, which should prevent most accidental usage of bon's internals.

A note for the community from the maintainers

Please vote on this issue by adding a 👍 reaction to help the maintainers with prioritizing it. You may add a comment describing your real use case related to this issue for us to better understand the problem domain.

[Veetaha]

Hi, thank you for creating the issue!

I suspected, this non-determinism could cause some trouble, although I didn't know how exactly, so I was waiting for an issue like this to convince myself that removing randomness would be important. Given your explanation, I'm okay with changing the private fields to use static identifiers.

It was my experimental way to ensure "hygiene" as you mentioned, which I thought would be very important given that the generated code internally uses unwrap_unchecked, that could otherwise be unsound if the users mess with the builder's internal fields. That unwrap_unchecked could be made into just unwrap, but that isn't always optimized out by the compiler according to my benchmarks, so I wouldn't change that.

Anyway, I guess this precaution isn't worth the headache of non-determinism that you found. I think it's enough for the fields to be marked with #[doc(hidden)], #[deprecated = "don't use this"] and begin with the __private word.

Just a followup question. How did you discover this problem? Did you stumble on repeated cache misses when building with Buck or did you pre-audit the code before upgrading to 3.1?

[Veetaha]

Removed non-determinism. If users ever try to access the private fields of the builder, they'll get this warning:

warning: use of deprecated field `FooBuilder::__unsafe_private_phantom`: this field should not be used directly; it's an implementation detail, and if you access it directly, you may break some internal unsafe invariants; if you found yourself needing it, then you are probably doing something wrong; feel free to open an issue/discussion in our GitHub repository (https://github.com/elastio/bon) or ask for help in our Discord server (https://bon-rs.com/discord)
 --> crates/sandbox/src/main.rs:9:5
  |
9 |     Foo::builder().__unsafe_private_phantom;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

(also added unsafe into field names to make it clear that using them may break internal unsafe invariants).

[davidbarsky]

Thanks so much for the prompt reply and fix!

It was my experimental way to ensure "hygiene" as you mentioned, which I thought would be very important given that the generated code internally uses unwrap_unchecked, that could otherwise be unsound if the users mess with the builder's internal fields. That unwrap_unchecked could be made into just unwrap, but that isn't always optimized out by the compiler according to my benchmarks, so I wouldn't change that.

Yeah, that makes sense—soundness is a pretty reasonable motivation.

Just a followup question. How did you discover this problem? Did you stumble on repeated cache misses when building with Buck or did you pre-audit the code before upgrading to 3.1?

Nope! Nothing that clever; just monorepo-wide CI! Some crates failed to build with E0460:

error[E0460]: found possibly newer version of crate `[redacted]` which `[redacted]` depends on`
out/v2/gen/fbcode/fcb149355dcd9da8/[redacted]/__srcs/src/[redacted]/lib.rs:28:1
  |
3 | extern crate [redacted];
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: perhaps that crate needs to be recompiled?
  = note: the following crate versions were found:
  [bunch of rlibs that are not vendored from crates.io]

I'm skipping past a few details, but this error shouldn't be possible to hit with Buck because of how the build graph is constructed and executed. While that error is a tell-tale sign of non-determinism in a crate (at least, under Buck/Bazel), some folks found the non-determinism inside Bon, and well, here we are.

[Veetaha]

Some crates failed to build with E0460:

Thank you for the pointer to the error. I'll keep that in mind in the future. Hopefuly, this problem wasn't too much of a headache to debug. Sorry, for breaking your CI 😳

I've released a patch 3.1.1 with the fix for this.

[davidbarsky]

Some crates failed to build with E0460:

Thank you for the pointer to the error. I'll keep that in mind in the future.

I honestly don't know how generalizable the E0460 trick actually is. I've mostly pattern matched on previous examples of non-determinism in Buck and this error was the common detail. I assume Bazel would also run into this error, but I don't know if, for example, Nix would report this error or if it'd just have suspiciously longer build times.

(It'd be cool if rustc could tell if a proc macro was non-deterministic or if there was a cargo tool that did this, but ah well.)

Hopefuly, this problem wasn't too much of a headache to debug. Sorry, for breaking your CI 😳

Don't worry about it! This took us about 5 minutes to debug, if that. I spent more time writing up this issue 😀. Besides, this broke the tests before the Phabricator diff/PR landed. Even if CI was broken after landing, CI is meant to be broken and it would've been automatically bisected and reverted by a bot. There is no need to apologize at all. I'm just really thankful over how quickly you fixed this problem and how much you communicated about the why behind the original decision!

I've released a patch 3.1.1 with the fix for this.

Thank you so much for the quick fix! We'll get on it right away.