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

Turn declare_clippy_lint into a declarative macro #13359

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Sep 6, 2024

Ease of development, and hopefully compile times (the dependencies are still there because of ui-test). The procedural macro was doing just some very basic processing (like assigning a lint level to each category), so it didn't have a reason to stay IMO

changelog: None

@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2024

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 6, 2024
@blyxyas blyxyas force-pushed the declare_clippy_macro branch from 7fbe95f to f58aae5 Compare September 6, 2024 12:17
@xFrednet
Copy link
Member

xFrednet commented Sep 6, 2024

Maybe:
r? @Alexendoo since I believe that you developed the macro originally?

@rustbot rustbot assigned Alexendoo and unassigned dswij Sep 6, 2024
@Alexendoo
Copy link
Member

Recompile time seems pretty similar, maybe try ${concat} instead of paste but my impression is proc macros aren't really slow

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be in the clippy_utils crate, as this is not something users of that crate should use.

Copy link
Member Author

@blyxyas blyxyas Sep 6, 2024

Choose a reason for hiding this comment

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

So, should it be in clippy_lints/lib.rs? Or somewhere else maybe

Copy link
Member

Choose a reason for hiding this comment

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

In clippy_lints probably yeah. But not necessarily in lib.rs. But lib.rs should at least re-export it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving it from clippy_utils to clippy_lints would need prepending all lint files with use crate::declare_clippy_lint. It can be done a simple bash script, but is it what we want?

I'm not sure why this isn't the case anymore, but when the macro was in clippy_utils just auto-imported it into the lint modules.

Copy link
Member

Choose a reason for hiding this comment

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

It's because of #[macro_use]

#[macro_use]
extern crate clippy_utils;

To do the same for a local macro you can either #[macro_export] it or put it in a #[macro_use] module

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's because of the #[macro_use], but giving it to another module is not working. (I've tried both with all combinations of #[macro_export] on the macro and #[macro_use] on the module)

I've tried both in clippy_lints/lib.rs, in a separate file and in the clippy_lints::utils module. Still, not working :/

Copy link
Member

Choose a reason for hiding this comment

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

Oh there's also that it needs to be defined above the lint modules that use the macro for some reason

@blyxyas
Copy link
Member Author

blyxyas commented Sep 6, 2024

maybe try ${concat} instead of paste

Before using paste, I tried with concat_idents, we cannot declare a value with static and concat_idents (nor do any macro magic for this to happen AFAIK)

proc macros aren't really slow

In the common nightly the diference isn't all that great, I've just tried with the parallel compiler (-Z threads=8) and the difference is 23 seconds, both from cargo clean.

@Alexendoo
Copy link
Member

Alexendoo commented Sep 6, 2024

${concat} does not have the same problems concat_idents does

@blyxyas
Copy link
Member Author

blyxyas commented Sep 6, 2024

Fixed, I didn't know about ${concat}, seems very useful (and possibly a general replacement for paste!)

@blyxyas blyxyas force-pushed the declare_clippy_macro branch from a63f367 to f58aae5 Compare September 10, 2024 11:37
@blyxyas
Copy link
Member Author

blyxyas commented Sep 13, 2024

Oh, something I forgot to mention. The main motivator behind this change is rust-lang/rust#125116. I need to mark some lints as always evaluated (so that, even if they are allow by default and are not manually triggered, they still run).

The proc macro API is hard to work with, and I wasn't getting to the point of accepting "[eval_always: true]" into the proc macro optionally. That's the reason I translated it, and most of the reason.

@bors
Copy link
Contributor

bors commented Sep 22, 2024

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

clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
Comment on lines 17 to 15
rustc_session::declare_tool_lint! {
$(#[doc = $lit])*
#[clippy::version = $version_lit]
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is a bit off here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

The parts in declare_tool_lint! { should be indented

clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
@blyxyas blyxyas force-pushed the declare_clippy_macro branch 2 times, most recently from c4d3198 to d562708 Compare September 23, 2024 01:12
clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
clippy_lints/src/declare_clippy_lint.rs Outdated Show resolved Hide resolved
@blyxyas blyxyas force-pushed the declare_clippy_macro branch from d562708 to 4997ee7 Compare September 24, 2024 13:48
tests/compile-test.rs Outdated Show resolved Hide resolved
@blyxyas blyxyas force-pushed the declare_clippy_macro branch from 9e840fa to 13e2633 Compare October 1, 2024 17:43
@blyxyas
Copy link
Member Author

blyxyas commented Oct 10, 2024

Is there anything more to be done? I'm waiting for the merge & sync to resolve this conversation

@Alexendoo
Copy link
Member

👍

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 13e2633 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 13e2633 with merge 8125cd5...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 8125cd5 to master...

@bors bors merged commit 8125cd5 into rust-lang:master Oct 11, 2024
8 checks passed
bors added a commit that referenced this pull request Oct 26, 2024
Fix indentation of website code snippets

Fixes #13568

Follow up to #13359, I didn't catch that it swapped the `strip_prefix` out for a `trim` but they aren't interchangeable here

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants