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

Allow a reason="foo" in lint attributes #9025

Closed
wants to merge 3 commits into from
Closed

Allow a reason="foo" in lint attributes #9025

wants to merge 3 commits into from

Conversation

emberian
Copy link
Member

@emberian emberian commented Sep 6, 2013

No description provided.

Also add a lint that asserts that all lint attributes have a reason (allow by
default).

Closes #3358
@emberian
Copy link
Member Author

emberian commented Sep 6, 2013

r? @brson

@alexcrichton
Copy link
Member

It seems a bit odd to have a reason without actually using it anywhere. It seems like the only use case for this would be to have #[<lint level>(no_lint_reason)] to make sure that you justify all your lints. This never actually uses the message, although I suppose it does show up in the NOTE (but not guaranteed).

Also, would you mind adding some tests for lint attributes? They're easy to become wrong over time.

@emberian
Copy link
Member Author

emberian commented Sep 6, 2013

@alexcrichton well when it shows "lint level set here", the reason line will be there.

@emberian
Copy link
Member Author

emberian commented Sep 6, 2013

@alexcrichton is that test sufficient?

@alexcrichton
Copy link
Member

Could you add some controls with scope? Just to make sure that it picks up changes in the lint level accordingly. Something like #[allow(...)] and then some without reasons.

@emberian
Copy link
Member Author

emberian commented Sep 6, 2013

I don't understand what you're asking for.

On Fri, Sep 6, 2013 at 2:13 PM, Alex Crichton notifications@github.comwrote:

Could you add some controls with scope? Just to make sure that it picks up
changes in the lint level accordingly. Something like #[allow(...)] and
then some without reasons.


Reply to this email directly or view it on GitHubhttps://github.com//pull/9025#issuecomment-23958899
.

@alexcrichton
Copy link
Member

#[deny(no_lint_reason)];

#[allow(no_lint_reason)]
mod foo {
  #[warn(unused_unsafe)];
}

should compile or something like that

@emberian
Copy link
Member Author

emberian commented Sep 6, 2013

#[deny(no_lint_reason, reason="")];

#[allow(no_lint_reason, reason="")]
mod foo {
    #[warn(unused_unsafe)];
}

fn main() { }

compiles; perhaps this isn't quite the desired functionality though.

@emberian
Copy link
Member Author

emberian commented Sep 8, 2013

@alexcrichton is this now acceptable?

@alexcrichton
Copy link
Member

The code itself looks fine (a few stray error! statements though), but I'm not entirely convinced that this is necessary. I'd be curious what others thought about this as well.

@catamorphism
Copy link
Contributor

I'll add this to the agenda for the next meeting.

@catamorphism
Copy link
Contributor

We discussed this at the meeting and agreed the pull request is good, but it's not compelling enough of a feature to include. Sorry!

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2022
unused_async: lint async methods

Now lints:

```rust
impl Foo {
    async fn method(&self) -> &'static str {
        "no await here"
    }
}
```

changelog: [`unused_async`]: lint async methods

Fixes rust-lang#9024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants