Skip to content

Commit

Permalink
Rollup merge of rust-lang#111757 - lowr:fix/lint-attr-on-match-arm, r…
Browse files Browse the repository at this point in the history
…=eholk

Consider lint check attributes on match arms

Currently, lint check attributes on match arms have no effect for some lints. This PR makes some lint passes to take those attributes into account.

- `LateContextAndPass` for late lint doesn't update `last_node_with_lint_attrs` when it visits match arms. This leads to lint check attributes on match arms taking no effects on late lints that operate on the arms' pattern:

  ```rust
  match value {
      #[deny(non_snake_case)]
      PAT => {} // `non_snake_case` only warned due to default lint level
  }
  ```

  To be honest, I'm not sure whether this is intentional or just an oversight. I've dug the implementation history and searched up issues/PRs but couldn't find any discussion on this.

- `MatchVisitor` doesn't update its lint level when it visits match arms. This leads to check lint attributes on match arms taking no effect on some lints handled by this visitor, namely: `bindings_with_variant_name` and `irrefutable_let_patterns`.

  This seems to be a fallout from rust-lang#108504. Before 05082f5, when the visitor operated on HIR rather than THIR, check lint attributes for the said lints were effective. [This playground][play] compiles successfully on current stable (1.69) but fails on current beta and nightly.

  I wasn't sure where best to place the test for this. Let me know if there's a better place.

[play]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=38432b79e535cb175f8f7d6d236d29c3
[play-match]: https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=629aa71b7c84b269beadeba664e2221d
  • Loading branch information
matthiaskrgr authored May 25, 2023
2 parents 8eaef97 + 3a03587 commit a6cc412
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 64 deletions.
6 changes: 4 additions & 2 deletions compiler/rustc_lint/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,10 @@ impl<'tcx, T: LateLintPass<'tcx>> hir_visit::Visitor<'tcx> for LateContextAndPas
}

fn visit_arm(&mut self, a: &'tcx hir::Arm<'tcx>) {
lint_callback!(self, check_arm, a);
hir_visit::walk_arm(self, a);
self.with_lint_attrs(a.hir_id, |cx| {
lint_callback!(cx, check_arm, a);
hir_visit::walk_arm(cx, a);
})
}

