Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dependency on more-asserts #4408

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

alexcrichton
Copy link
Member

In my recent adventures to do a bit of gardening on our dependencies I
noticed that there's a new major version for the more-asserts crate.
Instead of updating to this though I've opted to instead remove the
dependency since I don't think we heavily lean on this crate and
otherwise one-off prints are probably sufficient to avoid the need for
pulling in a whole crate for this.

@sunfishcode
Copy link
Member

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.

@alexcrichton
Copy link
Member Author

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.

@cfallin
Copy link
Member

cfallin commented Jul 7, 2022

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 :-)

@sunfishcode
Copy link
Member

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 rust-lang/rust#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 :-).

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I for one like removing dependencies too, notably when they're small and not too far from being natively supported by rustc. I think with sufficient effort we could also decide to recreate the custom error messages since assert! supports custom formatting afaik. This seems good to take as is, though, and there seems to be consensus around that now, so approving.

In my recent adventures to do a bit of gardening on our dependencies I
noticed that there's a new major version for the `more-asserts` crate.
Instead of updating to this though I've opted to instead remove the
dependency since I don't think we heavily lean on this crate and
otherwise one-off prints are probably sufficient to avoid the need for
pulling in a whole crate for this.
@alexcrichton alexcrichton enabled auto-merge (squash) July 26, 2022 16:06
@alexcrichton alexcrichton merged commit 1321c23 into bytecodealliance:main Jul 26, 2022
@alexcrichton alexcrichton deleted the less-asserts branch July 26, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants