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 #5230

Merged
merged 1 commit into from Mar 5, 2020
Merged

Macro use #5230

merged 1 commit into from Mar 5, 2020

Conversation

DevinR528
Copy link
Contributor


changelog: This lint enforces Rust 2018 idiom of importing macro's directly without #[macro_use] fixes #5179 .

@DevinR528
Copy link
Contributor Author

DevinR528 commented Feb 27, 2020

I can just turn the "use {}::<macro name>" now. I did attempt to impl the suggestion portion of this and got as far as collecting all the use imports, and macro "references" but see no way of linking them together without ast::visit::Visitor as done in librustc_resolve they even check for the macro_use but only for unused imports.

Should I mess with the failed test this (as in the error below) should not be there and didn't fail on my machine, its from missing-docs?

error[E0277]: the size for values of type `Self` cannot be known at compilation time
  --> $DIR/missing-doc-impl.rs:51:5
   |
LL | pub trait E {
   | ----------- required by `E`
LL |     type AssociatedType;
LL |     type AssociatedTypeDef = Self;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `Self`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

The CI failure is unrelated to this PR, should be fixed by #5231.

clippy_lints/src/macro_use.rs Outdated Show resolved Hide resolved
@DevinR528
Copy link
Contributor Author

DevinR528 commented Feb 27, 2020

I ran the commands the failing tests told be to run
tests/ui/update-references.sh '/home/devinr/aprog/rust/__forks__/rust-clippy/target/debug/test_build_base' 'regex.rs'
and
tests/ui/update-references.sh '/home/devinr/aprog/rust/__forks__/rust-clippy/target/debug/test_build_base' 'single_component_path_imports.rs'
hopefully that was ok.

@JohnTitor
Copy link
Member

A quick workaround for 4e18568 is cargo clean. Also you should rebase to make CI green.

@DevinR528
Copy link
Contributor Author

I cargo cleaned locally then git fetch upstrem and git rebase upstream/master?

@DevinR528
Copy link
Contributor Author

Yay finally thanks for the help getting that working @JohnTitor !

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Nice! I'm not sure we allow to include merge commit here though, it's up to the team. I left a nit point.

clippy_lints/src/macro_use.rs Outdated Show resolved Hide resolved
@DevinR528
Copy link
Contributor Author

Should I squash that commit, or the last three really into one update-reference.sh commit

@DevinR528
Copy link
Contributor Author

Dang sorry I can't seem to get a good test run going they all pass locally this time there were a bunch of ICE errors. The 6 tests that failed did so with

error: internal compiler error: src/librustc_mir/dataflow/generic/engine.rs:315: Switch on discriminant of non-ADT

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:881:9

@JohnTitor
Copy link
Member

It isn't your fault, I can reproduce them on master. Let me take a look.

@DevinR528 DevinR528 force-pushed the macro-use branch 2 times, most recently from 4bde11f to be6c313 Compare March 2, 2020 21:14
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 2, 2020
clippy_lints/src/macro_use.rs Outdated Show resolved Hide resolved
then {
let msg = "`macro_use` attribute's are no longer needed in the Rust 2018 edition";
let help = format!(
"remove the attribute and import the macro directly `use {}::<macro name>`",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, you should use span_lint_and_sugg with Applicability::HasPlaceholders

clippy_lints/src/macro_use.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 3, 2020
@DevinR528
Copy link
Contributor Author

Any idea why this timed out, all the tests seemed to pass?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Please remove the empty macro_use_import.stdout file

/// **Why is this bad?** Since the Rust 2018 edition you can import
/// macro's directly, this is considered idiomatic.
///
/// **Known problems:** None.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add to the known problems, that the suggestion is not auto-applicable.

@flip1995
Copy link
Member

flip1995 commented Mar 3, 2020

Don't worry about the timeout. GHA failed here I guess. (didn't start a new run for the force push, but canceled the prev run because of a new commit)

@flip1995
Copy link
Member

flip1995 commented Mar 3, 2020

Thanks! Impl looks really clean

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 3, 2020

📌 Commit 2840f68 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Mar 3, 2020

⌛ Testing commit 2840f68 with merge 3f8656c...

bors added a commit that referenced this pull request Mar 3, 2020
Macro use

---

changelog: This lint enforces Rust 2018 idiom of importing macro's directly without `#[macro_use]` fixes #5179 .
@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 3, 2020
@bors
Copy link
Collaborator

bors commented Mar 3, 2020

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2020

@bors retry

@bors
Copy link
Collaborator

bors commented Mar 4, 2020

⌛ Testing commit 2840f68 with merge b791260...

bors added a commit that referenced this pull request Mar 4, 2020
Macro use

---

changelog: This lint enforces Rust 2018 idiom of importing macro's directly without `#[macro_use]` fixes #5179 .
@bors
Copy link
Collaborator

bors commented Mar 4, 2020

💔 Test failed - checks-action_dev_test

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2020

cargo dev update_lints

@DevinR528
Copy link
Contributor Author

Do I need to pull first, when I ran it it made no changes

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2020

Oh yeah, you need to rebase first probably.

@DevinR528
Copy link
Contributor Author

Thats odd it fails locally but passed in CI?

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2020

You accidentally pushed a binary file in 1601106. Can you remove this please?

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2020

Can you squash some commits please? Then this is good to go.

@flip1995
Copy link
Member

flip1995 commented Mar 5, 2020

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Mar 5, 2020

📌 Commit 597e02d has been approved by flip1995

bors added a commit that referenced this pull request Mar 5, 2020
Macro use

---

changelog: This lint enforces Rust 2018 idiom of importing macro's directly without `#[macro_use]` fixes #5179 .
@bors
Copy link
Collaborator

bors commented Mar 5, 2020

⌛ Testing commit 597e02d with merge a6a8a95...

@bors
Copy link
Collaborator

bors commented Mar 5, 2020

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

flip1995 commented Mar 5, 2020

Don't worry about the failure. This is not a issue with Clippy/this PR.

@DevinR528
Copy link
Contributor Author

Thanks for all the help/input on this everyone!

@flip1995
Copy link
Member

flip1995 commented Mar 5, 2020

@bors retry

@bors
Copy link
Collaborator

bors commented Mar 5, 2020

⌛ Testing commit 597e02d with merge 23d2b21...

@bors
Copy link
Collaborator

bors commented Mar 5, 2020

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

@bors bors merged commit 23d2b21 into rust-lang:master Mar 5, 2020
bors added a commit that referenced this pull request Mar 5, 2020
Rename macro_use_import -> macro_use_imports

I missed this during review of #5230. We can just do this, without deprecating the old name, since this lint didn't hit nightly rustc yet.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: using #[macro_use] instead of a use statement
5 participants