instrument macro can make functions not compile as of 0.1.29

[jplatte]

Bug Report

Given this minimized example:

type Foo = ();
enum Bar {
    Foo,
}

fn this_is_fine() -> Foo {
    // glob import imports Bar::Foo, shadowing Foo
    use Bar::*;
}

adding #[tracing::instrument] to this_is_fine makes it no longer compile.

This used to work before #3108 (i.e. in tracing-attributes 0.1.28), and stopped working afterwards. rust-analyzer's expand macro assist produces identical output for both (there's a chance I failed to properly rebuild proc-macros in between, but I tried at least), so I'm pretty sure that this is a macro hygiene issue (just like the issue[s] fixed by #3108).

Any ideas what we could do @hds @9999years?

[hds]

@jplatte The macro expansion shouldn't be identical. At the very least, I believe you should see a difference in the allow attribute due to this change:

https://github.com/tokio-rs/tracing/pull/3108/files#diff-6761a09f21c6b3fb071909d44e37696801fa01bf2f92d3a5ce9608b64f2ff7deR72-R80

Which expansion did you see? Do you see clippy::needless_return in the list (it was added in #3108).

[hds]

It looks like we lost a set of braces which ensured that use Bar::* didn't affect any of the macro generated code. This is the previous expansion (taken from play.r

fn this_is_fine() -> Foo {
    {}

    #[allow(clippy :: suspicious_else_formatting)]
    {
        let __tracing_attr_span;
        let __tracing_attr_guard;
        if 
                // glob import imports Bar::Foo, shadowing Foo

                //tracing::info!(name: "hi", parent: None, "thing");
                tracing::Level::INFO <=
                        ::tracing::level_filters::STATIC_MAX_LEVEL &&
                    tracing::Level::INFO <=
                        ::tracing::level_filters::LevelFilter::current() ||
                { false } {
            __tracing_attr_span =
                {
                    use ::tracing::__macro_support::Callsite as _;
                    static __CALLSITE: ::tracing::callsite::DefaultCallsite =
                        {
                            static META: ::tracing::Metadata<'static> =
                                {
                                    ::tracing_core::metadata::Metadata::new("this_is_fine",
                                        "playground", tracing::Level::INFO,
                                        ::tracing_core::__macro_support::Option::Some("src/main.rs"),
                                        ::tracing_core::__macro_support::Option::Some(6u32),
                                        ::tracing_core::__macro_support::Option::Some("playground"),
                                        ::tracing_core::field::FieldSet::new(&[],
                                            ::tracing_core::callsite::Identifier(&__CALLSITE)),
                                        ::tracing::metadata::Kind::SPAN)
                                };
                            ::tracing::callsite::DefaultCallsite::new(&META)
                        };
                    let mut interest = ::tracing::subscriber::Interest::never();
                    if tracing::Level::INFO <=
                                        ::tracing::level_filters::STATIC_MAX_LEVEL &&
                                    tracing::Level::INFO <=
                                        ::tracing::level_filters::LevelFilter::current() &&
                                { interest = __CALLSITE.interest(); !interest.is_never() }
                            &&
                            ::tracing::__macro_support::__is_enabled(__CALLSITE.metadata(),
                                interest) {
                        let meta = __CALLSITE.metadata();
                        ::tracing::Span::new(meta,
                            &{ meta.fields().value_set(&[]) })
                    } else {
                        let span =
                            ::tracing::__macro_support::__disabled_span(__CALLSITE.metadata());
                        {};
                        span
                    }
                };
            __tracing_attr_guard = __tracing_attr_span.enter();
        }

        #[warn(clippy :: suspicious_else_formatting)]
        {

            #[allow(unknown_lints, unreachable_code, clippy ::
            diverging_sub_expression, clippy :: let_unit_value, clippy ::
            unreachable, clippy :: let_with_type_underscore, clippy ::
            empty_loop)]
            if false {
                let __tracing_attr_fake_return: Foo = loop {};
                return __tracing_attr_fake_return;
            }
            {
                use Bar::*;
            }
        }
    }
}

And this is the new expansion:

fn this_is_fine() -> Foo {
    {}
    #[allow(clippy::suspicious_else_formatting)]
    {
        let __tracing_attr_span;
        let __tracing_attr_guard;
        if (::tracing::Level::INFO) <= tracing::level_filters::STATIC_MAX_LEVEL
            && (::tracing::Level::INFO) <= tracing::level_filters::LevelFilter::current()
            || if match (::tracing::Level::INFO) {
                tracing::Level::ERROR => tracing::log::Level::Error,
                tracing::Level::WARN => tracing::log::Level::Warn,
                tracing::Level::INFO => tracing::log::Level::Info,
                tracing::Level::DEBUG => tracing::log::Level::Debug,
                _ => tracing::log::Level::Trace,
            } <= tracing::log::STATIC_MAX_LEVEL
            {
                #[allow(unused_braces)]
                {
                    true
                }
            } else {
                {
                    false
                }
            }
        {
            __tracing_attr_span = {
                use tracing::__macro_support::Callsite as _;
                static __CALLSITE: tracing::callsite::DefaultCallsite = {
                    static META: tracing::Metadata<'static> = {
                        tracing_core::metadata::Metadata::new(
                            "this_is_fine",
                            (module_path!()),
                            (::tracing::Level::INFO),
                            tracing_core::__macro_support::Option::Some(
                                tracing_core::__macro_support::file!(),
                            ),
                            tracing_core::__macro_support::Option::Some(0u32),
                            tracing_core::__macro_support::Option::Some(
                                tracing_core::__macro_support::module_path!(),
                            ),
                            tracing_core::field::FieldSet::new(
                                (&[]),
                                tracing_core::callsite::Identifier((&__CALLSITE)),
                            ),
                            (tracing::metadata::Kind::SPAN),
                        )
                    };
                    tracing::callsite::DefaultCallsite::new(&META)
                };
                let mut interest = tracing::subscriber::Interest::never();
                if (::tracing::Level::INFO) <= tracing::level_filters::STATIC_MAX_LEVEL
                    && (::tracing::Level::INFO) <= tracing::level_filters::LevelFilter::current()
                    && {
                        interest = __CALLSITE.interest();
                        !interest.is_never()
                    }
                    && tracing::__macro_support::__is_enabled(__CALLSITE.metadata(), interest)
                {
                    let meta = __CALLSITE.metadata();
                    tracing::Span::new(meta, &{ (meta.fields()).value_set(&[]) })
                } else {
                    let span = tracing::__macro_support::__disabled_span(__CALLSITE.metadata());
                    if match (::tracing::Level::INFO) {
                        tracing::Level::ERROR => tracing::log::Level::Error,
                        tracing::Level::WARN => tracing::log::Level::Warn,
                        tracing::Level::INFO => tracing::log::Level::Info,
                        tracing::Level::DEBUG => tracing::log::Level::Debug,
                        _ => tracing::log::Level::Trace,
                    } <= tracing::log::STATIC_MAX_LEVEL
                    {
                        #[allow(unused_braces)]
                        {
                            span.record_all(&{ (__CALLSITE.metadata().fields()).value_set(&[]) });
                        }
                    } else {
                        {}
                    };
                    span
                }
            };
            __tracing_attr_guard = __tracing_attr_span.enter();
        }
        #[warn(clippy::suspicious_else_formatting)]
        {
            #[allow(
                unknown_lints,
                unreachable_code,
                clippy::diverging_sub_expression,
                clippy::empty_loop,
                clippy::let_unit_value,
                clippy::let_with_type_underscore,
                clippy::needless_return,
                clippy::unreachable
            )]
            if false {
                let __tracing_attr_fake_return: Foo = loop {};
                return __tracing_attr_fake_return;
            }
            use Bar::*;
        }
    }
}

The problem is that previously the body of the function was in a separate (inner) scope from if false ... and now it isn't.

Before:

            if false {
                let __tracing_attr_fake_return: Foo = loop {};
                return __tracing_attr_fake_return;
            }
            {
                use Bar::*;
            }

After:

            if false {
                let __tracing_attr_fake_return: Foo = loop {};
                return __tracing_attr_fake_return;
            }
            use Bar::*;

[jplatte]

Oh, right. I'll try it again today.