Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macro use sugg #5279

Merged
merged 10 commits into from
Jun 9, 2020
Merged

Macro use sugg #5279

merged 10 commits into from
Jun 9, 2020

Conversation

DevinR528
Copy link
Contributor

@DevinR528 DevinR528 commented Mar 6, 2020

changelog: Add auto applicable suggstion to macro_use_imports

fixes #5275

Where exactly is the wildcard_imports_helper I haven't been able to import anything ex.
use lazy_static; or something like for that I get version/compiler conflicts?

Found it.

Should we also check for #[macro_use] extern crate, although this is still depended on for stuff like rustc_private?

Comment on lines 100 to 122
for kid in lcx.tcx.item_children(id).iter() {
if let Res::Def(DefKind::Macro(_mac_type), mac_id) = kid.res {
let span = mac_attr.span.clone();
println!("{:#?}", lcx.tcx.def_path_str(mac_id));
self.imports.push((lcx.tcx.def_path_str(mac_id), span));
}
}
Copy link
Contributor Author

@DevinR528 DevinR528 Mar 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work exactly how we need it to, so far it ignores all the modules the macro they are defined in but not found in. I need to figure out how to import from another crate into my test crate so I can have the macro stick in a module.

Note: since the prelude import is shadowed by the (builtin?) macro prelude import it doesn't catch that any more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For macros from another crate you can use the mini-macro subcrate in the Clippy repo: https://github.com/rust-lang/rust-clippy/tree/master/mini-macro

This is also used in some other tests and includes a few proc-macros.

@flip1995
Copy link
Member

flip1995 commented Mar 9, 2020

Since I'm not really sure, how far you're along with implementing this, can you give me a short summary, what you already implemented, what you think needs to be added, and which questions you still might have, before you can continue?

@DevinR528
Copy link
Contributor Author

DevinR528 commented Mar 9, 2020

It catches anything from any level of an imported crate directly imported

// crate importing from "macros"
extern crate macro_rules;

#[macro_export]
macro_rules! pub_macro {
    () => {
        ()
    };
}
pub mod inner {
    // RE-EXPORT
    // this will stick in `inner` module
    pub use macro_rules::try_err;

    #[macro_export]
    macro_rules! inner_mod {
        () => {
            #[allow(dead_code)]
            pub struct Tardis;
        };
    }
}

// using in this crate 
#[macro_use]
use mac;

fn main() {
    pub_macro!();
    inner_mod!();
    // without the inner:: it fails to compile?
    inner::try_err!();
}

it correctly ignores modules only using the root/crate name, it also aliases correctly so it will use the renamed name. The re-export works (it will produce a suggestion of macro::inner::try_err) but only if I call it like inner::try_err!(), otherwise it fails to compile can't resolve macro try_err. This seems more like me setting up something wrong in the auxiliary crate folder though?

By iterating over the children of an imported Mod we miss std::prelude so the original test of println fails. I'd imagine not many people are #[macro_use] std::prelude so maybe that's ok?

I can update the .stderr file that may make it easier to see what I've done?

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 9, 2020
@bors
Copy link
Contributor

bors commented Mar 10, 2020

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

@DevinR528 DevinR528 force-pushed the macro-use-sugg branch 2 times, most recently from 530fc20 to a46ff1f Compare March 18, 2020 02:46
@DevinR528
Copy link
Contributor Author

DevinR528 commented Mar 18, 2020

Since we know if we encountered a macro that wasn't found in the imports search, and therefor is one of the weird import kinds (std::prelude). Could we defer all the span_lint_and_suggest calls until this point pushing them into a Vec then once we know there are no weird macro imports we emit all the suggestions as machine fixable.

how they are gathered
if lcx.sess().opts.edition == Edition::Edition2018;
if let hir::ItemKind::Use(path, _kind) = &item.kind;
if let Some(mac_attr) = item
    .attrs
    .iter()
    .find(|attr| attr.ident().map(|s| s.to_string()) == Some("macro_use".to_string()));
if let Res::Def(DefKind::Mod, id) = path.res;
then {
    for kid in lcx.tcx.item_children(id).iter() {
        if let Res::Def(DefKind::Macro(_mac_type), mac_id) = kid.res {
            let span = mac_attr.span.clone();
            self.imports.push((lcx.tcx.def_path_str(mac_id), span));
        }
    }
}
where this would happen
fn check_crate_post(&mut self, lcx: &LateContext<'_, '_>, _krate: &hir::Crate<'_>) {
    for (import, span) in self.imports.iter() {
        let matched = self
            .mac_refs
            .iter()
            .find(|(_span, mac)| import.ends_with(&mac.name))
            .is_some();

        if matched {
            self.mac_refs.retain(|(_span, mac)| !import.ends_with(&mac.name));
            let msg = "`macro_use` attributes are no longer needed in the Rust 2018 edition";
            let help = format!("use {}", import);
            span_lint_and_sugg(
                lcx,
                MACRO_USE_IMPORTS,
                *span,
                msg,
                "remove the attribute and import the macro directly, try",
                help,
                Applicability::HasPlaceholders,
            )
        }
    }
    if !self.mac_refs.is_empty() {
        // TODO if not empty we found one we could not make a suggestion for
        // such as std::prelude::v1 or something else I haven't thought of.
        // If we defer the calling of span_lint_and_sugg we can make a decision about its
        // applicability?
    }
}

@bors
Copy link
Contributor

bors commented Mar 23, 2020

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

@flip1995 flip1995 self-requested a review March 30, 2020 19:11
@bors
Copy link
Contributor

bors commented Mar 30, 2020

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

@DevinR528 DevinR528 force-pushed the macro-use-sugg branch 2 times, most recently from d611683 to 81048cd Compare April 10, 2020 10:35
@flip1995
Copy link
Member

@DevinR528 Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently. I didn't forget this PR and want to review it in the coming days.

@bors
Copy link
Contributor

bors commented Apr 15, 2020

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

@bors
Copy link
Contributor

bors commented Apr 17, 2020

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

@bors
Copy link
Contributor

bors commented Apr 19, 2020

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

@bors
Copy link
Contributor

bors commented Apr 20, 2020

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

@bors
Copy link
Contributor

bors commented Apr 27, 2020

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

@bors
Copy link
Contributor

bors commented May 2, 2020

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

@DevinR528
Copy link
Contributor Author

Is this compile failure something I can fix?

error[E0624]: associated function `seek_after` is private
   --> clippy_lints/src/redundant_clone.rs:594:25
    |
594 |         self.maybe_live.seek_after(at);
    |                         ^^^^^^^^^^ private associated function

@flip1995
Copy link
Member

flip1995 commented May 4, 2020

#5566 We're currently figuring out some stuff with the move to subtree in the Rust repo, so a rustup may take a day or two more this time. A fix is ready, though.

@flip1995
Copy link
Member

flip1995 commented Jun 8, 2020

stderr differs? That is weird 🤔

@bors
Copy link
Contributor

bors commented Jun 8, 2020

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

@DevinR528
Copy link
Contributor Author

It's the ordering of the suggestions, hmmm I'll look into this...

@flip1995
Copy link
Member

flip1995 commented Jun 9, 2020

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jun 9, 2020

📌 Commit 3f91e39 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 9, 2020

⌛ Testing commit 3f91e39 with merge 0537c9d...

bors added a commit that referenced this pull request Jun 9, 2020
Macro use sugg

changelog: Add auto applicable suggstion to macro_use_imports

fixes #5275

<s>Where exactly is the `wildcard_imports_helper` I haven't been able to import anything ex.
`use lazy_static;` or something like for that I get version/compiler conflicts?</s>
Found it.

Should we also check for `#[macro_use] extern crate`, although this is still depended on for stuff like `rustc_private`?
@bors
Copy link
Contributor

bors commented Jun 9, 2020

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

flip1995 commented Jun 9, 2020

Wait the problem is, that the output is different on 32bit linux 🤔 Since only the order is different, you can add // ignore-32bit

@flip1995
Copy link
Member

flip1995 commented Jun 9, 2020

You also have to update the .fixed and .stderr files

@DevinR528
Copy link
Contributor Author

Oops duh 🤦

@flip1995
Copy link
Member

flip1995 commented Jun 9, 2020

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2020

📌 Commit e521a4e has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 9, 2020

⌛ Testing commit e521a4e with merge f065d4b...

@bors
Copy link
Contributor

bors commented Jun 9, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing f065d4b to master...

@bors bors merged commit f065d4b into rust-lang:master Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance macro_use_imports lint with a auto applicable suggstion
3 participants