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

Add macro_braces lint to check for irregular brace use in certain macros #7299

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

DevinR528
Copy link
Contributor

The name is a bit long but this sounds good as #[allow(unconventional_macro_braces)] and it seems more clear that we are talking about the macro call not macro definitions, any feedback let me know. Thanks!
fixes #7278

changelog: Add [`unconventional_macro_braces`] lint that checks for uncommon brace usage with macros.

@rust-highfive
Copy link

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 31, 2021
clippy_lints/src/unconventional_macro_braces.rs Outdated Show resolved Hide resolved
clippy_lints/src/unconventional_macro_braces.rs Outdated Show resolved Hide resolved
tests/ui/unconventional_macro_braces.stderr Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor

llogiq commented Jun 1, 2021

Agree with @jplatte 's comments. Also I'd consider "nonstandard" instead of "unconventional", it's slightly shorter and consistent with "nonstandard_style".

@DevinR528
Copy link
Contributor Author

Ahh thanks for the "nonstandard" I spent a half-hour trying to figure out a good word 😆!

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks mostly good, I only have a few comments, otherwise it's ready to merge.

clippy_lints/src/nonstandard_macro_braces.rs Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
@DevinR528 DevinR528 force-pushed the macro-brace branch 2 times, most recently from 95906be to 791d428 Compare June 3, 2021 19:53
@llogiq
Copy link
Contributor

llogiq commented Jun 5, 2021

The only thing left is the question whether configuration can change brace style (perhaps someone has their own style guide that differs from the standard).

@DevinR528
Copy link
Contributor Author

DevinR528 commented Jun 7, 2021

Do you mean since print is already defined as ( should we be able to override that {which is the way it currently works)? If so then I think yes users should be able to override the default MacroMatchers.

Edit: I realized that since both fields of the MacroMatcher were hashed you couldn't actually overwrite them. I fixed it to only hash the name field so they can be overwritten.

@bors
Copy link
Collaborator

bors commented Jun 10, 2021

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

@llogiq
Copy link
Contributor

llogiq commented Jun 10, 2021

Good! I like that this is also tested. I think once this is rebased, it's ready to merge.

@DevinR528
Copy link
Contributor Author

Should I squash? Oh also I thought maybe the lint should also check_ty in the EarlyLintPass macros in type position is a thing right?

@jplatte
Copy link
Contributor

jplatte commented Jun 10, 2021

Yes, macros in type position are a thing, e.g. syn::Token.

@llogiq
Copy link
Contributor

llogiq commented Jun 17, 2021

I'm not sure if bors does it automatically, but it probably won't hurt to squash.

@flip1995
Copy link
Member

bors merges as-is. So no auto-squashing.

Rename unconventional -> nonstandard, add config field

Add standard_macro_braces fields so users can specify macro names and
brace combinations to lint for in the clippy.toml file.

Fix errors caused by nonstandard_macro_braces in other lint tests

Fix users ability to override the default nonstandard macro braces

Add type position macros impl `check_ty`
@DevinR528
Copy link
Contributor Author

Squashed, thanks for all the help everyone!

@llogiq
Copy link
Contributor

llogiq commented Jun 19, 2021

Thank you! @bors r+

@bors
Copy link
Collaborator

bors commented Jun 19, 2021

📌 Commit 723f515 has been approved by llogiq

@bors
Copy link
Collaborator

bors commented Jun 19, 2021

⌛ Testing commit 723f515 with merge 3120b09...

@bors
Copy link
Collaborator

bors commented Jun 19, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 3120b09 to master...

@bors bors merged commit 3120b09 into rust-lang:master Jun 19, 2021
bors added a commit that referenced this pull request Jun 21, 2021
Fixed broken deploy script due to multiline configuration docs

The deploy script on master currently runs into an error (See [log](https://github.com/rust-lang/rust-clippy/runs/2865828873)) due to the new configuration documentation added in #7299. The current documentation collection for the configuration macro sadly doesn't support multiline doc comments. This will be changes in the future with the new metadata collector tracked in #7172 For now we have to use `<br>` inside doc comments to add paragraphs.

This PR restricts `define_Conf!` macro to single lines and adds a comment explaining the reasoning behind it. It also adjusted the actual document parsing to fix a bug. (The parsing was automatically stopping on the first curly bracket, even if it was part of a doc comment).

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.

New lint: Check quote! macro is always used with curyl braces
6 participants