remove-unnecessary-else bad suggestions and false positives on thread_local macro

[awused]

rust-analyzer version: rust-analyzer 0.3.1839-standalone

rustc version: rustc 1.76.0 (07dca489a 2024-02-04)

relevant settings: Unlikely any are relevant

Bad suggestions: Given this code

fn main() {
    let files = [1];

    let query = if files.is_empty() {
        return;
    } else if files.len() == 1 {
        1
    } else {
        17
    };
    print!("{query}");
}

remove-unnecessary-else suggests

fn main() {
    let files = [1];

    let query = if files.is_empty() {
        return;
    }
    if files.len() == 1 {
        1
    } else {
        17
    };
    print!("{query}");
}

which doesn't compile.

I also get false positives on thread_local!{} macros, but I don't have a minimal reproduction. In my projects it lints on every thread_local!{} call, even those as simple as thread_local!(static SOMETHING: Cell<u32> = Cell::default()); but I don't get the lints in minimal projects.

[zjp-CN]

I'm hitting the same issue here after upgrading the nightly rustc and rust-analyzer.

rustc 1.78.0-nightly (a84bb95a1 2024-02-13)

rust-analyzer 2024-02-12

Here's the reproducible code.

use icu_segmenter::LineSegmenter;
use syntect::{
    highlighting::ThemeSet,
    parsing::SyntaxSet,
};

thread_local! {
   static SYNTHEME: (SyntaxSet, ThemeSet) = (
       SyntaxSet::load_defaults_newlines(),
       ThemeSet::load_defaults(),
   );
   static SEGMENTER: LineSegmenter = LineSegmenter::new_auto();
}

You can see the remove-unnecessary-else warnings on rustexplorer which utilizes the latest RA I guess.

BTW the warning disappears when the source file containing the above thread_local! is not open.

[ShoyuVanilla]

So, this is due to RA's check_for_unnecessary_else diagnostic; it checks whether the if-then block is never type and mark else block as unnecessary if so.
This is true for something like if "statements", but not holds for if "expressions", which has a value.
My PR fixes this and the RA build from that branch removes the false positive diagnostic for awused's example code (fn main) and still diagnosis the true ones as in fn foo

Yeah, it still marks thread_local as warning for zjp-CN's example code, but since the thread_local macro is expanded as (cargo-expand)

const SYNTHEME: ::std::thread::LocalKey<(SyntaxSet, ThemeSet)> = {
    #[inline]
    fn __init() -> (SyntaxSet, ThemeSet) {
        (SyntaxSet::load_defaults_newlines(), ThemeSet::load_defaults())
    }
    #[inline]
    unsafe fn __getit(
        init: ::std::option::Option<&mut ::std::option::Option<(SyntaxSet, ThemeSet)>>,
    ) -> ::std::option::Option<&'static (SyntaxSet, ThemeSet)> {
        #[thread_local]
        static __KEY: ::std::thread::local_impl::Key<(SyntaxSet, ThemeSet)> = ::std::thread::local_impl::Key::<
            (SyntaxSet, ThemeSet),
        >::new();
        unsafe {
            __KEY
                .get(move || {
                    if let ::std::option::Option::Some(init) = init {
                        if let ::std::option::Option::Some(value) = init.take() {
                            return value;
                        } else if true {
                            {
                                ::core::panicking::panic_fmt(
                                    format_args!(
                                        "internal error: entered unreachable code: {0}",
                                        format_args!("missing default value"),
                                    ),
                                );
                            };
                        }
                    }
                    __init()
                })
        }
    }
    unsafe { ::std::thread::LocalKey::new(__getit) }
};
const SEGMENTER: ::std::thread::LocalKey<LineSegmenter> = {
    #[inline]
    fn __init() -> LineSegmenter {
        LineSegmenter::new_auto()
    }
    #[inline]
    unsafe fn __getit(
        init: ::std::option::Option<&mut ::std::option::Option<LineSegmenter>>,
    ) -> ::std::option::Option<&'static LineSegmenter> {
        #[thread_local]
        static __KEY: ::std::thread::local_impl::Key<LineSegmenter> = ::std::thread::local_impl::Key::<
            LineSegmenter,
        >::new();
        unsafe {
            __KEY
                .get(move || {
                    if let ::std::option::Option::Some(init) = init {
                        if let ::std::option::Option::Some(value) = init.take() {
                            return value;
                        } else if true {
                            {
                                ::core::panicking::panic_fmt(
                                    format_args!(
                                        "internal error: entered unreachable code: {0}",
                                        format_args!("missing default value"),
                                    ),
                                );
                            };
                        }
                    }
                    __init()
                })
        }
    }
    unsafe { ::std::thread::LocalKey::new(__getit) }
};

