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 several lints into unused lint group #45424

Merged
merged 1 commit into from
Oct 30, 2017
Merged

Conversation

petrochenkov
Copy link
Contributor

Also a couple of obsolete (not reported) lints are removed.

r? @oli-obk

@@ -140,8 +140,7 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> {
self.tcx.lint_node(CONST_ERR,
expr.id,
expr.span,
&format!("constant evaluation error: {}. This will \
become a HARD ERROR in the future",
&format!("constant evaluation error: {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oli-obk
I initially wanted to move const_err into the future_incompatible group based on this comment, but it looks like this comment was actually outdated and this lint is not going to be an error?

Copy link
Contributor

@oli-obk oli-obk Oct 21, 2017

Choose a reason for hiding this comment

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

Yea. These are essentially bugs

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 20, 2017

I've also audited the remaining ungrouped lints.
In principle new lint groups can be created for them, but it looks like fine-grained lint grouping didn't find its use even in clippy, so I didn't do anything. cc clippy people just in case @mcarton @llogiq

Unused++.
These can also be quite reasonably added into the unused group, but less obviously than those I added into it in this PR.

STABLE_FEATURES
RENAMED_AND_REMOVED_LINTS
UNKNOWN_LINTS
UNUSED_COMPARISONS

Bad style++.
Probably can be added into the bad_style group, but it currently consists only of casing-related lints.

NON_SHORTHAND_FIELD_PATTERNS
WHILE_TRUE

Future compatibility++.
Errors that are reported as lints for some reasons unknown to me.
See the question in #45424 (comment) as well.

CONST_ERR // ?
UNKNOWN_CRATE_TYPES // Deny-by-default
NO_MANGLE_CONST_ITEMS // Deny-by-default
NO_MANGLE_GENERIC_ITEMS

Restrictions.
Something generally reasonable that can be prohibited if necessary.

BOX_POINTERS // Allow-by-default
UNSAFE_CODE // Allow-by-default
UNSTABLE_FEATURES // Allow-by-default
MISSING_DOCS // Allow-by-default
MISSING_COPY_IMPLEMENTATIONS // Allow-by-default
MISSING_DEBUG_IMPLEMENTATIONS // Allow-by-default

Pedantic.
Something not bad enough to always report/fix.

UNUSED_RESULTS // Allow-by-default
UNUSED_IMPORT_BRACES // Allow-by-default
UNUSED_QUALIFICATIONS // Allow-by-default
TRIVIAL_CASTS // Allow-by-default
TRIVIAL_NUMERIC_CASTS // Allow-by-default
VARIANT_SIZE_DIFFERENCES // Allow-by-default
UNIONS_WITH_DROP_FIELDS

Obvious mistakes.
Prevent foot shooting, some can become hard errors in principle.

OVERFLOWING_LITERALS
EXCEEDING_BITSHIFTS // Deny-by-default
UNCONDITIONAL_RECURSION
MUTABLE_TRANSMUTES // Deny-by-default
IMPROPER_CTYPES
PLUGIN_AS_LIBRARY
PRIVATE_NO_MANGLE_FNS
PRIVATE_NO_MANGLE_STATICS

General purpose lints.

WARNINGS
DEPRECATED

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2017
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

The lint store has a function register_removed

@@ -621,12 +621,6 @@ impl EarlyLintPass for AnonymousParameters {
}
}

declare_lint! {
DEPRECATED_ATTR,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just removing these lints will cause users who mention them in allow/warn/deny statements to produce warnings about unknown lints. Does rustc have a removed lint list like clippy does?

Copy link
Contributor Author

@petrochenkov petrochenkov Oct 21, 2017

Choose a reason for hiding this comment

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

Yes, there's a list of removed lints, I'll add these lints into it.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 21, 2017

I would like fine grained grouping. I think we have an open issue in clippy. People often complain that they are overwhelmed by the style lints, when they want to get to the bug lints first

@bors
Copy link
Contributor

bors commented Oct 28, 2017

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

@kennytm kennytm 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 Oct 28, 2017
@petrochenkov
Copy link
Contributor Author

Rebased and updated with a fix for #45424 (comment)

@kennytm kennytm 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 Oct 29, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2017

Lgtm now. Do we have an issue to discuss the fine grained lint groups and not clear cut "unused"-lints? #45424 (comment) as an issue would be good I think

@bors r+

@bors
Copy link
Contributor

bors commented Oct 29, 2017

@oli-obk: 🔑 Insufficient privileges: Not in reviewers

@petrochenkov
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Oct 29, 2017

📌 Commit 0b7a8b5 has been approved by oli-obk

@petrochenkov
Copy link
Contributor Author

@oli-obk

Do we have an issue to discuss the fine grained lint groups and not clear cut "unused"-lints?

I've created #45615 for this.

@bors
Copy link
Contributor

bors commented Oct 29, 2017

⌛ Testing commit 0b7a8b5b146c002177ade20be0a29a2d34cafed3 with merge 518ec2985dd8476aabaa3966564073bc7a393464...

@bors
Copy link
Contributor

bors commented Oct 29, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Oct 29, 2017

@bors retry #43283 (arm-android test_process_output_fail_to_start 30min no output)

[01:10:10] test process::tests::test_process_output_fail_to_start has been running for over 60 seconds


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

@bors
Copy link
Contributor

bors commented Oct 29, 2017

⌛ Testing commit 0b7a8b5b146c002177ade20be0a29a2d34cafed3 with merge 5b370c494c9f0039676c7b72c21206d960155e95...

@bors
Copy link
Contributor

bors commented Oct 29, 2017

💔 Test failed - status-travis

Remove a couple of obsolete lints
@kennytm
Copy link
Member

kennytm commented Oct 29, 2017

The use of :vis have broken clippy. But should the feature gate error be emitted when compiling clippy, as a user of the macro instead of the provider? cc #41022 @durka.

[00:34:29]    Compiling clippy_lints v0.0.166 (file:///checkout/src/tools/clippy/clippy_lints)
[00:34:29] error: :vis fragment specifier is experimental and subject to change (see issue #41022)
[00:34:29]  --> <declare_lint macros>:1:3
[00:34:29]   |
[00:34:29] 1 | ( $ vis : vis $ NAME : ident , $ Level : ident , $ desc : expr ) => (
[00:34:29]   |   ^^^^^^^^^^^
[00:34:29]   |
[00:34:29]   = help: add #![feature(macro_vis_matcher)] to the crate attributes to enable
[00:34:29] 
[00:34:29] error: :vis fragment specifier is experimental and subject to change (see issue #41022)
[00:34:29]  --> <declare_lint macros>:1:3
[00:34:29]   |
[00:34:29] 1 | ( $ vis : vis $ NAME : ident , $ Level : ident , $ desc : expr ) => (
[00:34:29]   |   ^^^^^^^^^^^
[00:34:29]   |
[00:34:29]   = help: add #![feature(macro_vis_matcher)] to the crate attributes to enable
[00:34:29] 
[00:34:30] error: aborting due to previous error
[00:34:30] 
[00:34:30] error: Could not compile `clippy_lints`.
[00:34:30] warning: build failed, waiting for other jobs to finish...
[00:34:30] error: aborting due to previous error
[00:34:30] 
[00:34:30] error: Could not compile `clippy_lints`.
[00:34:30] warning: build failed, waiting for other jobs to finish...
[00:34:41] error: build failed

@petrochenkov
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Oct 29, 2017

📌 Commit bf0cdb5 has been approved by oli-obk

@durka
Copy link
Contributor

durka commented Oct 29, 2017

Hmm. Ideally the writer of the macro should need the feature gate, not the user. But AFAIK pretty much all checking of macros is left until the last possible second. For example the current version of error_chain has an undefined capture and nobody noticed until yesterday (and only because they wrote a stricter compiler).

@durka
Copy link
Contributor

durka commented Oct 29, 2017

Let's just stabilize :vis and remove the issue 😛

@bors
Copy link
Contributor

bors commented Oct 29, 2017

⌛ Testing commit bf0cdb5 with merge dae6868...

bors added a commit that referenced this pull request Oct 29, 2017
Add several lints into `unused` lint group

Also a couple of obsolete (not reported) lints are removed.

r? @oli-obk
@bors
Copy link
Contributor

bors commented Oct 30, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing dae6868 to master...

@bors bors merged commit bf0cdb5 into rust-lang:master Oct 30, 2017
@Manishearth
Copy link
Member

Uh, please at least try to fix tools that break (even if you mark it as broken in the interim).

In this case for some reason #![feature(macro_vis_matcher)] does not fix the problem in clippy; it continues to give the same error despite being allowed already.

@Manishearth
Copy link
Member

Oh, nvm, I made a mistake. Sorry.

@petrochenkov petrochenkov deleted the grlint branch June 5, 2019 15:56
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