Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
Contributor

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 sanxiyn force-pushed the unused-type-parameter-2 branch from 14cd171 to f14439d Compare November 24, 2016 14:12
@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
Collaborator

bors commented Dec 8, 2016

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

@sanxiyn sanxiyn force-pushed the unused-type-parameter-2 branch from f14439d to e107d50 Compare December 8, 2016 08:18
@sanxiyn sanxiyn force-pushed the unused-type-parameter-2 branch from e107d50 to 9604b59 Compare December 15, 2016 09:34
@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
Collaborator

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!

# for free to join this conversation on GitHub. Already have an account? # 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