Auto merge of rust-lang#116623 - Nadrieril:validate-range-endpoints, r=oli-obk

Fix overflow checking in range patterns

When a range pattern contains an overflowing literal, if we're not careful we might not notice the overflow and use the wrapped value. This makes for confusing error messages because linting against overflowing literals is only done in a later pass. So when a range is invalid we check for overflows to provide a better error.

This check didn't use to handle negative types; this PR fixes that. First commit adds tests, second cleans up without changing behavior, third does the fix.

EDIT: while I was at it, I fixed a small annoyance about the span of the overflow lint on negated literals.

Fixes rust-lang#94239

Author: bors

compiler/rustc_lint/src/types.rs

@@ -140,13 +140,15 @@ declare_lint! {
 pub struct TypeLimits {
     /// Id of the last visited negated expression
     negated_expr_id: Option<hir::HirId>,
+    /// Span of the last visited negated expression
+    negated_expr_span: Option<Span>,
 }
 
 impl_lint_pass!(TypeLimits => [UNUSED_COMPARISONS, OVERFLOWING_LITERALS, INVALID_NAN_COMPARISONS]);
 
 impl TypeLimits {
     pub fn new() -> TypeLimits {
-        TypeLimits { negated_expr_id: None }
+        TypeLimits { negated_expr_id: None, negated_expr_span: None }
     }
 }
 
@@ -426,17 +428,15 @@ fn lint_int_literal<'tcx>(
             return;
         }
 
-        let lit = cx
-            .sess()
-            .source_map()
-            .span_to_snippet(lit.span)
-            .expect("must get snippet from literal");
+        let span = if negative { type_limits.negated_expr_span.unwrap() } else { e.span };
+        let lit =
+            cx.sess().source_map().span_to_snippet(span).expect("must get snippet from literal");
         let help = get_type_suggestion(cx.typeck_results().node_type(e.hir_id), v, negative)
             .map(|suggestion_ty| OverflowingIntHelp { suggestion_ty });
 
         cx.emit_spanned_lint(
             OVERFLOWING_LITERALS,
-            e.span,
+            span,
             OverflowingInt { ty: t.name_str(), lit, min, max, help },
         );
     }
@@ -622,9 +622,10 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
         match e.kind {
             hir::ExprKind::Unary(hir::UnOp::Neg, ref expr) => {
-                // propagate negation, if the negation itself isn't negated
+                // Propagate negation, if the negation itself isn't negated
                 if self.negated_expr_id != Some(e.hir_id) {
                     self.negated_expr_id = Some(expr.hir_id);
+                    self.negated_expr_span = Some(e.span);
                 }
             }
             hir::ExprKind::Binary(binop, ref l, ref r) => {

compiler/rustc_mir_build/messages.ftl

@@ -173,7 +173,7 @@ mir_build_leading_irrefutable_let_patterns = leading irrefutable {$count ->
 
 mir_build_literal_in_range_out_of_bounds =
     literal out of range for `{$ty}`
-    .label = this value doesn't fit in `{$ty}` whose maximum value is `{$max}`
+    .label = this value does not fit into the type `{$ty}` whose range is `{$min}..={$max}`
 
 mir_build_lower_range_bound_must_be_less_than_or_equal_to_upper =
     lower range bound must be less than or equal to upper

compiler/rustc_mir_build/src/errors.rs

@@ -551,6 +551,7 @@ pub struct LiteralOutOfRange<'tcx> {
     #[label]
     pub span: Span,
     pub ty: Ty<'tcx>,
+    pub min: i128,
     pub max: u128,
 }
 

compiler/rustc_mir_build/src/thir/pattern/mod.rs

@@ -20,15 +20,15 @@ use rustc_index::Idx;
 use rustc_middle::mir::interpret::{
     ErrorHandled, GlobalId, LitToConstError, LitToConstInput, Scalar,
 };
-use rustc_middle::mir::{self, Const, UserTypeProjection};
-use rustc_middle::mir::{BorrowKind, Mutability};
+use rustc_middle::mir::{self, BorrowKind, Const, Mutability, UserTypeProjection};
 use rustc_middle::thir::{Ascription, BindingMode, FieldPat, LocalVarId, Pat, PatKind, PatRange};
-use rustc_middle::ty::CanonicalUserTypeAnnotation;
-use rustc_middle::ty::TypeVisitableExt;
-use rustc_middle::ty::{self, AdtDef, Region, Ty, TyCtxt, UserType};
-use rustc_middle::ty::{GenericArg, GenericArgsRef};
-use rustc_span::{Span, Symbol};
-use rustc_target::abi::FieldIdx;
+use rustc_middle::ty::layout::IntegerExt;
+use rustc_middle::ty::{
+    self, AdtDef, CanonicalUserTypeAnnotation, GenericArg, GenericArgsRef, Region, Ty, TyCtxt,
+    TypeVisitableExt, UserType,
+};
+use rustc_span::{ErrorGuaranteed, Span, Symbol};
+use rustc_target::abi::{FieldIdx, Integer};
 
 use std::cmp::Ordering;
 
@@ -85,127 +85,159 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
         )
     }
 
-    fn lower_range_expr(
+    fn lower_pattern_range_endpoint(
         &mut self,
-        expr: &'tcx hir::Expr<'tcx>,
-    ) -> (PatKind<'tcx>, Option<Ascription<'tcx>>) {
-        match self.lower_lit(expr) {
-            PatKind::AscribeUserType { ascription, subpattern: box Pat { kind, .. } } => {
-                (kind, Some(ascription))
+        expr: Option<&'tcx hir::Expr<'tcx>>,
+    ) -> Result<(Option<mir::Const<'tcx>>, Option<Ascription<'tcx>>), ErrorGuaranteed> {
+        match expr {
+            None => Ok((None, None)),
+            Some(expr) => {
+                let (kind, ascr) = match self.lower_lit(expr) {
+                    PatKind::AscribeUserType { ascription, subpattern: box Pat { kind, .. } } => {
+                        (kind, Some(ascription))
+                    }
+                    kind => (kind, None),
+                };
+                let value = if let PatKind::Constant { value } = kind {
+                    value
+                } else {
+                    let msg = format!(
+                        "found bad range pattern endpoint `{expr:?}` outside of error recovery"
+                    );
+                    return Err(self.tcx.sess.delay_span_bug(expr.span, msg));
+                };
+                Ok((Some(value), ascr))
             }
-            kind => (kind, None),
         }
     }
 
+    /// Overflowing literals are linted against in a late pass. This is mostly fine, except when we
+    /// encounter a range pattern like `-130i8..2`: if we believe `eval_bits`, this looks like a
+    /// range where the endpoints are in the wrong order. To avoid a confusing error message, we
+    /// check for overflow then.
+    /// This is only called when the range is already known to be malformed.
+    fn error_on_literal_overflow(
+        &self,
+        expr: Option<&'tcx hir::Expr<'tcx>>,
+        ty: Ty<'tcx>,
+    ) -> Result<(), ErrorGuaranteed> {
+        use hir::{ExprKind, UnOp};
+        use rustc_ast::ast::LitKind;
+
+        let Some(mut expr) = expr else {
+            return Ok(());
+        };
+        let span = expr.span;
+
+        // We need to inspect the original expression, because if we only inspect the output of
+        // `eval_bits`, an overflowed value has already been wrapped around.
+        // We mostly copy the logic from the `rustc_lint::OVERFLOWING_LITERALS` lint.
+        let mut negated = false;
+        if let ExprKind::Unary(UnOp::Neg, sub_expr) = expr.kind {
+            negated = true;
+            expr = sub_expr;
+        }
+        let ExprKind::Lit(lit) = expr.kind else {
+            return Ok(());
+        };
+        let LitKind::Int(lit_val, _) = lit.node else {
+            return Ok(());
+        };
+        let (min, max): (i128, u128) = match ty.kind() {
+            ty::Int(ity) => {
+                let size = Integer::from_int_ty(&self.tcx, *ity).size();
+                (size.signed_int_min(), size.signed_int_max() as u128)
+            }
+            ty::Uint(uty) => {
+                let size = Integer::from_uint_ty(&self.tcx, *uty).size();
+                (0, size.unsigned_int_max())
+            }
+            _ => {
+                return Ok(());
+            }
+        };
+        // Detect literal value out of range `[min, max]` inclusive, avoiding use of `-min` to
+        // prevent overflow/panic.
+        if (negated && lit_val > max + 1) || (!negated && lit_val > max) {
+            return Err(self.tcx.sess.emit_err(LiteralOutOfRange { span, ty, min, max }));
+        }
+        Ok(())
+    }
+
     fn lower_pattern_range(
         &mut self,
-        ty: Ty<'tcx>,
-        lo: mir::Const<'tcx>,
-        hi: mir::Const<'tcx>,
+        lo_expr: Option<&'tcx hir::Expr<'tcx>>,
+        hi_expr: Option<&'tcx hir::Expr<'tcx>>,
         end: RangeEnd,
+        ty: Ty<'tcx>,
         span: Span,
-        lo_expr: Option<&hir::Expr<'tcx>>,
-        hi_expr: Option<&hir::Expr<'tcx>>,
-    ) -> PatKind<'tcx> {
+    ) -> Result<PatKind<'tcx>, ErrorGuaranteed> {
+        if lo_expr.is_none() && hi_expr.is_none() {
+            let msg = format!("found twice-open range pattern (`..`) outside of error recovery");
+            return Err(self.tcx.sess.delay_span_bug(span, msg));
+        }
+
+        let (lo, lo_ascr) = self.lower_pattern_range_endpoint(lo_expr)?;
+        let (hi, hi_ascr) = self.lower_pattern_range_endpoint(hi_expr)?;
+
+        let lo = lo.unwrap_or_else(|| {
+            // Unwrap is ok because the type is known to be numeric.
+            let lo = ty.numeric_min_val(self.tcx).unwrap();
+            mir::Const::from_ty_const(lo, self.tcx)
+        });
+        let hi = hi.unwrap_or_else(|| {
+            // Unwrap is ok because the type is known to be numeric.
+            let hi = ty.numeric_max_val(self.tcx).unwrap();
+            mir::Const::from_ty_const(hi, self.tcx)
+        });
         assert_eq!(lo.ty(), ty);
         assert_eq!(hi.ty(), ty);
+
         let cmp = compare_const_vals(self.tcx, lo, hi, self.param_env);
-        let max = || {
-            self.tcx
-                .layout_of(self.param_env.with_reveal_all_normalized(self.tcx).and(ty))
-                .ok()
-                .unwrap()
-                .size
-                .unsigned_int_max()
-        };
-        match (end, cmp) {
+        let mut kind = match (end, cmp) {
             // `x..y` where `x < y`.
             // Non-empty because the range includes at least `x`.
             (RangeEnd::Excluded, Some(Ordering::Less)) => {
                 PatKind::Range(Box::new(PatRange { lo, hi, end }))
             }
-            // `x..y` where `x >= y`. The range is empty => error.
-            (RangeEnd::Excluded, _) => {
-                let mut lower_overflow = false;
-                let mut higher_overflow = false;
-                if let Some(hir::Expr { kind: hir::ExprKind::Lit(lit), .. }) = lo_expr
-                    && let rustc_ast::ast::LitKind::Int(val, _) = lit.node
-                {
-                    if lo.eval_bits(self.tcx, self.param_env) != val {
-                        lower_overflow = true;
-                        self.tcx.sess.emit_err(LiteralOutOfRange { span: lit.span, ty, max: max() });
-                    }
-                }
-                if let Some(hir::Expr { kind: hir::ExprKind::Lit(lit), .. }) = hi_expr
-                    && let rustc_ast::ast::LitKind::Int(val, _) = lit.node
-                {
-                    if hi.eval_bits(self.tcx, self.param_env) != val {
-                        higher_overflow = true;
-                        self.tcx.sess.emit_err(LiteralOutOfRange { span: lit.span, ty, max: max() });
-                    }
-                }
-                if !lower_overflow && !higher_overflow {
-                    self.tcx.sess.emit_err(LowerRangeBoundMustBeLessThanUpper { span });
-                }
-                PatKind::Wild
-            }
             // `x..=y` where `x == y`.
             (RangeEnd::Included, Some(Ordering::Equal)) => PatKind::Constant { value: lo },
             // `x..=y` where `x < y`.
             (RangeEnd::Included, Some(Ordering::Less)) => {
                 PatKind::Range(Box::new(PatRange { lo, hi, end }))
             }
-            // `x..=y` where `x > y` hence the range is empty => error.
-            (RangeEnd::Included, _) => {
-                let mut lower_overflow = false;
-                let mut higher_overflow = false;
-                if let Some(hir::Expr { kind: hir::ExprKind::Lit(lit), .. }) = lo_expr
-                    && let rustc_ast::ast::LitKind::Int(val, _) = lit.node
-                {
-                    if lo.eval_bits(self.tcx, self.param_env) != val {
-                        lower_overflow = true;
-                        self.tcx.sess.emit_err(LiteralOutOfRange { span: lit.span, ty, max: max() });
+            // `x..y` where `x >= y`, or `x..=y` where `x > y`. The range is empty => error.
+            _ => {
+                // Emit a more appropriate message if there was overflow.
+                self.error_on_literal_overflow(lo_expr, ty)?;
+                self.error_on_literal_overflow(hi_expr, ty)?;
+                let e = match end {
+                    RangeEnd::Included => {
+                        self.tcx.sess.emit_err(LowerRangeBoundMustBeLessThanOrEqualToUpper {
+                            span,
+                            teach: self.tcx.sess.teach(&error_code!(E0030)).then_some(()),
+                        })
                     }
-                }
-                if let Some(hir::Expr { kind: hir::ExprKind::Lit(lit), .. }) = hi_expr
-                    && let rustc_ast::ast::LitKind::Int(val, _) = lit.node
-                {
-                    if hi.eval_bits(self.tcx, self.param_env) != val {
-                        higher_overflow = true;
-                        self.tcx.sess.emit_err(LiteralOutOfRange { span: lit.span, ty, max: max() });
+                    RangeEnd::Excluded => {
+                        self.tcx.sess.emit_err(LowerRangeBoundMustBeLessThanUpper { span })
                     }
-                }
-                if !lower_overflow && !higher_overflow {
-                    self.tcx.sess.emit_err(LowerRangeBoundMustBeLessThanOrEqualToUpper {
-                        span,
-                        teach: self.tcx.sess.teach(&error_code!(E0030)).then_some(()),
-                    });
-                }
-                PatKind::Wild
+                };
+                return Err(e);
             }
-        }
-    }
+        };
 
-    fn normalize_range_pattern_ends(
-        &self,
-        ty: Ty<'tcx>,
-        lo: Option<&PatKind<'tcx>>,
-        hi: Option<&PatKind<'tcx>>,
-    ) -> Option<(mir::Const<'tcx>, mir::Const<'tcx>)> {
-        match (lo, hi) {
-            (Some(PatKind::Constant { value: lo }), Some(PatKind::Constant { value: hi })) => {
-                Some((*lo, *hi))
-            }
-            (Some(PatKind::Constant { value: lo }), None) => {
-                let hi = ty.numeric_max_val(self.tcx)?;
-                Some((*lo, mir::Const::from_ty_const(hi, self.tcx)))
-            }
-            (None, Some(PatKind::Constant { value: hi })) => {
-                let lo = ty.numeric_min_val(self.tcx)?;
-                Some((mir::Const::from_ty_const(lo, self.tcx), *hi))
+        // If we are handling a range with associated constants (e.g.
+        // `Foo::<'a>::A..=Foo::B`), we need to put the ascriptions for the associated
+        // constants somewhere. Have them on the range pattern.
+        for ascr in [lo_ascr, hi_ascr] {
+            if let Some(ascription) = ascr {
+                kind = PatKind::AscribeUserType {
+                    ascription,
+                    subpattern: Box::new(Pat { span, ty, kind }),
+                };
             }
-            _ => None,
         }
+        Ok(kind)
     }
 
     #[instrument(skip(self), level = "debug")]
@@ -220,37 +252,10 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
 
             hir::PatKind::Range(ref lo_expr, ref hi_expr, end) => {
                 let (lo_expr, hi_expr) = (lo_expr.as_deref(), hi_expr.as_deref());
-                let lo_span = lo_expr.map_or(pat.span, |e| e.span);
-                let lo = lo_expr.map(|e| self.lower_range_expr(e));
-                let hi = hi_expr.map(|e| self.lower_range_expr(e));
-
-                let (lp, hp) = (lo.as_ref().map(|(x, _)| x), hi.as_ref().map(|(x, _)| x));
-                let mut kind = match self.normalize_range_pattern_ends(ty, lp, hp) {
-                    Some((lc, hc)) => {
-                        self.lower_pattern_range(ty, lc, hc, end, lo_span, lo_expr, hi_expr)
-                    }
-                    None => {
-                        let msg = format!(
-                            "found bad range pattern `{:?}` outside of error recovery",
-                            (&lo, &hi),
-                        );
-                        self.tcx.sess.delay_span_bug(pat.span, msg);
-                        PatKind::Wild
-                    }
-                };
-
-                // If we are handling a range with associated constants (e.g.
-                // `Foo::<'a>::A..=Foo::B`), we need to put the ascriptions for the associated
-                // constants somewhere. Have them on the range pattern.
-                for end in &[lo, hi] {
-                    if let Some((_, Some(ascription))) = end {
-                        let subpattern = Box::new(Pat { span: pat.span, ty, kind });
-                        kind =
-                            PatKind::AscribeUserType { ascription: ascription.clone(), subpattern };
-                    }
-                }
-
-                kind
+                // FIXME?: returning `_` can cause inaccurate "unreachable" warnings. This can be
+                // fixed by returning `PatKind::Const(ConstKind::Error(...))` if #115937 gets
+                // merged.
+                self.lower_pattern_range(lo_expr, hi_expr, end, ty, span).unwrap_or(PatKind::Wild)
             }
 
             hir::PatKind::Path(ref qpath) => {

tests/ui/error-codes/E0030-teach.stderr

@@ -2,7 +2,7 @@ error[E0030]: lower range bound must be less than or equal to upper
   --> $DIR/E0030-teach.rs:5:9
    |
 LL |         1000 ..= 5 => {}
-   |         ^^^^ lower bound larger than upper bound
+   |         ^^^^^^^^^^ lower bound larger than upper bound
    |
    = note: When matching against a range, the compiler verifies that the range is non-empty. Range patterns include both end-points, so this is equivalent to requiring the start of the range to be less than or equal to the end of the range.