From 5c70f25f8c68716ae448fd62c47483d0e0128e53 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Sat, 25 Sep 2021 18:23:07 +0000 Subject: [PATCH 1/2] Detect when negative literal indices are used and suggest appropriate code --- compiler/rustc_typeck/src/check/expr.rs | 2 +- compiler/rustc_typeck/src/check/place_op.rs | 53 ++++++++++++++++++- .../suggestions/negative-literal-index.fixed | 22 ++++++++ .../ui/suggestions/negative-literal-index.rs | 22 ++++++++ .../suggestions/negative-literal-index.stderr | 35 ++++++++++++ 5 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/suggestions/negative-literal-index.fixed create mode 100644 src/test/ui/suggestions/negative-literal-index.rs create mode 100644 src/test/ui/suggestions/negative-literal-index.stderr diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 8a69e0a737d50..09aeb62fc02c7 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -2136,7 +2136,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { idx_t } else { let base_t = self.structurally_resolved_type(base.span, base_t); - match self.lookup_indexing(expr, base, base_t, idx_t) { + match self.lookup_indexing(expr, base, base_t, idx, idx_t) { Some((index_ty, element_ty)) => { // two-phase not needed because index_ty is never mutable self.demand_coerce(idx, idx_t, index_ty, None, AllowTwoPhase::No); diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index 055072d3a1d9d..e5a5066544a8f 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -1,5 +1,7 @@ use crate::check::method::MethodCallee; use crate::check::{has_expected_num_generic_args, FnCtxt, PlaceOp}; +use rustc_ast as ast; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; use rustc_infer::infer::InferOk; @@ -47,6 +49,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr: &hir::Expr<'_>, base_expr: &'tcx hir::Expr<'tcx>, base_ty: Ty<'tcx>, + idx_expr: &'tcx hir::Expr<'tcx>, idx_ty: Ty<'tcx>, ) -> Option<(/*index type*/ Ty<'tcx>, /*element type*/ Ty<'tcx>)> { // FIXME(#18741) -- this is almost but not quite the same as the @@ -56,7 +59,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let mut autoderef = self.autoderef(base_expr.span, base_ty); let mut result = None; while result.is_none() && autoderef.next().is_some() { - result = self.try_index_step(expr, base_expr, &autoderef, idx_ty); + result = self.try_index_step(expr, base_expr, &autoderef, idx_ty, idx_expr); } self.register_predicates(autoderef.into_obligations()); result @@ -73,6 +76,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { base_expr: &hir::Expr<'_>, autoderef: &Autoderef<'a, 'tcx>, index_ty: Ty<'tcx>, + idx_expr: &hir::Expr<'_>, ) -> Option<(/*index type*/ Ty<'tcx>, /*element type*/ Ty<'tcx>)> { let adjusted_ty = self.structurally_resolved_type(autoderef.span(), autoderef.final_ty(false)); @@ -82,6 +86,53 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr, base_expr, adjusted_ty, index_ty ); + let negative_index = || { + let ty = self.resolve_vars_if_possible(adjusted_ty); + let mut err = self.tcx.sess.struct_span_err( + idx_expr.span, + &format!("negative integers cannot be used to index on a `{}`", ty), + ); + err.span_label( + idx_expr.span, + &format!("cannot use a negative integer for indexing on `{}`", ty), + ); + if let (hir::ExprKind::Path(..), Ok(snippet)) = + (&base_expr.kind, self.tcx.sess.source_map().span_to_snippet(base_expr.span)) + { + // `foo[-1]` to `foo[foo.len() - 1]` + err.span_suggestion_verbose( + idx_expr.span.shrink_to_lo(), + &format!( + "if you wanted to access an element starting from the end of the `{}`, you \ + must compute it", + ty, + ), + format!("{}.len() ", snippet), + Applicability::MachineApplicable, + ); + } + err.emit(); + Some((self.tcx.ty_error(), self.tcx.ty_error())) + }; + if let hir::ExprKind::Unary( + hir::UnOp::Neg, + hir::Expr { + kind: hir::ExprKind::Lit(hir::Lit { node: ast::LitKind::Int(..), .. }), + .. + }, + ) = idx_expr.kind + { + match adjusted_ty.kind() { + ty::Adt(ty::AdtDef { did, .. }, _) + if self.tcx.is_diagnostic_item(sym::vec_type, *did) => + { + return negative_index(); + } + ty::Slice(_) | ty::Array(_, _) => return negative_index(), + _ => {} + } + } + for unsize in [false, true] { let mut self_ty = adjusted_ty; if unsize { diff --git a/src/test/ui/suggestions/negative-literal-index.fixed b/src/test/ui/suggestions/negative-literal-index.fixed new file mode 100644 index 0000000000000..e52714cf97fe6 --- /dev/null +++ b/src/test/ui/suggestions/negative-literal-index.fixed @@ -0,0 +1,22 @@ +// run-rustfix + +use std::ops::Index; +struct X; +impl Index for X { + type Output = (); + + fn index(&self, _: i32) -> &() { + &() + } +} + +fn main() { + let x = vec![1, 2, 3]; + x[x.len() -1]; //~ ERROR negative integers cannot be used to index on a + let x = [1, 2, 3]; + x[x.len() -1]; //~ ERROR negative integers cannot be used to index on a + let x = &[1, 2, 3]; + x[x.len() -1]; //~ ERROR negative integers cannot be used to index on a + let _ = x; + X[-1]; +} diff --git a/src/test/ui/suggestions/negative-literal-index.rs b/src/test/ui/suggestions/negative-literal-index.rs new file mode 100644 index 0000000000000..d88b66e679e56 --- /dev/null +++ b/src/test/ui/suggestions/negative-literal-index.rs @@ -0,0 +1,22 @@ +// run-rustfix + +use std::ops::Index; +struct X; +impl Index for X { + type Output = (); + + fn index(&self, _: i32) -> &() { + &() + } +} + +fn main() { + let x = vec![1, 2, 3]; + x[-1]; //~ ERROR negative integers cannot be used to index on a + let x = [1, 2, 3]; + x[-1]; //~ ERROR negative integers cannot be used to index on a + let x = &[1, 2, 3]; + x[-1]; //~ ERROR negative integers cannot be used to index on a + let _ = x; + X[-1]; +} diff --git a/src/test/ui/suggestions/negative-literal-index.stderr b/src/test/ui/suggestions/negative-literal-index.stderr new file mode 100644 index 0000000000000..f5ea980048b5b --- /dev/null +++ b/src/test/ui/suggestions/negative-literal-index.stderr @@ -0,0 +1,35 @@ +error: negative integers cannot be used to index on a `Vec<{integer}>` + --> $DIR/negative-literal-index.rs:15:7 + | +LL | x[-1]; + | ^^ cannot use a negative integer for indexing on `Vec<{integer}>` + | +help: if you wanted to access an element starting from the end of the `Vec<{integer}>`, you must compute it + | +LL | x[x.len() -1]; + | +++++++ + +error: negative integers cannot be used to index on a `[{integer}; 3]` + --> $DIR/negative-literal-index.rs:17:7 + | +LL | x[-1]; + | ^^ cannot use a negative integer for indexing on `[{integer}; 3]` + | +help: if you wanted to access an element starting from the end of the `[{integer}; 3]`, you must compute it + | +LL | x[x.len() -1]; + | +++++++ + +error: negative integers cannot be used to index on a `[{integer}; 3]` + --> $DIR/negative-literal-index.rs:19:7 + | +LL | x[-1]; + | ^^ cannot use a negative integer for indexing on `[{integer}; 3]` + | +help: if you wanted to access an element starting from the end of the `[{integer}; 3]`, you must compute it + | +LL | x[x.len() -1]; + | +++++++ + +error: aborting due to 3 previous errors + From e19d82f1bf190de6c67114811e12256cba6831e8 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Tue, 28 Sep 2021 16:13:39 +0000 Subject: [PATCH 2/2] review comments --- compiler/rustc_typeck/src/check/place_op.rs | 72 ++++++++++--------- .../suggestions/negative-literal-index.stderr | 6 +- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index e5a5066544a8f..64775d7aba9f8 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -49,7 +49,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr: &hir::Expr<'_>, base_expr: &'tcx hir::Expr<'tcx>, base_ty: Ty<'tcx>, - idx_expr: &'tcx hir::Expr<'tcx>, + index_expr: &'tcx hir::Expr<'tcx>, idx_ty: Ty<'tcx>, ) -> Option<(/*index type*/ Ty<'tcx>, /*element type*/ Ty<'tcx>)> { // FIXME(#18741) -- this is almost but not quite the same as the @@ -59,12 +59,42 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let mut autoderef = self.autoderef(base_expr.span, base_ty); let mut result = None; while result.is_none() && autoderef.next().is_some() { - result = self.try_index_step(expr, base_expr, &autoderef, idx_ty, idx_expr); + result = self.try_index_step(expr, base_expr, &autoderef, idx_ty, index_expr); } self.register_predicates(autoderef.into_obligations()); result } + fn negative_index( + &self, + ty: Ty<'tcx>, + span: Span, + base_expr: &hir::Expr<'_>, + ) -> Option<(Ty<'tcx>, Ty<'tcx>)> { + let ty = self.resolve_vars_if_possible(ty); + let mut err = self.tcx.sess.struct_span_err( + span, + &format!("negative integers cannot be used to index on a `{}`", ty), + ); + err.span_label(span, &format!("cannot use a negative integer for indexing on `{}`", ty)); + if let (hir::ExprKind::Path(..), Ok(snippet)) = + (&base_expr.kind, self.tcx.sess.source_map().span_to_snippet(base_expr.span)) + { + // `foo[-1]` to `foo[foo.len() - 1]` + err.span_suggestion_verbose( + span.shrink_to_lo(), + &format!( + "to access an element starting from the end of the `{}`, compute the index", + ty, + ), + format!("{}.len() ", snippet), + Applicability::MachineApplicable, + ); + } + err.emit(); + Some((self.tcx.ty_error(), self.tcx.ty_error())) + } + /// To type-check `base_expr[index_expr]`, we progressively autoderef /// (and otherwise adjust) `base_expr`, looking for a type which either /// supports builtin indexing or overloaded indexing. @@ -76,7 +106,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { base_expr: &hir::Expr<'_>, autoderef: &Autoderef<'a, 'tcx>, index_ty: Ty<'tcx>, - idx_expr: &hir::Expr<'_>, + index_expr: &hir::Expr<'_>, ) -> Option<(/*index type*/ Ty<'tcx>, /*element type*/ Ty<'tcx>)> { let adjusted_ty = self.structurally_resolved_type(autoderef.span(), autoderef.final_ty(false)); @@ -86,49 +116,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr, base_expr, adjusted_ty, index_ty ); - let negative_index = || { - let ty = self.resolve_vars_if_possible(adjusted_ty); - let mut err = self.tcx.sess.struct_span_err( - idx_expr.span, - &format!("negative integers cannot be used to index on a `{}`", ty), - ); - err.span_label( - idx_expr.span, - &format!("cannot use a negative integer for indexing on `{}`", ty), - ); - if let (hir::ExprKind::Path(..), Ok(snippet)) = - (&base_expr.kind, self.tcx.sess.source_map().span_to_snippet(base_expr.span)) - { - // `foo[-1]` to `foo[foo.len() - 1]` - err.span_suggestion_verbose( - idx_expr.span.shrink_to_lo(), - &format!( - "if you wanted to access an element starting from the end of the `{}`, you \ - must compute it", - ty, - ), - format!("{}.len() ", snippet), - Applicability::MachineApplicable, - ); - } - err.emit(); - Some((self.tcx.ty_error(), self.tcx.ty_error())) - }; if let hir::ExprKind::Unary( hir::UnOp::Neg, hir::Expr { kind: hir::ExprKind::Lit(hir::Lit { node: ast::LitKind::Int(..), .. }), .. }, - ) = idx_expr.kind + ) = index_expr.kind { match adjusted_ty.kind() { ty::Adt(ty::AdtDef { did, .. }, _) if self.tcx.is_diagnostic_item(sym::vec_type, *did) => { - return negative_index(); + return self.negative_index(adjusted_ty, index_expr.span, base_expr); + } + ty::Slice(_) | ty::Array(_, _) => { + return self.negative_index(adjusted_ty, index_expr.span, base_expr); } - ty::Slice(_) | ty::Array(_, _) => return negative_index(), _ => {} } } diff --git a/src/test/ui/suggestions/negative-literal-index.stderr b/src/test/ui/suggestions/negative-literal-index.stderr index f5ea980048b5b..2b51bf7b7ce87 100644 --- a/src/test/ui/suggestions/negative-literal-index.stderr +++ b/src/test/ui/suggestions/negative-literal-index.stderr @@ -4,7 +4,7 @@ error: negative integers cannot be used to index on a `Vec<{integer}>` LL | x[-1]; | ^^ cannot use a negative integer for indexing on `Vec<{integer}>` | -help: if you wanted to access an element starting from the end of the `Vec<{integer}>`, you must compute it +help: to access an element starting from the end of the `Vec<{integer}>`, compute the index | LL | x[x.len() -1]; | +++++++ @@ -15,7 +15,7 @@ error: negative integers cannot be used to index on a `[{integer}; 3]` LL | x[-1]; | ^^ cannot use a negative integer for indexing on `[{integer}; 3]` | -help: if you wanted to access an element starting from the end of the `[{integer}; 3]`, you must compute it +help: to access an element starting from the end of the `[{integer}; 3]`, compute the index | LL | x[x.len() -1]; | +++++++ @@ -26,7 +26,7 @@ error: negative integers cannot be used to index on a `[{integer}; 3]` LL | x[-1]; | ^^ cannot use a negative integer for indexing on `[{integer}; 3]` | -help: if you wanted to access an element starting from the end of the `[{integer}; 3]`, you must compute it +help: to access an element starting from the end of the `[{integer}; 3]`, compute the index | LL | x[x.len() -1]; | +++++++