Replace some matches! with if let or ==

[estebank]

No description provided.

[rustbot]

Some changes occurred to constck

cc @fee1-dead

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in exhaustiveness checking

cc @Nadrieril

rustc_errors::emitter was changed

cc @Muscraft

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in check-cfg diagnostics

cc @Urgau

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred in compiler/rustc_ast/src/expand/autodiff_attrs.rs

cc @ZuseZ4

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rust-log-analyzer]

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] unicode_segmentation test:false 0.259
[RUSTC-TIMING] askama_derive test:false 1.271
    Checking askama v0.14.0
[RUSTC-TIMING] askama test:false 0.165
error[E0369]: binary operation `==` cannot be applied to type `cfg::Format`
   --> src/librustdoc/clean/cfg.rs:228:33
    |
228 |         let mut msg = if format == Format::LongHtml {
    |                          ------ ^^ ---------------- cfg::Format
    |                          |
    |                          cfg::Format
    |
note: an implementation of `std::cmp::PartialEq` might be missing for `cfg::Format`
   --> src/librustdoc/clean/cfg.rs:403:1

[RalfJung]

What is the reason for this change? What I like about matches! is that I can use it without worrying about whether the type implements PartialEq. Also, with if let I have to carefully check the condition to see if anything is being bound; matches! makes it clear that nothing is bound.

So while I don't really mind any of these diffs, it also doesn't look like a clear improvement that's worth a sweeping change, and new "unnecessary" matches! will inevitably be added again.

[jdonszelmann]

agreed, not sure this is necessarily better.

[nnethercote]

I don't mind the change. if let/== is shorter than matches!, and using built-in features rather than macros is nice.

I do think a PR touching 82 files deserves a description that explains the how and the why, though. What prompted this? Did you automate any of it? When did you use == vs if let?

[estebank]

What is the reason for this change?

I do think a PR touching 82 files deserves a description that explains the how and the why, though. What prompted this? Did you automate any of it? When did you use == vs if let?

That's fair! I was touching something else and noticed a matches! that was present most likely only because let chains didn't exist at the time, so I did a quick if matches! search and went through them manually while I was at an airport and published the PR with the intention to start a conversation about whether we should prioritize this new style or not (and then proceeded not to start it). As alluded by Ralf, I tried to use == whenever PartialEq was already implemented and avoided adding it to those that didn't (even though they probably should, like those that are purely Yes/No enums).

What I like about matches! is that I can use it without worrying about whether the type implements PartialEq. Also, with if let I have to carefully check the condition to see if anything is being bound; matches! makes it clear that nothing is bound.

So while I don't really mind any of these diffs, it also doesn't look like a clear improvement that's worth a sweeping change, and new "unnecessary" matches! will inevitably be added again.

I went through these manually because I wanted to get a sense for whether making a clippy or rustc lint for this would be useful. I see that clippy has almost the opposite, when a match expression could be replaced with matches!, and for when if let could have been == instead, but none for the case touched in this PR.

---

Regardless, the reason I created the PR was to gather feedback: is the style shown here what we'd want? I do think that if we have to keep matches! around it shows a lang expressiveness limitation. I am not too convinced by the argument about having to double check whether there are new bounds introduced (but understand where it comes from). Seeing matches!(expr, Pat if cond) was what made me do this, because that should really be an if let chain most of the time, IMO. The only cases where I saw that matches! is still clearly necessary are 1) when wanting to be stable-compatible and would need an if let chain in a match arm condition and 2) when you have || instead of a let chain.

[RalfJung]

Stylistically I don't like if let that don't actually bind anything. let is a binding construct, it is confusing to use it without binding anything. I think matches! expresses the intent a lot more clearly there.

[fee1-dead]

I am fine with the changes from matches to ==, such as

if matches!(data.terminator().kind, TerminatorKind::Return) {

into

if data.terminator().kind == TerminatorKind::Return

As for the rest I agree with Ralf's comment.

View changes since this review

[fee1-dead]

This one should be an if let chain:

Suggested change

	if let ty::Ref(_, ty, _) = args[0].node.ty(&self.ccx.body.local_decls, tcx).kind()
	&& let ty::Ref(_, ty, _) = ty.kind()
	&& ty.is_str()
	{
	// Allow this call, skip all the checks below.
	return;
	} else {
	self.check_op(ops::PanicNonStr);
	}

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[nnethercote]

Seeing matches!(expr, Pat if cond) was what made me do this

I agree that those case are particularly ugly, and if-let is an improvement.

Sounds like splitting the changes into groups might help with progress. Using == with types that already implement PartialEq seems uncontroversial, that could be one PR. Then perhaps the matches! that use if could be another PR, if there are enough of them.

[bors]

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

[oli-obk]

Using == with types that already implement PartialEq seems uncontroversial, that could be one PR.

Idk. It is annoying that you need to change back to if let the moment you add a field to an enum variant that you don't care about in most places.

Stylistically I don't like if let that don't actually bind anything. let is a binding construct, it is confusing to use it without binding anything.

To me it's just a single variant version of match, and we obviously prefer match over if chains. == to me is for literals and values, not in-place constructed values.

[nnethercote]

It is annoying that you need to change back to if let the moment you add a field to an enum variant that you don't care about in most places.

"This code is uglier than the alternative, but it might minimize the size of a future diff" feels off-kilter to me.

[fee1-dead]

I think oli was talking about something like this:

if MyEnum::Variant == value {}

over

if let MyEnum::Variant = value {}

Which IMO I have no preference over either. But I don't think any of these are uglier over the other either. In any case, I don't think any of these things changed in the diff are using enum variants that might add new data soon. Someone's going to have to change the if let case to MyEnum::Variant(..) if they do decide to add fields in the future, so I think it's fine.