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

new lint: unused_unit #3233

Merged
merged 1 commit into from
Oct 13, 2018
Merged

new lint: unused_unit #3233

merged 1 commit into from
Oct 13, 2018

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 28, 2018

This is the rejected unused_parens extension plus a lint for units in function return types and return expressions.

It appears to fail to obey the allow(..) attributes though (see copies.rs) – does anyone know why that happens?

@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 29, 2018
@flip1995
Copy link
Member

Maybe the ignored allow(..) is related to #3198. Something weird is going on with defining lint levels. I didn't find the time to investigate this further yet.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 29, 2018

Maybe the lint level selection was broken when tools lints were added?

@flip1995
Copy link
Member

I thought the same, so I searched for merge commits by @bors that broke the lint level definition and I got it down to 3 commits: #3198 (comment). None of these commits are related to tool_lints though..

@llogiq
Copy link
Contributor Author

llogiq commented Oct 8, 2018

I have extended the lint to cover explicit unit return types in fn types and Fn* trait bounds. Apart from the lint level selection issue, this should be ready to merge.

@llogiq llogiq force-pushed the unused-unit branch 2 times, most recently from 2c413d8 to 82686a8 Compare October 12, 2018 10:25
@llogiq
Copy link
Contributor Author

llogiq commented Oct 12, 2018

I had missed adding the lint to the lint-array, which resulted in the failing allow annotation. This should hopefully now work. r? once CI is happy.

@llogiq llogiq force-pushed the unused-unit branch 2 times, most recently from 4568461 to f9732ef Compare October 12, 2018 14:39
@llogiq
Copy link
Contributor Author

llogiq commented Oct 12, 2018

@oli-obk r?

@@ -68,6 +68,25 @@ declare_clippy_lint! {
the end of a block"
}

/// **What it does:** Checks for unit (`()`) expressions that can be removed
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe too pedantic but there is no dot at the end of the line.

@oli-obk oli-obk merged commit 8b12eee into master Oct 13, 2018
@oli-obk oli-obk deleted the unused-unit branch October 13, 2018 07:30
@aldanor
Copy link

aldanor commented Nov 13, 2018

@llogiq This seems to act weird in cases like this:

thread_local!(static A: () = ());

yields

warning: unneeded unit expression
   |
20 | thread_local!(static A: () = ());
   |                              ^^ help: remove the final `()`

... and allow(clippy::unused_unit) doesn't seem to work in this case (only if done at the module level).

@llogiq
Copy link
Contributor Author

llogiq commented Nov 13, 2018

True, that looks weird. Then again, why do you need a zero-sized thread-local?

@aldanor
Copy link

aldanor commented Nov 13, 2018

Then again, why do you need a zero-sized thread-local?

You'd be surprised :) For identifying threads when implementing a reentrant mutex.

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.

6 participants