-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix unused_parens issue for higher ranked function pointers #106148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you still want to look into the alternative approach suggested by @cjgillot? I am a bit uncomfortable with using spans for stuff like that as I can never get myself to be certain that we're correctly dealing with macros.
Also, can you add a test for both: (fn()): Trait
if we don't have one yet (this should lint), and ((for<'a> fn(&'a u32))): Trait
(this should lint for the inner brace).
Yes, I met an issue last time I tried it, will reconsider it later. |
ab3b25b
to
7689401
Compare
compiler/rustc_lint/src/early.rs
Outdated
if let WherePredicate::BoundPredicate(WhereBoundPredicate { bounded_ty, .. }) = p && | ||
let ast::TyKind::BareFn(b) = &bounded_ty.kind && | ||
b.generic_params.len() > 0 { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this check for ast::TyKInd::Paren
containing a BareFn
I think the condition should also include the check bound_generic_params.is_empty()
.
THis should lint as for<'b> for<'a> fn(Inv<'a>): Trait<'b>
is also correct:
#![deny(warnings)]
struct Inv<'a>(&'a mut &'a ());
trait Trait<'a> {}
impl<'b> Trait<'b> for for<'a> fn(Inv<'a>) {}
fn with_bound()
where
for<'b> (for<'a> fn(Inv<'a>)): Trait<'b>, //~ ERROR unnecessary parentheses around type
{}
fn main() {
with_bound();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, but instead of changing the visiting behavior for all lints, you should instead add a method like fn enter_lint_where_bound
or something to EarlyLintPass
where the UnusedParens
lint has special behavior. Similar to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html#method.enter_lint_attrs
compiler/rustc_lint/src/unused.rs
Outdated
@@ -1033,7 +1033,6 @@ impl EarlyLintPass for UnusedParens { | |||
if let ast::TyKind::Paren(r) = &ty.kind { | |||
match &r.kind { | |||
ast::TyKind::TraitObject(..) => {} | |||
ast::TyKind::BareFn(b) if b.generic_params.len() > 0 => {} | |||
ast::TyKind::ImplTrait(_, bounds) if bounds.len() > 1 => {} | |||
ast::TyKind::Array(_, len) => { | |||
self.check_unused_delims_expr( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized, that's a bug 😁 it causes us to not lint on
fn main() {
let _x: ([u32; 3]);
// ^^^^^^^^^^ should lint
let _y: [u8; (3)];
// ^^^ should lint
let _z: ([u8; (3)]);
// ^^^ does lint
}
may make sense as part of this PR. HAve to move the ast::TyKind::Array
branch to the same level as the if let ast::TyKind::Paren(r) = &ty.kind {
, not inside of TyKind::Paren
.
If not I am going to open a separate issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we find some lint error in std
😂:
error: unnecessary parentheses around const expression
--> library/std/src/io/error/repr_bitpacked.rs:377:27
|
377 | static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE);
| ^ ^
|
= note: `-D unused-parens` implied by `-D warnings`
help: remove these parentheses
|
377 - static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE);
377 + static_assert!(@usize_eq: TAG_MASK & TAG_SIMPLE_MESSAGE, TAG_SIMPLE_MESSAGE);
|
error: unnecessary parentheses around const expression
--> library/std/src/io/error/repr_bitpacked.rs:378:27
|
378 | static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM);
| ^ ^
|
help: remove these parentheses
|
378 - static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM);
378 + static_assert!(@usize_eq: TAG_MASK & TAG_CUSTOM, TAG_CUSTOM);
|
This comment has been minimized.
This comment has been minimized.
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 👍
r=me after nits
compiler/rustc_lint/src/unused.rs
Outdated
@@ -1030,21 +1040,22 @@ impl EarlyLintPass for UnusedParens { | |||
} | |||
|
|||
fn check_ty(&mut self, cx: &EarlyContext<'_>, ty: &ast::Ty) { | |||
if let ast::TyKind::Array(_, len) = &ty.kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: can you use a match
instead of 2 unrelated if let
compiler/rustc_lint/src/unused.rs
Outdated
None, | ||
None, | ||
); | ||
} | ||
if let ast::TyKind::Paren(r) = &ty.kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you set self.with_self_ty_parens
to false
at the end of this branch and instead assert that it is false in exit_where_predicate
?
Because right now I think you don't lint for the inner paren in (dyn Trait<(for<'a> fn(&'a ())>): Trait,
even though we should.
27934df
to
9d74bb8
Compare
@rustbot |
@bors r+ rollup |
Rollup of 5 pull requests Successful merges: - rust-lang#101698 (Constify `TypeId` ordering impls) - rust-lang#106148 (Fix unused_parens issue for higher ranked function pointers) - rust-lang#106922 (Avoid unsafe code in `to_ascii_[lower/upper]case()`) - rust-lang#106951 (Remove ineffective run of SimplifyConstCondition) - rust-lang#106962 (Fix use suggestion span) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
fixes #105061
r? @lcnr