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

Warn unused type parameters #37946

Closed
wants to merge 2 commits into from

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Nov 23, 2016

Rebase of #26684. Fix #25871.

Following @huonw's suggestion, warning is disabled for intrinsics and lang items. An example of unused but not incorrect type parameters is core::intrinsics::size_of for intrinsics and core::marker::PhantomData for lang items.

Following @nikomatsakis's suggestion, warning can be silenced by prefixing the underscore as in _T. As pointed out, fn _is_sync_send<_T: Sync+Send>() {} is analogous to fn drop<T>(_x: T) {} for unused variables.

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@frewsxcv
Copy link
Member

error: unused type parameter
   --> /checkout/src/libserialize/serialize.rs:677:18
    |
677 |     fn not_found<S, T: ?Sized>(trait_name: &'static str,
    |                  ^
    |
note: lint level defined here
   --> /checkout/src/libserialize/lib.rs:28:31
    |
28  | #![cfg_attr(not(stage0), deny(warnings))]
    |                               ^^^^^^^^

error: unused type parameter
   --> /checkout/src/libserialize/serialize.rs:677:21
    |
677 |     fn not_found<S, T: ?Sized>(trait_name: &'static str,
    |                     ^

error: aborting due to 2 previous errors

@sanxiyn
Copy link
Member Author

sanxiyn commented Nov 24, 2016

I guess that's what I get for testing with make check-stage1-cfail. Fixed all tests.

@nikomatsakis
Copy link
Contributor

I'm not sure what the policy is to decide on a new lint, but I do think that this is a useful one -- or at minimum, the error messages you get when you have unused type parameters are terrible. I'm not sure how much this helps though since (as a lint) it gets reported later. I might rather just improve the "unconstrained type parameter" message. @jonathandturner was doing some work on that, I think?

@sophiajt
Copy link
Contributor

sophiajt commented Dec 1, 2016

Nothing from me, so please feel free to take it and run with it.

@bors
Copy link
Contributor

bors commented Dec 8, 2016

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

@sanxiyn
Copy link
Member Author

sanxiyn commented Dec 15, 2016

I would like to get some (any) opinion on this, unlike my last attempt in 2015 which received no comments for a month and closed for inactivity.

@petrochenkov
Copy link
Contributor

@sanxiyn

I would like to get some (any) opinion on this, unlike my last attempt in 2015 which received no comments for a month and closed for inactivity.

Everyone seems to be on vacation at the moment.
It would be interesting to test this with crater (probably in forbid-by-default form) to see the statistics of breakage. The potential for breakage seems to be relatively high, maybe it will end up as allow-by-default in the worst case.

@nikomatsakis
Copy link
Contributor

The code seems ok. We don't really have a clear policy for how to decide whether to add a new lint, do we? I'm not even 100% sure what team is responsible. I don't generally see the compiler team as making "user-facing" decisions of this kind. It could be a @rust-lang/lang decision or perhaps @rust-lang/tools -- have we settled this question in the past?

cc @rust-lang/core

@aturon
Copy link
Member

aturon commented Dec 20, 2016

@nikomatsakis I believe that new lints are required to go through an RFC and be approved by the lang team.

@bors
Copy link
Contributor

bors commented Dec 23, 2016

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

@steveklabnik
Copy link
Member

I believe that new lints are required to go through an RFC and be approved by the lang team.

Yes, this is true, as I mentioned on 25871. @sanxiyn as this is a new lint, please open an RFC. Thanks!

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.

Should have lint to disallow unused type parameters
10 participants