Stream: git-wasmtime

Topic: wasmtime / issue #4408 Remove dependency on `more-asserts`

Wasmtime GitHub notifications bot (Jul 07 2022 at 18:36):

sunfishcode commented on issue #4408:

I'm fond of more-asserts, myself, because it prints the values of the comparison operands when it fails. We can print things manually when we anticipate the need, but the point of most asserts is that we don't expect them to ever fail, so it's difficult to predict when we need to do this. Particularly with Wasmtime being used in increasingly "interesting" environments, where it isn't always easy to reproduce failures, the more information we can get from an assertion failure, the better.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:06):

alexcrichton commented on issue #4408:

I don't disagree this is nice-to-have and I'm not going to try to replace assert_eq! with assert! any time soon but I do still feel this dependency isn't pulling its weight. It's not like 100% of our assertions are using more_asserts or we 100% aren't using assert! anywhere in the codebase. The vast majority of the usage of more-asserts was in tests which wouldn't come up from error reports anyway.

I personally don't want to ignore the cost of having an external dependency where anyone unfamiliar with the crate has to learn it, it needs updating from time-to-time, etc. Given the limited capacity in which this is used and I don't feel like it's worth keeping around just in case we get a better error message. So far at least I feel like most of our assert failures are coming from fuzz bugs rather than users who give us a report and then disappear and don't follow-up.

Wasmtime GitHub notifications bot (Jul 07 2022 at 20:39):

cfallin commented on issue #4408:

If this needs any more input to tip the balance one way or another, I'll offer my subjective input as well: I'd tend to prefer cutting dependencies where feasible as well as the build-time / build-size costs alone tend to add up, in addition to the complexity costs that Alex mentions. I realize I'm somewhat against the Rust-culture grain here (Cargo certainly enables a really vast and healthy ecosystem of reuse, and we shouldn't go back to the bad old days of C libraries reinventing everything from scratch) but I think frugality pays off significantly more lower in the dependency graph where we tend to sit, and the stakes are higher. For something like an assert library maybe it doesn't matter too much either way; but the general trend and precedent does, a bit more, I guess.

Anyway that's just my opinion and I'm happy to be overruled :-)

Wasmtime GitHub notifications bot (Jul 07 2022 at 23:00):

sunfishcode commented on issue #4408:

I generally agree about minimizing dependencies, though as dependencies go, this one is very mild. We indeed don't use it a lot, but we do use it in some core places, and by a quick grep, it seems we could use it in a lot more places than we currently do. And even in tests, it can be helpful when something fails in CI that isn't convenient to reproduce.

That said, while looking into this I noticed that https://github.com/rust-lang/rust/pull/97665 recently landed, which implements the features of assert_lt, and beyond, within regular assert, even printing values of variables in things like x + y == z. It's not stabilized yet, but perhaps it's enough for me to just wait for that.

So it's ok with me to merge this :-).

Last updated: Nov 22 2024 at 16:03 UTC