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

stabilise assert_matches #120234

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Dylan-DPC
Copy link
Member

Closes #82775

Currently there is some discussion around #82913. So I assume this would either be blocked on that if needed.

@rustbot
Copy link
Collaborator

rustbot commented Jan 22, 2024

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 22, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment was marked as outdated.

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2024
@Dylan-DPC Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2024
@bors
Copy link
Contributor

bors commented Jan 26, 2024

☔ The latest upstream changes (presumably #119968) made this pull request unmergeable. Please resolve the merge conflicts.

@cuviper
Copy link
Member

cuviper commented Jan 26, 2024

Currently there is some discussion around #82913. So I assume this would either be blocked on that if needed

Then I'll leave that to API folks.

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 26, 2024
@rustbot rustbot assigned m-ou-se and unassigned cuviper Jan 26, 2024
@Voultapher

This comment was marked as resolved.

@m-ou-se m-ou-se added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 8, 2024
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting. The consensus we came up with was:

We want to stabilize this. We're not thrilled with the module name assert_matches (std::assert_matches::assert_matches!), and would prefer a more general name there. However, we don't want to delay stabilization while trying to bikeshed that name. So, we're proposing that we leave the assert_matches module unstable for now (and let that bikeshed painting happen asynchronously), but go ahead and re-export the assert_matches macro from std::prelude::rust_2024, so that people can use it either by moving to Rust 2024 or by importing std::prelude::rust_2024::assert_matches.

Meanwhile, if someone comes up with a great idea for a module assert_matches should live in, we can move it there and stabilize that module.

@Voultapher
Copy link
Contributor

@joshtriplett to clarify, did the libs team talk about the concern I raised? Your comment does not mention it at all. That's why I'm asking.

@Amanieu
Copy link
Member

Amanieu commented Feb 27, 2024

@Voultapher We did discuss your concern about the if syntax. Our conclusion is that it is more important for the syntax of assert_matches to be consistent with the one for matches.

If changes to the syntax are needed, this should be applied to both matches and assert_matches, although the bar for changing an already-stable API (matches) is very high. Regardless, this should be discussed separately from the stabilization of assert_matches.

@cuviper
Copy link
Member

cuviper commented Feb 27, 2024

Meanwhile, if someone comes up with a great idea for a module assert_matches should live in, we can move it there and stabilize that module.

Was a simple core::assert module considered? All of the assertion macros could live there, while still being re-exported from the root for compatibility. I know core::assert::assert_matches! is a bit of a stutter, but that's no different than alloc::vec::Vec, core::sync::atomic::Atomic*, etc.

@Amanieu
Copy link
Member

Amanieu commented Feb 27, 2024

core::assert causes issues because importing will cause conflicts with the assert macro in the crate root.

@cuviper
Copy link
Member

cuviper commented Feb 27, 2024

Ah -- then maybe core::assertions, though that's a bit more of a mouthful. A broader core::macros wouldn't be horrible either IMO, but I could understand hesitation about that.

@Voultapher
Copy link
Contributor

@Amanieu thanks for clarifying and considering the point. While it's a bit unfortunate I don't see a better alternative either, that doesn't involve a time machine.

@Amanieu Amanieu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 28, 2024
@joshtriplett
Copy link
Member

joshtriplett commented Feb 29, 2024

Ah -- then maybe core::assertions, though that's a bit more of a mouthful. A broader core::macros wouldn't be horrible either IMO, but I could understand hesitation about that.

We did consider core::macros as well. We're going to consider the module name further, but we wanted to let this proceed without waiting on the bikeshedding. :)

@eirnym
Copy link

eirnym commented Apr 30, 2024

@joshtriplett Have you considered the same placement as the rest assertions? I believe this one would useful be the same as other assertions

@joshtriplett
Copy link
Member

@eirnym The rest of the assertions are in the root of core, which we can't do with assert_matches due to conflicts.

@joshtriplett
Copy link
Member

What's the current status of this? Is it possible to stabilize the addition of assert_matches to the prelude without stabilizing the non-prelude location of it?

@Dylan-DPC
Copy link
Member Author

I haven't looked at it since the last review, i'll give it this week or next (hopefully) and update here accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for assert_matches
10 participants