fn visit_generic_param(&mut self, p: &'tcx hir::GenericParam<'tcx>) {
Expand Down
58 changes: 35 additions & 23 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,35 +90,34 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for MatchVisitor<'a, '_, 'tcx> {

#[instrument(level = "trace", skip(self))]
fn visit_arm(&mut self, arm: &Arm<'tcx>) {
match arm.guard {
Some(Guard::If(expr)) => {
self.with_let_source(LetSource::IfLetGuard, |this| {
this.visit_expr(&this.thir[expr])
});
}
Some(Guard::IfLet(ref pat, expr)) => {
self.with_let_source(LetSource::IfLetGuard, |this| {
this.check_let(pat, expr, LetSource::IfLetGuard, pat.span);
this.visit_pat(pat);
this.visit_expr(&this.thir[expr]);
});
self.with_lint_level(arm.lint_level, |this| {
match arm.guard {
Some(Guard::If(expr)) => {
this.with_let_source(LetSource::IfLetGuard, |this| {
this.visit_expr(&this.thir[expr])
});
}
Some(Guard::IfLet(ref pat, expr)) => {
this.with_let_source(LetSource::IfLetGuard, |this| {
this.check_let(pat, expr, LetSource::IfLetGuard, pat.span);
this.visit_pat(pat);
this.visit_expr(&this.thir[expr]);
});
}
None => {}
}
None => {}
}
self.visit_pat(&arm.pattern);
self.visit_expr(&self.thir[arm.body]);
this.visit_pat(&arm.pattern);
this.visit_expr(&self.thir[arm.body]);
});
}

#[instrument(level = "trace", skip(self))]
fn visit_expr(&mut self, ex: &Expr<'tcx>) {
match ex.kind {
ExprKind::Scope { value, lint_level, .. } => {
let old_lint_level = self.lint_level;
if let LintLevel::Explicit(hir_id) = lint_level {
self.lint_level = hir_id;
}
self.visit_expr(&self.thir[value]);
self.lint_level = old_lint_level;
self.with_lint_level(lint_level, |this| {
this.visit_expr(&this.thir[value]);
});
return;
}
ExprKind::If { cond, then, else_opt, if_then_scope: _ } => {
Expand Down Expand Up @@ -190,6 +189,17 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
self.let_source = old_let_source;
}

fn with_lint_level(&mut self, new_lint_level: LintLevel, f: impl FnOnce(&mut Self)) {
if let LintLevel::Explicit(hir_id) = new_lint_level {
let old_lint_level = self.lint_level;
self.lint_level = hir_id;
f(self);
self.lint_level = old_lint_level;
} else {
f(self);
}
}

fn check_patterns(&self, pat: &Pat<'tcx>, rf: RefutableFlag) {
pat.walk_always(|pat| check_borrow_conflicts_in_at_patterns(self, pat));
check_for_bindings_named_same_as_variants(self, pat, rf);
Expand Down Expand Up @@ -236,7 +246,9 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
for &arm in arms {
// Check the arm for some things unrelated to exhaustiveness.
let arm = &self.thir.arms[arm];
self.check_patterns(&arm.pattern, Refutable);
self.with_lint_level(arm.lint_level, |this| {
this.check_patterns(&arm.pattern, Refutable);
});
}

let tarms: Vec<_> = arms
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/lint/lint-attr-everywhere-early.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,14 @@ fn expressions() {
}
}

match f {
#[deny(ellipsis_inclusive_range_patterns)]
Match{f1: 0...100} => {}
//~^ ERROR range patterns are deprecated
//~| WARNING this is accepted in the current edition
_ => {}
}

// Statement Block
{
#![deny(unsafe_code)]
Expand Down
48 changes: 31 additions & 17 deletions tests/ui/lint/lint-attr-everywhere-early.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -384,103 +384,117 @@ note: the lint level is defined here
LL | #[deny(while_true)]
| ^^^^^^^^^^

error: `...` range patterns are deprecated
--> $DIR/lint-attr-everywhere-early.rs:139:20
|
LL | Match{f1: 0...100} => {}
| ^^^ help: use `..=` for an inclusive range
|
= warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2021!
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/warnings-promoted-to-error.html>
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:138:16
|
LL | #[deny(ellipsis_inclusive_range_patterns)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:140:9
--> $DIR/lint-attr-everywhere-early.rs:148:9
|
LL | unsafe {}
| ^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:139:17
--> $DIR/lint-attr-everywhere-early.rs:147:17
|
LL | #![deny(unsafe_code)]
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:144:9
--> $DIR/lint-attr-everywhere-early.rs:152:9
|
LL | unsafe {}
| ^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:143:16
--> $DIR/lint-attr-everywhere-early.rs:151:16
|
LL | #[deny(unsafe_code)]
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:149:5
--> $DIR/lint-attr-everywhere-early.rs:157:5
|
LL | unsafe {};
| ^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:148:12
--> $DIR/lint-attr-everywhere-early.rs:156:12
|
LL | #[deny(unsafe_code)]
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:151:27
--> $DIR/lint-attr-everywhere-early.rs:159:27
|
LL | [#[deny(unsafe_code)] unsafe {123}];
| ^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:151:13
--> $DIR/lint-attr-everywhere-early.rs:159:13
|
LL | [#[deny(unsafe_code)] unsafe {123}];
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:152:27
--> $DIR/lint-attr-everywhere-early.rs:160:27
|
LL | (#[deny(unsafe_code)] unsafe {123},);
| ^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:152:13
--> $DIR/lint-attr-everywhere-early.rs:160:13
|
LL | (#[deny(unsafe_code)] unsafe {123},);
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:154:31
--> $DIR/lint-attr-everywhere-early.rs:162:31
|
LL | call(#[deny(unsafe_code)] unsafe {123});
| ^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:154:17
--> $DIR/lint-attr-everywhere-early.rs:162:17
|
LL | call(#[deny(unsafe_code)] unsafe {123});
| ^^^^^^^^^^^

error: usage of an `unsafe` block
--> $DIR/lint-attr-everywhere-early.rs:156:38
--> $DIR/lint-attr-everywhere-early.rs:164:38
|
LL | TupleStruct(#[deny(unsafe_code)] unsafe {123});
| ^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:156:24
--> $DIR/lint-attr-everywhere-early.rs:164:24
|
LL | TupleStruct(#[deny(unsafe_code)] unsafe {123});
| ^^^^^^^^^^^

error: `...` range patterns are deprecated
--> $DIR/lint-attr-everywhere-early.rs:167:18
--> $DIR/lint-attr-everywhere-early.rs:175:18
|
LL | f1: 0...100,
| ^^^ help: use `..=` for an inclusive range
|
= warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2021!
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/warnings-promoted-to-error.html>
note: the lint level is defined here
--> $DIR/lint-attr-everywhere-early.rs:166:20
--> $DIR/lint-attr-everywhere-early.rs:174:20
|
LL | #[deny(ellipsis_inclusive_range_patterns)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 36 previous errors
error: aborting due to 37 previous errors

5 changes: 5 additions & 0 deletions tests/ui/lint/lint-attr-everywhere-late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ fn expressions() {
}
}

match 123 {
#[deny(non_snake_case)]
ARM_VAR => {} //~ ERROR variable `ARM_VAR` should have a snake case name
}

// Statement Block
{
#![deny(enum_intrinsics_non_enums)]
Expand Down
Loading

0 comments on commit a6cc412

Please sign in to comment.