Rust_2024_compatibility lint and the if let temporary scope

help
1

I just stumble over the lifetime capture change and the linter warnings when using rust_2024_compatibility.

I usually use something like:

if let Err(err: Error) = mutexguard.fn_call(...) {
    error!(...);
    ...
} else {
    ...
}

This gives me now a warning, I don't know why.
In this case there is no difference in code, because it doesn't matter when err is now dropped exactly.
You can't and couldn't use err outside of the scope and it hasn't been used.

I've now 3 possibilities, write everywhere a

#[allow(if_let_rescope)]

or rewrite the if let else to:

if mutexguard.fn_call(...).inspect_err(|err| error!(...)).is_ok() {}

or follow the linters suggestion to change the if-let to match with total crude Err-and-match-all syntax.

I would say readability of the old code is best and the warning makes no sense.

The mutexguard is retrieved with a match long before the if let else and also is hold locked hopefully after its else for the whole scope of the match.

Is this a linter bug? Or what should I think of this?

1 Like
2

If you’re asking a question about a warning, you should probably share the entire warning message…

…especially if you’re asking “is this a linter bug?” without demonstrating “this” in detail :wink:

3

The lint in question here is if_let_rescope, which is allow-by-default, so it is only because you are asking it to be warned on (as part of the rust_2024_compatibility lint group) that makes it warn. The description of the lint is clear that it can't detect whether a Drop implementation is significant or not just based on its existence, and that you should use your discretion as to whether to accept or reject the suggestion. The idea of the lints is that you use them to check for where it might cause problems and decide how to address them. The suggestion to use match is the course of action that is guaranteed to preserve semantics, but that may not be necessary or desired in your case.

4

Okay, I think I got it reproduced now, anyway. This is firing for most Error types with a destructor (only exception being the ones from the standard library that have been explicitly marked as “insignificant”). I would agree that this particular case seems particularly useless to lint against since the destructor of the whole Result isn’t really doing anything in your else case, if the pattern accepts any Err(_) – unless the Ok variant’s data also has a nontrivial destructor.

So regarding…

yeah, maybe the lint is less smart here than it could be.

2 Likes
5

I would suggest the following:

  • start out by adding #![warn(if_let_rescope)] to top of your whole crate, and run cargo check (without other rust_2024_compatibility lints, to help focus your attention to this relevant case)
  • manually look at all the places that are linted, see if there are any in your code where there is actually any change in behavior to be expected
    • if your code doesn’t use unsafe, there’s generally little concern of any bad bugs being introduced anyway, so if it’s too much effort to do the manual review, feel free to skip everything up to this point
  • when you’re happy with your manual handling of if_let_rescope-cases, change your code by adding a #![allow(if_let_rescope)] to the top of your crate instead
  • proceed with edition migration for other issues besides if_let_rescope. From these remarks for partial migration, it sounds like for more fine-graned / partial fixups like this you would have to:
    • not use the convenient cargo fix --edition command anymore, if you want to automatically fix other parts of your code. I couldn’t find a way to make this command apply less than all the lints it knows. Instead, if you still want other auto-fixes applied, you can use normal cargo fix – for example:
      • you could add #![warn(rust_2024_compatibility)] to the top of your crate, before the #![allow(if_let_rescope)] from above which disables the if-let specific part of this lint group
      • then use normal cargo fix without the --edition argument to fix the auto-fixable warnings that this produces
  • eventually, you’ll be updating the crate’s edition to 2024; make sure the code still compiles and works
  • after migration, you can remove the new crate-level #![warn(…)] and #![allow(…)] again
    • the if_let_rescope will not further warn on your code after the migration; you don’t have to worry about needing to keep around one (or many) such #[allow(if_let_rescope)] in your code, it isn’t necessary
    • most migration lints are not really meant to be activated outside of migration scenarios; if_let_rescope is allow-by-default for a reason, and this one is also explicitly meant to be ignored in cases where you know better than the lint and can determine that your code doesn’t actually change meaning – or at least not significantly so
6

Yes, the #4 is one of the cases.
And your description seems to hit it.

Allowing the lints (I do this in Cargo.toml) make the lints quite useless, but cluttering code with lint-allows on line/scope base for irrelevant lints makes also no sense.

[lints.rust]
rust_2024_compatibility = { level = "warn", priority = -1 }
edition_2024_expr_fragment_specifier = "allow" # 2024
if_let_rescope = "allow" # 2024
tail_expr_drop_order = "allow" # 2024

I've put it in to be edition compatible and have less work when I decide to migrate, which has worked quite nice with the editions before.