Move assert_matches to an inner module by m-ou-se · Pull Request #86947 · rust-l...

Copy link

Member

rust-highfive commented 16 days ago

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been hidden.

jyn514 commented 16 days ago

(I edited the name because "submodule" has a specific meaning WRT git, and I was confused at first.)

This comment has been hidden.

This comment has been hidden.

yaahc commented 9 days ago

@rfcbot merge

rfcbot commented 9 days ago •

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented 9 days ago

This is now entering its final comment period, as per the review above.

yaahc commented 8 days ago

@bors r+

bors commented 8 days ago

Commit e304443 has been approved by yaahc

richkadel commented 7 days ago

@m-ou-se @yaahc

This change is causing lots of downstream build failures (compiling Fuchsia for instance).

Choosing to call this std::assert::assert_matches! is causing compiler errors in code that explicitly references (e.g.,use) the macro std::assert!, because they are at the same item path as the parent mod: std::assert or core::assert now refer to both the default assert! macro and the mod containing assert_matches.

In fact, the feature gate for assert_matches generates compile time errors for legitimate references to the std::assert macro (Rust tries to apply the feature gate when it incorrectly assumes the reference is to the parent mod of assert_matches! making the error message unintuitive.

Example, in some code I'm trying to compile since this landed, the original author needs to specify an absolute package path to, for instance, deconflict with a different item in the local mod, with the name assert.

I think it might be better to use an item path that isn't already in use, like one of:

• std::assert_matches::assert_matches would at least match the feature gate name (I mention this because it was confusing at first that the error messages I got recommended enabling the feature(assert_matches) just to use core::assert.
• std::asserts::assert_matches is a shorter alternative you might also consider

cc: @tmandry @anp

m-ou-se commented 7 days ago

Oh, good catch. Let's move it to std::macros or something for now. The name doesn't matter much, since the module is unstable.

@yaahc , do you have time to do this and handle the backport too?

richkadel commented 7 days ago

I added #87189 for tracking purposes

RalfJung commented 7 days ago

@rust-lang/infra this PR changes rustc_mir/interpret but the bot failed to do the usual ping... is this a known problem? (The change is fine of course.)

I assume the change was not present in the PR at the beginning and got added by pushing to the branch later.