and this is not wrong as itself since

                        if let ::std::option::Option::Some(value) = init.take() {
                            return value;
                        } else if true {
                        //^^^^ This `else` is unnecessary, indeed
                            {
                                ::core::panicking::panic_fmt(
                                    format_args!(
                                        "internal error: entered unreachable code: {0}",
                                        format_args!("missing default value"),
                                    ),
                                );
                            };
                        }

So, we should look for another approach for this thread_local case

[ShoyuVanilla]

So, we should look for another approach for this thread_local case

And I've opened a PR that fixes this from thread_local macro implementation code
rust-lang/rust#121098

[Noratrieb]

Happy to take the fix, but this is also an r-a bug, it shouldn't lint things from other crates like this (just like rustc does).

[awused]

Honestly, I'm not even sure the code is better after rust-lang/rust#121098. At best it's arguable, I think the changed code can make the meaning of the code less clear.

if foo {
    return bar;
} else {
    do_something_else();
}

changing to

if foo {
    return bar;
}
do_something_else();

is fine.

But this code snippet

if foo {
    return bar;
} else if bar {
    do_something_else();
}

becoming

if foo {
    return bar;
}
if bar {
    do_something_else();
}

makes it less obvious that the intended condition for the second branch is !foo && bar. This might cause confusion later during changes or refactors, not to mention it increases the line count without increasing clarity. I'm not sure if it should fire on else-if branches.

[ShoyuVanilla]

Happy to take the fix, but this is also an r-a bug, it shouldn't lint things from other crates like this (just like rustc does).

Thanks, I'll take a look into the rustc's diagnostics on foreign crates' macro expansions. Maybe I could get some clues from them suitable for here

[ShoyuVanilla]

This might cause confusion later during changes or refactors, not to mention it increases the line count without increasing clarity. I'm not sure if it should fire on else-if branches.

That makes sense. I've seen many lsp/linters making warnings about similar if-else things and sometimes they are irritating and not seem to be "improved" by suggested changes.

Are there some idiomatic agreements or decisions about this within rust analyzer maintainers or rust community?

[zjp-CN]

Thanks for looking into this! @ShoyuVanilla

The thread_local case can be minimized as follows

thread_local! {
    static A : Vec<u8> = Vec::from([1]);
}

I tried this snippet last night, but RA didn't report remove-unnecessary-else. At present, I tried again, this time RA does emit it 😮 (I didn't change anything about rustc and RA neither the configs)
Maybe I've missed something...

[ShoyuVanilla]

I tried this snippet last night, but RA didn't report remove-unnecessary-else. At present, I tried again, this time RA does emit it 😮 (I didn't change anything about rustc and RA neither the configs)
Maybe I've missed something...

Since the thread_local! macro is expanded in multiple ways as you can see here, some thread local usages - e.g. const initializations - would be expanded into codes without if { return; } else if ... { ... }.
If your snippets in both tries are identical, this would be not the case, of course.
I suspect that the rust analyzer was busy running other things and could not show you the diagnostics in time on your previous try, but it maybe not the case, either 🤔

[ergl]

I see this issue has been closed by #16590, but I wonder if it also fixes the following, which just happened to me today:

rust-analyzer version: rust-analyzer 1.75.0-beta.5 (1a06ac5b5d7 2023-12-01)

rustc version: rustc 1.74.1 (a28077b28 2023-12-04)

Before (rewritten code just to show the faulty behaviour):

fn test() {
    let inner = || false;
    let mut count = 0;
    while count == 0 {
        if true {
            1;
            continue;
        } else { // Warning here
            count += 1;
            if !inner() {
                return;
            }
        }
    }
}

After:

fn test() {
    let inner = || false;
    let mut count = 0;
    while count == 0 {
        if true {
            1;
            continue;
        }
        count += 1;
    }
}

The call to inner has been removed, making the function do something else entirely. The correct fix would be the following:

// ...
if true {
    1;
    continue;
}
count += 1;
if !inner() {
    return;
}
// ...

[ShoyuVanilla]

@ergl I guess so because of this commit, which is included in that PR.
Sorry but I can't test it myself 'cause I'm outside now 😅