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 SEMICOLON_IN_EXPRESSIONS_FROM_MACROS lint #79819

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Dec 8, 2020

cc #79813

This PR adds an allow-by-default future-compatibility lint
SEMICOLON_IN_EXPRESSIONS_FROM_MACROS. It fires when a trailing semicolon in a
macro body is ignored due to the macro being used in expression
position:

macro_rules! foo {
    () => {
        true; // WARN
    }
}

fn main() {
    let val = match true {
        true => false,
        _ => foo!()
    };
}

The lint takes its level from the macro call site, and
can be allowed for a particular macro by adding
#[allow(macro_trailing_semicolon)].

The lint is set to warn for all internal rustc crates (when being built
by a stage1 compiler). After the next beta bump, we can enable
the lint for the bootstrap compiler as well.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Dec 8, 2020
@Aaron1011 Aaron1011 force-pushed the feature/macro-trailing-semicolon branch from f8734e0 to c138cfb Compare December 8, 2020 02:05
@Aaron1011 Aaron1011 force-pushed the feature/macro-trailing-semicolon branch from c138cfb to 1d8d56e Compare December 8, 2020 02:51
@Aaron1011
Copy link
Member Author

This becomes tricky when the macro is defined in a different crate. I think we need to instead take the lint level from the macro call site - otherwise, the warning becomes unsurpressable in downstream code.

@Aaron1011
Copy link
Member Author

Unfortunately, inert attributes on macro invocations (e.g. #[allow(macro_trailing_semicolon)] my_macro!()) is an issue of its own: #63221

@Aaron1011
Copy link
Member Author

Additionally, linting is currently done after HIR lowering, so the macro call doesn't even exist by the time we would be ready to emit the lint.

Implementing this will require some further thought.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@petrochenkov
Copy link
Contributor

I agree that the NodeId for linting should be taken from the use site rather than from definition site.

We have a method fn lint_node_id to obtain an ID that is close enough to the given macro invocation.
(#[allow(macro_trailing_semicolon)] my_macro!() still won't work though.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2020
@Dylan-DPC-zz
Copy link

@bors try

@bors
Copy link
Contributor

bors commented Jan 13, 2021

⌛ Trying commit 1d8d56e95636056cf085c02decd7a386264310d3 with merge 50473808b4098f3d96d962b3940d38962305e408...

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@Aaron1011 Aaron1011 force-pushed the feature/macro-trailing-semicolon branch from 1d8d56e to 2603555 Compare January 27, 2021 19:39
@Aaron1011
Copy link
Member Author

@petrochenkov I've updated the PR to use lint_node_id

@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2021
compiler/rustc_expand/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/expand.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

The comment on macro_rules! declare_lint explains how to fix and test the lint-docs check.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2021
@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 28, 2021
@Aaron1011 Aaron1011 force-pushed the feature/macro-trailing-semicolon branch from 96c5f6b to 4867fad Compare January 28, 2021 13:50
cc rust-lang#79813

This PR adds an allow-by-default future-compatibility lint
`SEMICOLON_IN_EXPRESSIONS_FROM_MACROS`. It fires when a trailing semicolon in a
macro body is ignored due to the macro being used in expression
position:

```rust
macro_rules! foo {
    () => {
        true; // WARN
    }
}

fn main() {
    let val = match true {
        true => false,
        _ => foo!()
    };
}
```

The lint takes its level from the macro call site, and
can be allowed for a particular macro by adding
`#[allow(semicolon_in_expressions_from_macros)]`.

The lint is set to warn for all internal rustc crates (when being built
by a stage1 compiler). After the next beta bump, we can enable
the lint for the bootstrap compiler as well.
@Aaron1011 Aaron1011 force-pushed the feature/macro-trailing-semicolon branch from 4867fad to f902551 Compare January 28, 2021 13:52
@Aaron1011
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit f902551 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 28, 2021
…micolon, r=petrochenkov

Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` lint

cc rust-lang#79813

This PR adds an allow-by-default future-compatibility lint
`SEMICOLON_IN_EXPRESSIONS_FROM_MACROS`. It fires when a trailing semicolon in a
macro body is ignored due to the macro being used in expression
position:

```rust
macro_rules! foo {
    () => {
        true; // WARN
    }
}

fn main() {
    let val = match true {
        true => false,
        _ => foo!()
    };
}
```

The lint takes its level from the macro call site, and
can be allowed for a particular macro by adding
`#[allow(macro_trailing_semicolon)]`.

The lint is set to warn for all internal rustc crates (when being built
by a stage1 compiler). After the next beta bump, we can enable
the lint for the bootstrap compiler as well.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#79570 (rustc: Stabilize `-Zrun-dsymutil` as `-Csplit-debuginfo`)
 - rust-lang#79819 (Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` lint)
 - rust-lang#79991 (rustdoc: Render HRTB correctly for bare functions)
 - rust-lang#80215 (Use -target when linking binaries for Mac Catalyst)
 - rust-lang#81158 (Point to span of upvar making closure FnMut)
 - rust-lang#81176 (Improve safety of `LateContext::qpath_res`)
 - rust-lang#81287 (Split rustdoc JSON types into separately versioned crate)
 - rust-lang#81306 (Fuse inner iterator in FlattenCompat and improve related tests)
 - rust-lang#81333 (clean up some const error reporting around promoteds)
 - rust-lang#81459 (Fix rustdoc text selection for page titles)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4003a73 into rust-lang:master Jan 29, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 29, 2021
notheotherben added a commit to SierraSoftworks/rex-rs that referenced this pull request Feb 1, 2021
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jul 4, 2021
Opening to gather data about the impact of changing the lint level of rust-lang#79819
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants