From 9e43b00d4fe2660573e40ec9125290b1d9974e44 Mon Sep 17 00:00:00 2001 From: mark Date: Thu, 30 Apr 2020 21:50:43 -0500 Subject: [PATCH 01/23] Turn of rustc-dev-guide toolstate for now --- src/ci/docker/x86_64-gnu-tools/checktools.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ci/docker/x86_64-gnu-tools/checktools.sh b/src/ci/docker/x86_64-gnu-tools/checktools.sh index 00ac20c08dc6b..c9d1cb21da8b0 100755 --- a/src/ci/docker/x86_64-gnu-tools/checktools.sh +++ b/src/ci/docker/x86_64-gnu-tools/checktools.sh @@ -14,7 +14,6 @@ python3 "$X_PY" test --no-fail-fast \ src/doc/rust-by-example \ src/doc/embedded-book \ src/doc/edition-guide \ - src/doc/rustc-dev-guide \ src/tools/clippy \ src/tools/rls \ src/tools/rustfmt \ From 75f066dc687e9a40e193ff059491b208a98291aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 7 Apr 2020 13:26:09 -0700 Subject: [PATCH 02/23] Handle binop on unbound type param When encountering a binary operation involving a type parameter that has no bindings, suggest adding the appropriate bound. --- src/librustc_hir/hir.rs | 30 +++++++ src/librustc_typeck/check/op.rs | 81 ++++++++++++++++--- src/test/ui/issues/issue-6738.stderr | 5 +- .../type/type-check/missing_trait_impl.stderr | 10 ++- 4 files changed, 110 insertions(+), 16 deletions(-) diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs index 258428d77da10..941430afaba86 100644 --- a/src/librustc_hir/hir.rs +++ b/src/librustc_hir/hir.rs @@ -2626,8 +2626,38 @@ impl Node<'_> { match self { Node::TraitItem(TraitItem { generics, .. }) | Node::ImplItem(ImplItem { generics, .. }) + | Node::Item(Item { kind: ItemKind::Trait(_, _, generics, ..), .. }) + | Node::Item(Item { kind: ItemKind::Impl { generics, .. }, .. }) | Node::Item(Item { kind: ItemKind::Fn(_, generics, _), .. }) => Some(generics), _ => None, } } + + pub fn hir_id(&self) -> Option { + match self { + Node::Item(Item { hir_id, .. }) + | Node::ForeignItem(ForeignItem { hir_id, .. }) + | Node::TraitItem(TraitItem { hir_id, .. }) + | Node::ImplItem(ImplItem { hir_id, .. }) + | Node::Field(StructField { hir_id, .. }) + | Node::AnonConst(AnonConst { hir_id, .. }) + | Node::Expr(Expr { hir_id, .. }) + | Node::Stmt(Stmt { hir_id, .. }) + | Node::Ty(Ty { hir_id, .. }) + | Node::Binding(Pat { hir_id, .. }) + | Node::Pat(Pat { hir_id, .. }) + | Node::Arm(Arm { hir_id, .. }) + | Node::Block(Block { hir_id, .. }) + | Node::Local(Local { hir_id, .. }) + | Node::MacroDef(MacroDef { hir_id, .. }) + | Node::Lifetime(Lifetime { hir_id, .. }) + | Node::Param(Param { hir_id, .. }) + | Node::GenericParam(GenericParam { hir_id, .. }) => Some(*hir_id), + Node::TraitRef(TraitRef { hir_ref_id, .. }) => Some(*hir_ref_id), + Node::PathSegment(PathSegment { hir_id, .. }) => *hir_id, + Node::Variant(Variant { id, .. }) => Some(*id), + Node::Ctor(variant) => variant.ctor_hir_id(), + Node::Crate(_) | Node::Visibility(_) => None, + } + } } diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index cac9113fd5d30..4cc68e7e54dea 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -13,6 +13,7 @@ use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, use rustc_middle::ty::{self, Ty, TypeFoldable}; use rustc_span::Span; use rustc_trait_selection::infer::InferCtxtExt; +use rustc_trait_selection::traits::error_reporting::suggest_constraining_type_param; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Checks a `a = b` @@ -253,6 +254,50 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // error types are considered "builtin" if !lhs_ty.references_error() { let source_map = self.tcx.sess.source_map(); + + let suggest_constraining_param = + |mut err: &mut DiagnosticBuilder<'_>, + missing_trait: &str, + p: ty::ParamTy, + set_output: bool| { + let hir = self.tcx.hir(); + let msg = + &format!("`{}` might need a bound for `{}`", lhs_ty, missing_trait); + if let Some(def_id) = hir + .find(hir.get_parent_item(expr.hir_id)) + .and_then(|node| node.hir_id()) + .and_then(|hir_id| hir.opt_local_def_id(hir_id)) + { + let generics = self.tcx.generics_of(def_id); + let param_def_id = generics.type_param(&p, self.tcx).def_id; + if let Some(generics) = hir + .as_local_hir_id(param_def_id) + .and_then(|id| hir.find(hir.get_parent_item(id))) + .as_ref() + .and_then(|node| node.generics()) + { + let output = if set_output { + format!("", rhs_ty) + } else { + String::new() + }; + suggest_constraining_type_param( + self.tcx, + generics, + &mut err, + &format!("{}", lhs_ty), + &format!("{}{}", missing_trait, output), + None, + ); + } else { + let span = self.tcx.def_span(param_def_id); + err.span_label(span, msg); + } + } else { + err.note(&msg); + } + }; + match is_assign { IsAssign::Yes => { let mut err = struct_span_err!( @@ -317,12 +362,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // This has nothing here because it means we did string // concatenation (e.g., "Hello " += "World!"). This means // we don't want the note in the else clause to be emitted - } else if let ty::Param(_) = lhs_ty.kind { - // FIXME: point to span of param - err.note(&format!( - "`{}` might need a bound for `{}`", - lhs_ty, missing_trait - )); + } else if let ty::Param(p) = lhs_ty.kind { + suggest_constraining_param(&mut err, missing_trait, p, false); } else if !suggested_deref { suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } @@ -330,46 +371,56 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { err.emit(); } IsAssign::No => { - let (message, missing_trait) = match op.node { + let (message, missing_trait, use_output) = match op.node { hir::BinOpKind::Add => ( format!("cannot add `{}` to `{}`", rhs_ty, lhs_ty), Some("std::ops::Add"), + true, ), hir::BinOpKind::Sub => ( format!("cannot subtract `{}` from `{}`", rhs_ty, lhs_ty), Some("std::ops::Sub"), + true, ), hir::BinOpKind::Mul => ( format!("cannot multiply `{}` to `{}`", rhs_ty, lhs_ty), Some("std::ops::Mul"), + true, ), hir::BinOpKind::Div => ( format!("cannot divide `{}` by `{}`", lhs_ty, rhs_ty), Some("std::ops::Div"), + true, ), hir::BinOpKind::Rem => ( format!("cannot mod `{}` by `{}`", lhs_ty, rhs_ty), Some("std::ops::Rem"), + true, ), hir::BinOpKind::BitAnd => ( format!("no implementation for `{} & {}`", lhs_ty, rhs_ty), Some("std::ops::BitAnd"), + true, ), hir::BinOpKind::BitXor => ( format!("no implementation for `{} ^ {}`", lhs_ty, rhs_ty), Some("std::ops::BitXor"), + true, ), hir::BinOpKind::BitOr => ( format!("no implementation for `{} | {}`", lhs_ty, rhs_ty), Some("std::ops::BitOr"), + true, ), hir::BinOpKind::Shl => ( format!("no implementation for `{} << {}`", lhs_ty, rhs_ty), Some("std::ops::Shl"), + true, ), hir::BinOpKind::Shr => ( format!("no implementation for `{} >> {}`", lhs_ty, rhs_ty), Some("std::ops::Shr"), + true, ), hir::BinOpKind::Eq | hir::BinOpKind::Ne => ( format!( @@ -378,6 +429,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { lhs_ty ), Some("std::cmp::PartialEq"), + false, ), hir::BinOpKind::Lt | hir::BinOpKind::Le @@ -389,6 +441,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { lhs_ty ), Some("std::cmp::PartialOrd"), + false, ), _ => ( format!( @@ -397,6 +450,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { lhs_ty ), None, + false, ), }; let mut err = struct_span_err!( @@ -459,12 +513,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // This has nothing here because it means we did string // concatenation (e.g., "Hello " + "World!"). This means // we don't want the note in the else clause to be emitted - } else if let ty::Param(_) = lhs_ty.kind { - // FIXME: point to span of param - err.note(&format!( - "`{}` might need a bound for `{}`", - lhs_ty, missing_trait - )); + } else if let ty::Param(p) = lhs_ty.kind { + suggest_constraining_param( + &mut err, + missing_trait, + p, + use_output, + ); } else if !suggested_deref && !involves_fn { suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } diff --git a/src/test/ui/issues/issue-6738.stderr b/src/test/ui/issues/issue-6738.stderr index 82b670bd03bc5..a428ff7e91fad 100644 --- a/src/test/ui/issues/issue-6738.stderr +++ b/src/test/ui/issues/issue-6738.stderr @@ -6,7 +6,10 @@ LL | self.x += v.x; | | | cannot use `+=` on type `T` | - = note: `T` might need a bound for `std::ops::AddAssign` +help: consider restricting type parameter `T` + | +LL | impl Foo { + | ^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error diff --git a/src/test/ui/type/type-check/missing_trait_impl.stderr b/src/test/ui/type/type-check/missing_trait_impl.stderr index 7186d6a542dc9..30df1261cefa1 100644 --- a/src/test/ui/type/type-check/missing_trait_impl.stderr +++ b/src/test/ui/type/type-check/missing_trait_impl.stderr @@ -6,7 +6,10 @@ LL | let z = x + y; | | | T | - = note: `T` might need a bound for `std::ops::Add` +help: consider restricting type parameter `T` + | +LL | fn foo>(x: T, y: T) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0368]: binary assignment operation `+=` cannot be applied to type `T` --> $DIR/missing_trait_impl.rs:9:5 @@ -16,7 +19,10 @@ LL | x += x; | | | cannot use `+=` on type `T` | - = note: `T` might need a bound for `std::ops::AddAssign` +help: consider restricting type parameter `T` + | +LL | fn bar(x: T) { + | ^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 2 previous errors From 1473a663188e9a8642abb700a23fa40369ba8e15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 7 Apr 2020 16:03:43 -0700 Subject: [PATCH 03/23] Suggest restricting type param when it doesn't satisfy projection When encountering a projection that isn't satisfied by a type parameter, suggest constraining the type parameter. --- src/librustc_middle/ty/diagnostics.rs | 184 ++++++++++++++++- src/librustc_middle/ty/error.rs | 40 +++- .../diagnostics/conflict_errors.rs | 3 +- .../traits/error_reporting/mod.rs | 186 +----------------- .../traits/error_reporting/suggestions.rs | 4 +- src/librustc_typeck/check/op.rs | 3 +- .../construct_with_other_type.stderr | 9 +- .../missing-bounds.fixed | 35 ++++ .../missing-bounds.rs | 35 ++++ .../missing-bounds.stderr | 49 +++++ src/test/ui/issues/issue-24204.stderr | 4 +- 11 files changed, 358 insertions(+), 194 deletions(-) create mode 100644 src/test/ui/generic-associated-types/missing-bounds.fixed create mode 100644 src/test/ui/generic-associated-types/missing-bounds.rs create mode 100644 src/test/ui/generic-associated-types/missing-bounds.stderr diff --git a/src/librustc_middle/ty/diagnostics.rs b/src/librustc_middle/ty/diagnostics.rs index 790eb8f49aff3..613d66d59c55b 100644 --- a/src/librustc_middle/ty/diagnostics.rs +++ b/src/librustc_middle/ty/diagnostics.rs @@ -2,7 +2,12 @@ use crate::ty::sty::InferTy; use crate::ty::TyKind::*; -use crate::ty::TyS; +use crate::ty::{TyCtxt, TyS}; +use rustc_errors::{Applicability, DiagnosticBuilder}; +use rustc_hir as hir; +use rustc_hir::def_id::DefId; +use rustc_hir::{QPath, TyKind, WhereBoundPredicate, WherePredicate}; +use rustc_span::{BytePos, Span}; impl<'tcx> TyS<'tcx> { /// Similar to `TyS::is_primitive`, but also considers inferred numeric values to be primitive. @@ -67,3 +72,180 @@ impl<'tcx> TyS<'tcx> { } } } + +/// Suggest restricting a type param with a new bound. +pub fn suggest_constraining_type_param( + tcx: TyCtxt<'_>, + generics: &hir::Generics<'_>, + err: &mut DiagnosticBuilder<'_>, + param_name: &str, + constraint: &str, + def_id: Option, +) -> bool { + let param = generics.params.iter().find(|p| p.name.ident().as_str() == param_name); + + let param = if let Some(param) = param { + param + } else { + return false; + }; + + const MSG_RESTRICT_BOUND_FURTHER: &str = "consider further restricting this bound"; + let msg_restrict_type = format!("consider restricting type parameter `{}`", param_name); + let msg_restrict_type_further = + format!("consider further restricting type parameter `{}`", param_name); + + if def_id == tcx.lang_items().sized_trait() { + // Type parameters are already `Sized` by default. + err.span_label(param.span, &format!("this type parameter needs to be `{}`", constraint)); + return true; + } + let mut suggest_restrict = |span| { + err.span_suggestion_verbose( + span, + MSG_RESTRICT_BOUND_FURTHER, + format!(" + {}", constraint), + Applicability::MachineApplicable, + ); + }; + + if param_name.starts_with("impl ") { + // If there's an `impl Trait` used in argument position, suggest + // restricting it: + // + // fn foo(t: impl Foo) { ... } + // -------- + // | + // help: consider further restricting this bound with `+ Bar` + // + // Suggestion for tools in this case is: + // + // fn foo(t: impl Foo) { ... } + // -------- + // | + // replace with: `impl Foo + Bar` + + suggest_restrict(param.span.shrink_to_hi()); + return true; + } + + if generics.where_clause.predicates.is_empty() + // Given `trait Base: Super` where `T: Copy`, suggest restricting in the + // `where` clause instead of `trait Base: Super`. + && !matches!(param.kind, hir::GenericParamKind::Type { default: Some(_), .. }) + { + if let Some(bounds_span) = param.bounds_span() { + // If user has provided some bounds, suggest restricting them: + // + // fn foo(t: T) { ... } + // --- + // | + // help: consider further restricting this bound with `+ Bar` + // + // Suggestion for tools in this case is: + // + // fn foo(t: T) { ... } + // -- + // | + // replace with: `T: Bar +` + suggest_restrict(bounds_span.shrink_to_hi()); + } else { + // If user hasn't provided any bounds, suggest adding a new one: + // + // fn foo(t: T) { ... } + // - help: consider restricting this type parameter with `T: Foo` + err.span_suggestion_verbose( + param.span.shrink_to_hi(), + &msg_restrict_type, + format!(": {}", constraint), + Applicability::MachineApplicable, + ); + } + + true + } else { + // This part is a bit tricky, because using the `where` clause user can + // provide zero, one or many bounds for the same type parameter, so we + // have following cases to consider: + // + // 1) When the type parameter has been provided zero bounds + // + // Message: + // fn foo(x: X, y: Y) where Y: Foo { ... } + // - help: consider restricting this type parameter with `where X: Bar` + // + // Suggestion: + // fn foo(x: X, y: Y) where Y: Foo { ... } + // - insert: `, X: Bar` + // + // + // 2) When the type parameter has been provided one bound + // + // Message: + // fn foo(t: T) where T: Foo { ... } + // ^^^^^^ + // | + // help: consider further restricting this bound with `+ Bar` + // + // Suggestion: + // fn foo(t: T) where T: Foo { ... } + // ^^ + // | + // replace with: `T: Bar +` + // + // + // 3) When the type parameter has been provided many bounds + // + // Message: + // fn foo(t: T) where T: Foo, T: Bar {... } + // - help: consider further restricting this type parameter with `where T: Zar` + // + // Suggestion: + // fn foo(t: T) where T: Foo, T: Bar {... } + // - insert: `, T: Zar` + + let mut param_spans = Vec::new(); + + for predicate in generics.where_clause.predicates { + if let WherePredicate::BoundPredicate(WhereBoundPredicate { + span, bounded_ty, .. + }) = predicate + { + if let TyKind::Path(QPath::Resolved(_, path)) = &bounded_ty.kind { + if let Some(segment) = path.segments.first() { + if segment.ident.to_string() == param_name { + param_spans.push(span); + } + } + } + } + } + + let where_clause_span = generics.where_clause.span_for_predicates_or_empty_place(); + // Account for `fn foo(t: T) where T: Foo,` so we don't suggest two trailing commas. + let mut trailing_comma = false; + if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(where_clause_span) { + trailing_comma = snippet.ends_with(','); + } + let where_clause_span = if trailing_comma { + let hi = where_clause_span.hi(); + Span::new(hi - BytePos(1), hi, where_clause_span.ctxt()) + } else { + where_clause_span.shrink_to_hi() + }; + + match ¶m_spans[..] { + &[¶m_span] => suggest_restrict(param_span.shrink_to_hi()), + _ => { + err.span_suggestion_verbose( + where_clause_span, + &msg_restrict_type_further, + format!(", {}: {}", param_name, constraint), + Applicability::MachineApplicable, + ); + } + } + + true + } +} diff --git a/src/librustc_middle/ty/error.rs b/src/librustc_middle/ty/error.rs index 4e1a8b0e92f13..329fd928c8717 100644 --- a/src/librustc_middle/ty/error.rs +++ b/src/librustc_middle/ty/error.rs @@ -1,4 +1,5 @@ use crate::traits::{ObligationCause, ObligationCauseCode}; +use crate::ty::diagnostics::suggest_constraining_type_param; use crate::ty::{self, BoundRegion, Region, Ty, TyCtxt}; use rustc_ast::ast; use rustc_errors::Applicability::{MachineApplicable, MaybeIncorrect}; @@ -401,8 +402,43 @@ impl<'tcx> TyCtxt<'tcx> { (ty::Projection(_), ty::Projection(_)) => { db.note("an associated type was expected, but a different one was found"); } - (ty::Param(_), ty::Projection(_)) | (ty::Projection(_), ty::Param(_)) => { - db.note("you might be missing a type parameter or trait bound"); + (ty::Param(p), ty::Projection(proj)) | (ty::Projection(proj), ty::Param(p)) => { + let generics = self.generics_of(body_owner_def_id); + let p_span = self.def_span(generics.type_param(p, self).def_id); + if !sp.contains(p_span) { + db.span_label(p_span, "this type parameter"); + } + let hir = self.hir(); + let mut note = true; + if let Some(generics) = hir + .as_local_hir_id(generics.type_param(p, self).def_id) + .and_then(|id| self.hir().find(self.hir().get_parent_node(id))) + .as_ref() + .and_then(|node| node.generics()) + { + // Synthesize the associated type restriction `Add`. + // FIXME: extract this logic for use in other diagnostics. + let trait_ref = proj.trait_ref(self); + let path = + self.def_path_str_with_substs(trait_ref.def_id, trait_ref.substs); + let item_name = self.item_name(proj.item_def_id); + let path = if path.ends_with('>') { + format!("{}, {} = {}>", &path[..path.len() - 1], item_name, p) + } else { + format!("{}<{} = {}>", path, item_name, p) + }; + note = !suggest_constraining_type_param( + self, + generics, + db, + &format!("{}", proj.self_ty()), + &path, + None, + ); + } + if note { + db.note("you might be missing a type parameter or trait bound"); + } } (ty::Param(p), ty::Dynamic(..) | ty::Opaque(..)) | (ty::Dynamic(..) | ty::Opaque(..), ty::Param(p)) => { diff --git a/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs b/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs index 5bc9f6df889c7..14a094b9d5273 100644 --- a/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs +++ b/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs @@ -10,10 +10,9 @@ use rustc_middle::mir::{ FakeReadCause, Local, LocalDecl, LocalInfo, LocalKind, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, TerminatorKind, VarBindingForm, }; -use rustc_middle::ty::{self, Ty}; +use rustc_middle::ty::{self, suggest_constraining_type_param, Ty}; use rustc_span::source_map::DesugaringKind; use rustc_span::Span; -use rustc_trait_selection::traits::error_reporting::suggest_constraining_type_param; use crate::dataflow::drop_flag_effects; use crate::dataflow::indexes::{MoveOutIndex, MovePathIndex}; diff --git a/src/librustc_trait_selection/traits/error_reporting/mod.rs b/src/librustc_trait_selection/traits/error_reporting/mod.rs index fa2af24c94534..19ed6b50f92a6 100644 --- a/src/librustc_trait_selection/traits/error_reporting/mod.rs +++ b/src/librustc_trait_selection/traits/error_reporting/mod.rs @@ -15,17 +15,16 @@ use rustc_data_structures::fx::FxHashMap; use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticBuilder, ErrorReported}; use rustc_hir as hir; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; -use rustc_hir::{Node, QPath, TyKind, WhereBoundPredicate, WherePredicate}; +use rustc_hir::Node; use rustc_middle::mir::interpret::ErrorHandled; use rustc_middle::ty::error::ExpectedFound; -use rustc_middle::ty::fast_reject; use rustc_middle::ty::fold::TypeFolder; -use rustc_middle::ty::SubtypePredicate; use rustc_middle::ty::{ - self, AdtKind, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness, + self, fast_reject, AdtKind, SubtypePredicate, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, + TypeFoldable, WithConstness, }; use rustc_session::DiagnosticMessageId; -use rustc_span::{BytePos, ExpnKind, Span, DUMMY_SP}; +use rustc_span::{ExpnKind, Span, DUMMY_SP}; use std::fmt; use crate::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -1700,180 +1699,3 @@ impl ArgKind { } } } - -/// Suggest restricting a type param with a new bound. -pub fn suggest_constraining_type_param( - tcx: TyCtxt<'_>, - generics: &hir::Generics<'_>, - err: &mut DiagnosticBuilder<'_>, - param_name: &str, - constraint: &str, - def_id: Option, -) -> bool { - let param = generics.params.iter().find(|p| p.name.ident().as_str() == param_name); - - let param = if let Some(param) = param { - param - } else { - return false; - }; - - const MSG_RESTRICT_BOUND_FURTHER: &str = "consider further restricting this bound"; - let msg_restrict_type = format!("consider restricting type parameter `{}`", param_name); - let msg_restrict_type_further = - format!("consider further restricting type parameter `{}`", param_name); - - if def_id == tcx.lang_items().sized_trait() { - // Type parameters are already `Sized` by default. - err.span_label(param.span, &format!("this type parameter needs to be `{}`", constraint)); - return true; - } - let mut suggest_restrict = |span| { - err.span_suggestion_verbose( - span, - MSG_RESTRICT_BOUND_FURTHER, - format!(" + {}", constraint), - Applicability::MachineApplicable, - ); - }; - - if param_name.starts_with("impl ") { - // If there's an `impl Trait` used in argument position, suggest - // restricting it: - // - // fn foo(t: impl Foo) { ... } - // -------- - // | - // help: consider further restricting this bound with `+ Bar` - // - // Suggestion for tools in this case is: - // - // fn foo(t: impl Foo) { ... } - // -------- - // | - // replace with: `impl Foo + Bar` - - suggest_restrict(param.span.shrink_to_hi()); - return true; - } - - if generics.where_clause.predicates.is_empty() - // Given `trait Base: Super` where `T: Copy`, suggest restricting in the - // `where` clause instead of `trait Base: Super`. - && !matches!(param.kind, hir::GenericParamKind::Type { default: Some(_), .. }) - { - if let Some(bounds_span) = param.bounds_span() { - // If user has provided some bounds, suggest restricting them: - // - // fn foo(t: T) { ... } - // --- - // | - // help: consider further restricting this bound with `+ Bar` - // - // Suggestion for tools in this case is: - // - // fn foo(t: T) { ... } - // -- - // | - // replace with: `T: Bar +` - suggest_restrict(bounds_span.shrink_to_hi()); - } else { - // If user hasn't provided any bounds, suggest adding a new one: - // - // fn foo(t: T) { ... } - // - help: consider restricting this type parameter with `T: Foo` - err.span_suggestion_verbose( - param.span.shrink_to_hi(), - &msg_restrict_type, - format!(": {}", constraint), - Applicability::MachineApplicable, - ); - } - - true - } else { - // This part is a bit tricky, because using the `where` clause user can - // provide zero, one or many bounds for the same type parameter, so we - // have following cases to consider: - // - // 1) When the type parameter has been provided zero bounds - // - // Message: - // fn foo(x: X, y: Y) where Y: Foo { ... } - // - help: consider restricting this type parameter with `where X: Bar` - // - // Suggestion: - // fn foo(x: X, y: Y) where Y: Foo { ... } - // - insert: `, X: Bar` - // - // - // 2) When the type parameter has been provided one bound - // - // Message: - // fn foo(t: T) where T: Foo { ... } - // ^^^^^^ - // | - // help: consider further restricting this bound with `+ Bar` - // - // Suggestion: - // fn foo(t: T) where T: Foo { ... } - // ^^ - // | - // replace with: `T: Bar +` - // - // - // 3) When the type parameter has been provided many bounds - // - // Message: - // fn foo(t: T) where T: Foo, T: Bar {... } - // - help: consider further restricting this type parameter with `where T: Zar` - // - // Suggestion: - // fn foo(t: T) where T: Foo, T: Bar {... } - // - insert: `, T: Zar` - - let mut param_spans = Vec::new(); - - for predicate in generics.where_clause.predicates { - if let WherePredicate::BoundPredicate(WhereBoundPredicate { - span, bounded_ty, .. - }) = predicate - { - if let TyKind::Path(QPath::Resolved(_, path)) = &bounded_ty.kind { - if let Some(segment) = path.segments.first() { - if segment.ident.to_string() == param_name { - param_spans.push(span); - } - } - } - } - } - - let where_clause_span = generics.where_clause.span_for_predicates_or_empty_place(); - // Account for `fn foo(t: T) where T: Foo,` so we don't suggest two trailing commas. - let mut trailing_comma = false; - if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(where_clause_span) { - trailing_comma = snippet.ends_with(','); - } - let where_clause_span = if trailing_comma { - let hi = where_clause_span.hi(); - Span::new(hi - BytePos(1), hi, where_clause_span.ctxt()) - } else { - where_clause_span.shrink_to_hi() - }; - - match ¶m_spans[..] { - &[¶m_span] => suggest_restrict(param_span.shrink_to_hi()), - _ => { - err.span_suggestion_verbose( - where_clause_span, - &msg_restrict_type_further, - format!(", {}: {}", param_name, constraint), - Applicability::MachineApplicable, - ); - } - } - - true - } -} diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index ce7b1390d46b6..74dd47a91c279 100644 --- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs +++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs @@ -3,7 +3,6 @@ use super::{ }; use crate::infer::InferCtxt; -use crate::traits::error_reporting::suggest_constraining_type_param; use rustc_errors::{error_code, struct_span_err, Applicability, DiagnosticBuilder, Style}; use rustc_hir as hir; @@ -13,7 +12,8 @@ use rustc_hir::intravisit::Visitor; use rustc_hir::{AsyncGeneratorKind, GeneratorKind, Node}; use rustc_middle::ty::TypeckTables; use rustc_middle::ty::{ - self, AdtKind, DefIdTree, Infer, InferTy, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness, + self, suggest_constraining_type_param, AdtKind, DefIdTree, Infer, InferTy, ToPredicate, Ty, + TyCtxt, TypeFoldable, WithConstness, }; use rustc_span::symbol::{kw, sym, Symbol}; use rustc_span::{MultiSpan, Span, DUMMY_SP}; diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 4cc68e7e54dea..89a6b98cd8f52 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -10,10 +10,9 @@ use rustc_middle::ty::adjustment::{ Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, }; use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, Uint}; -use rustc_middle::ty::{self, Ty, TypeFoldable}; +use rustc_middle::ty::{self, suggest_constraining_type_param, Ty, TypeFoldable}; use rustc_span::Span; use rustc_trait_selection::infer::InferCtxtExt; -use rustc_trait_selection::traits::error_reporting::suggest_constraining_type_param; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Checks a `a = b` diff --git a/src/test/ui/generic-associated-types/construct_with_other_type.stderr b/src/test/ui/generic-associated-types/construct_with_other_type.stderr index bad746f7ef121..b9468b3330b44 100644 --- a/src/test/ui/generic-associated-types/construct_with_other_type.stderr +++ b/src/test/ui/generic-associated-types/construct_with_other_type.stderr @@ -2,11 +2,16 @@ error[E0271]: type mismatch resolving `for<'a> <::Baa<'a> as std::ops: --> $DIR/construct_with_other_type.rs:19:9 | LL | impl Baz for T where T: Foo { - | ^^^ expected type parameter `T`, found associated type + | - ^^^ expected type parameter `T`, found associated type + | | + | this type parameter | = note: expected associated type `::Bar<'_, 'static>` found associated type `<::Quux<'_> as Foo>::Bar<'_, 'static>` - = note: you might be missing a type parameter or trait bound +help: consider further restricting this bound + | +LL | impl Baz for T where T: Foo + Baz { + | ^^^^^^^^^^^^^^^ error: aborting due to previous error diff --git a/src/test/ui/generic-associated-types/missing-bounds.fixed b/src/test/ui/generic-associated-types/missing-bounds.fixed new file mode 100644 index 0000000000000..c2e619d1a7e27 --- /dev/null +++ b/src/test/ui/generic-associated-types/missing-bounds.fixed @@ -0,0 +1,35 @@ +// run-rustfix + +use std::ops::Add; + +struct A(B); + +impl Add for A where B: Add + std::ops::Add { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + A(self.0 + rhs.0) //~ ERROR mismatched types + } +} + +struct C(B); + +impl> Add for C { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR mismatched types + } +} + +struct D(B); + +impl> Add for D { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR cannot add `B` to `B` + } +} + +fn main() {} diff --git a/src/test/ui/generic-associated-types/missing-bounds.rs b/src/test/ui/generic-associated-types/missing-bounds.rs new file mode 100644 index 0000000000000..1852ef62fe6ec --- /dev/null +++ b/src/test/ui/generic-associated-types/missing-bounds.rs @@ -0,0 +1,35 @@ +// run-rustfix + +use std::ops::Add; + +struct A(B); + +impl Add for A where B: Add { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + A(self.0 + rhs.0) //~ ERROR mismatched types + } +} + +struct C(B); + +impl Add for C { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR mismatched types + } +} + +struct D(B); + +impl Add for D { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR cannot add `B` to `B` + } +} + +fn main() {} diff --git a/src/test/ui/generic-associated-types/missing-bounds.stderr b/src/test/ui/generic-associated-types/missing-bounds.stderr new file mode 100644 index 0000000000000..630ceac093ef2 --- /dev/null +++ b/src/test/ui/generic-associated-types/missing-bounds.stderr @@ -0,0 +1,49 @@ +error[E0308]: mismatched types + --> $DIR/missing-bounds.rs:11:11 + | +LL | impl Add for A where B: Add { + | - this type parameter +... +LL | A(self.0 + rhs.0) + | ^^^^^^^^^^^^^^ expected type parameter `B`, found associated type + | + = note: expected type parameter `B` + found associated type `::Output` +help: consider further restricting this bound + | +LL | impl Add for A where B: Add + std::ops::Add { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0308]: mismatched types + --> $DIR/missing-bounds.rs:21:14 + | +LL | impl Add for C { + | - this type parameter +... +LL | Self(self.0 + rhs.0) + | ^^^^^^^^^^^^^^ expected type parameter `B`, found associated type + | + = note: expected type parameter `B` + found associated type `::Output` +help: consider further restricting this bound + | +LL | impl> Add for C { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0369]: cannot add `B` to `B` + --> $DIR/missing-bounds.rs:31:21 + | +LL | Self(self.0 + rhs.0) + | ------ ^ ----- B + | | + | B + | +help: consider restricting type parameter `B` + | +LL | impl> Add for D { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + +Some errors have detailed explanations: E0308, E0369. +For more information about an error, try `rustc --explain E0308`. diff --git a/src/test/ui/issues/issue-24204.stderr b/src/test/ui/issues/issue-24204.stderr index d69efc860059b..d5cbcf786bf1a 100644 --- a/src/test/ui/issues/issue-24204.stderr +++ b/src/test/ui/issues/issue-24204.stderr @@ -7,7 +7,9 @@ LL | type A: MultiDispatch; | -------- required by this bound in `Trait` ... LL | fn test>(b: i32) -> T where T::A: MultiDispatch { T::new(b) } - | ^^^^^^^^^^^^ expected type parameter `T`, found associated type + | - ^^^^^^^^^^^^ expected type parameter `T`, found associated type + | | + | this type parameter | = note: expected type parameter `T` found associated type `<::A as MultiDispatch>::O` From 3453db7bfcdd210a1a34c8b4e16b3114d4bee13b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 7 Apr 2020 16:51:33 -0700 Subject: [PATCH 04/23] review comments: use or-pattern --- src/librustc_hir/hir.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs index 941430afaba86..e6d673b30f7bc 100644 --- a/src/librustc_hir/hir.rs +++ b/src/librustc_hir/hir.rs @@ -2626,9 +2626,13 @@ impl Node<'_> { match self { Node::TraitItem(TraitItem { generics, .. }) | Node::ImplItem(ImplItem { generics, .. }) - | Node::Item(Item { kind: ItemKind::Trait(_, _, generics, ..), .. }) - | Node::Item(Item { kind: ItemKind::Impl { generics, .. }, .. }) - | Node::Item(Item { kind: ItemKind::Fn(_, generics, _), .. }) => Some(generics), + | Node::Item(Item { + kind: + ItemKind::Trait(_, _, generics, ..) + | ItemKind::Impl { generics, .. } + | ItemKind::Fn(_, generics, _), + .. + }) => Some(generics), _ => None, } } From d8d02f8f1806b564603982d8cf25795db744e0ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 8 Apr 2020 11:10:16 -0700 Subject: [PATCH 05/23] On incorrect equality constraint likely to be assoc type, suggest appropriate syntax When encountering `where ::Bar = B`, it is possible that `Bar` is an associated type. If so, suggest `where A: Foo`. CC #20041. --- src/librustc_ast_passes/ast_validation.rs | 94 ++++++++++++++++--- .../missing-bounds.fixed | 11 +++ .../missing-bounds.rs | 11 +++ .../missing-bounds.stderr | 30 +++++- 4 files changed, 134 insertions(+), 12 deletions(-) diff --git a/src/librustc_ast_passes/ast_validation.rs b/src/librustc_ast_passes/ast_validation.rs index 395fd7460850f..5f47c9eb2c2a6 100644 --- a/src/librustc_ast_passes/ast_validation.rs +++ b/src/librustc_ast_passes/ast_validation.rs @@ -23,6 +23,7 @@ use rustc_session::Session; use rustc_span::symbol::{kw, sym}; use rustc_span::Span; use std::mem; +use std::ops::DerefMut; const MORE_EXTERN: &str = "for more information, visit https://doc.rust-lang.org/std/keyword.extern.html"; @@ -1113,17 +1114,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> { for predicate in &generics.where_clause.predicates { if let WherePredicate::EqPredicate(ref predicate) = *predicate { - self.err_handler() - .struct_span_err( - predicate.span, - "equality constraints are not yet supported in `where` clauses", - ) - .span_label(predicate.span, "not supported") - .note( - "see issue #20041 \ - for more information", - ) - .emit(); + deny_equality_constraints(self, predicate, generics); } } @@ -1300,6 +1291,87 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } } +fn deny_equality_constraints( + this: &mut AstValidator<'_>, + predicate: &WhereEqPredicate, + generics: &Generics, +) { + let mut err = this.err_handler().struct_span_err( + predicate.span, + "equality constraints are not yet supported in `where` clauses", + ); + err.span_label(predicate.span, "not supported"); + + // Given `::Bar = RhsTy`, suggest `A: Foo`. + if let TyKind::Path(Some(qself), full_path) = &predicate.lhs_ty.kind { + if let TyKind::Path(None, path) = &qself.ty.kind { + match &path.segments[..] { + [PathSegment { ident, args: None, .. }] => { + for param in &generics.params { + if param.ident == *ident { + let param = ident; + match &full_path.segments[qself.position..] { + [PathSegment { ident, .. }] => { + // Make a new `Path` from `foo::Bar` to `Foo`. + let mut assoc_path = full_path.clone(); + // Remove `Bar` from `Foo::Bar`. + assoc_path.segments.pop(); + let len = assoc_path.segments.len() - 1; + // Build ``. + let arg = AngleBracketedArg::Constraint(AssocTyConstraint { + id: rustc_ast::node_id::DUMMY_NODE_ID, + ident: *ident, + kind: AssocTyConstraintKind::Equality { + ty: predicate.rhs_ty.clone(), + }, + span: ident.span, + }); + // Add `` to `Foo`. + match &mut assoc_path.segments[len].args { + Some(args) => match args.deref_mut() { + GenericArgs::Parenthesized(_) => continue, + GenericArgs::AngleBracketed(args) => { + args.args.push(arg); + } + }, + empty_args => { + *empty_args = AngleBracketedArgs { + span: ident.span, + args: vec![arg], + } + .into(); + } + } + err.span_suggestion_verbose( + predicate.span, + &format!( + "if `{}` is an associated type you're trying to set, \ + use the associated type binding syntax", + ident + ), + format!( + "{}: {}", + param, + pprust::path_to_string(&assoc_path) + ), + Applicability::MaybeIncorrect, + ); + } + _ => {} + }; + } + } + } + _ => {} + } + } + } + err.note( + "see issue #20041 for more information", + ); + err.emit(); +} + pub fn check_crate(session: &Session, krate: &Crate, lints: &mut LintBuffer) -> bool { let mut validator = AstValidator { session, diff --git a/src/test/ui/generic-associated-types/missing-bounds.fixed b/src/test/ui/generic-associated-types/missing-bounds.fixed index c2e619d1a7e27..364d2388741b0 100644 --- a/src/test/ui/generic-associated-types/missing-bounds.fixed +++ b/src/test/ui/generic-associated-types/missing-bounds.fixed @@ -32,4 +32,15 @@ impl> Add for D { } } +struct E(B); + +impl Add for E where B: Add, B: std::ops::Add { + //~^ ERROR equality constraints are not yet supported in `where` clauses + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR mismatched types + } +} + fn main() {} diff --git a/src/test/ui/generic-associated-types/missing-bounds.rs b/src/test/ui/generic-associated-types/missing-bounds.rs index 1852ef62fe6ec..ffafff5e9f586 100644 --- a/src/test/ui/generic-associated-types/missing-bounds.rs +++ b/src/test/ui/generic-associated-types/missing-bounds.rs @@ -32,4 +32,15 @@ impl Add for D { } } +struct E(B); + +impl Add for E where ::Output = B { + //~^ ERROR equality constraints are not yet supported in `where` clauses + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self(self.0 + rhs.0) //~ ERROR mismatched types + } +} + fn main() {} diff --git a/src/test/ui/generic-associated-types/missing-bounds.stderr b/src/test/ui/generic-associated-types/missing-bounds.stderr index 630ceac093ef2..50536fdaca96e 100644 --- a/src/test/ui/generic-associated-types/missing-bounds.stderr +++ b/src/test/ui/generic-associated-types/missing-bounds.stderr @@ -1,3 +1,15 @@ +error: equality constraints are not yet supported in `where` clauses + --> $DIR/missing-bounds.rs:37:33 + | +LL | impl Add for E where ::Output = B { + | ^^^^^^^^^^^^^^^^^^^^^^ not supported + | + = note: see issue #20041 for more information +help: if `Output` is an associated type you're trying to set, use the associated type binding syntax + | +LL | impl Add for E where B: Add { + | ^^^^^^^^^^^^^^^^^^ + error[E0308]: mismatched types --> $DIR/missing-bounds.rs:11:11 | @@ -43,7 +55,23 @@ help: consider restricting type parameter `B` LL | impl> Add for D { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 3 previous errors +error[E0308]: mismatched types + --> $DIR/missing-bounds.rs:42:14 + | +LL | impl Add for E where ::Output = B { + | - this type parameter +... +LL | Self(self.0 + rhs.0) + | ^^^^^^^^^^^^^^ expected type parameter `B`, found associated type + | + = note: expected type parameter `B` + found associated type `::Output` +help: consider further restricting type parameter `B` + | +LL | impl Add for E where ::Output = B, B: std::ops::Add { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors Some errors have detailed explanations: E0308, E0369. For more information about an error, try `rustc --explain E0308`. From c93c660b0de0f5df4ae8699e983b7cd67738b2a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 17 Apr 2020 14:14:23 -0700 Subject: [PATCH 06/23] review comments and rebase fix --- src/librustc_typeck/check/op.rs | 106 +++++++++++++++----------- src/test/ui/issues/issue-20005.stderr | 12 ++- 2 files changed, 70 insertions(+), 48 deletions(-) diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 89a6b98cd8f52..b2026266b5116 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -10,7 +10,7 @@ use rustc_middle::ty::adjustment::{ Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, }; use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, Uint}; -use rustc_middle::ty::{self, suggest_constraining_type_param, Ty, TypeFoldable}; +use rustc_middle::ty::{self, suggest_constraining_type_param, Ty, TyCtxt, TypeFoldable}; use rustc_span::Span; use rustc_trait_selection::infer::InferCtxtExt; @@ -254,49 +254,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if !lhs_ty.references_error() { let source_map = self.tcx.sess.source_map(); - let suggest_constraining_param = - |mut err: &mut DiagnosticBuilder<'_>, - missing_trait: &str, - p: ty::ParamTy, - set_output: bool| { - let hir = self.tcx.hir(); - let msg = - &format!("`{}` might need a bound for `{}`", lhs_ty, missing_trait); - if let Some(def_id) = hir - .find(hir.get_parent_item(expr.hir_id)) - .and_then(|node| node.hir_id()) - .and_then(|hir_id| hir.opt_local_def_id(hir_id)) - { - let generics = self.tcx.generics_of(def_id); - let param_def_id = generics.type_param(&p, self.tcx).def_id; - if let Some(generics) = hir - .as_local_hir_id(param_def_id) - .and_then(|id| hir.find(hir.get_parent_item(id))) - .as_ref() - .and_then(|node| node.generics()) - { - let output = if set_output { - format!("", rhs_ty) - } else { - String::new() - }; - suggest_constraining_type_param( - self.tcx, - generics, - &mut err, - &format!("{}", lhs_ty), - &format!("{}{}", missing_trait, output), - None, - ); - } else { - let span = self.tcx.def_span(param_def_id); - err.span_label(span, msg); - } - } else { - err.note(&msg); - } - }; - match is_assign { IsAssign::Yes => { let mut err = struct_span_err!( @@ -362,7 +319,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // concatenation (e.g., "Hello " += "World!"). This means // we don't want the note in the else clause to be emitted } else if let ty::Param(p) = lhs_ty.kind { - suggest_constraining_param(&mut err, missing_trait, p, false); + suggest_constraining_param( + self.tcx, + &mut err, + lhs_ty, + rhs_ty, + &expr, + missing_trait, + p, + false, + ); } else if !suggested_deref { suggest_impl_missing(&mut err, lhs_ty, &missing_trait); } @@ -514,7 +480,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // we don't want the note in the else clause to be emitted } else if let ty::Param(p) = lhs_ty.kind { suggest_constraining_param( + self.tcx, &mut err, + lhs_ty, + rhs_ty, + &expr, missing_trait, p, use_output, @@ -965,3 +935,49 @@ fn suggest_impl_missing(err: &mut DiagnosticBuilder<'_>, ty: Ty<'_>, missing_tra } } } + +fn suggest_constraining_param( + tcx: TyCtxt<'_>, + mut err: &mut DiagnosticBuilder<'_>, + lhs_ty: Ty<'_>, + rhs_ty: Ty<'_>, + expr: &hir::Expr<'_>, + missing_trait: &str, + p: ty::ParamTy, + set_output: bool, +) { + let hir = tcx.hir(); + let msg = &format!("`{}` might need a bound for `{}`", lhs_ty, missing_trait); + // Try to find the def-id and details for the parameter p. We have only the index, + // so we have to find the enclosing function's def-id, then look through its declared + // generic parameters to get the declaration. + if let Some(def_id) = hir + .find(hir.get_parent_item(expr.hir_id)) + .and_then(|node| node.hir_id()) + .and_then(|hir_id| hir.opt_local_def_id(hir_id)) + { + let generics = tcx.generics_of(def_id); + let param_def_id = generics.type_param(&p, tcx).def_id; + if let Some(generics) = hir + .as_local_hir_id(param_def_id) + .and_then(|id| hir.find(hir.get_parent_item(id))) + .as_ref() + .and_then(|node| node.generics()) + { + let output = if set_output { format!("", rhs_ty) } else { String::new() }; + suggest_constraining_type_param( + tcx, + generics, + &mut err, + &format!("{}", lhs_ty), + &format!("{}{}", missing_trait, output), + None, + ); + } else { + let span = tcx.def_span(param_def_id); + err.span_label(span, msg); + } + } else { + err.note(&msg); + } +} diff --git a/src/test/ui/issues/issue-20005.stderr b/src/test/ui/issues/issue-20005.stderr index 19ccf7076199a..f53489a99f3b4 100644 --- a/src/test/ui/issues/issue-20005.stderr +++ b/src/test/ui/issues/issue-20005.stderr @@ -5,12 +5,18 @@ LL | trait From { | --- required by this bound in `From` ... LL | ) -> >::Result where Dst: From { - | ^^^^^^^^^^- help: consider further restricting `Self`: `, Self: std::marker::Sized` - | | - | doesn't have a size known at compile-time + | ^^^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `std::marker::Sized` is not implemented for `Self` = note: to learn more, visit +help: consider further restricting `Self` + | +LL | ) -> >::Result where Dst: From, Self: std::marker::Sized { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: consider relaxing the implicit `Sized` restriction + | +LL | trait From { + | ^^^^^^^^ error: aborting due to previous error From 5d64e914585e10b82ac4997b1d2c16585b8a77e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 18 Apr 2020 18:14:25 -0700 Subject: [PATCH 07/23] review comment: use `body_id` --- src/librustc_typeck/check/op.rs | 51 ++++++++++++++------------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index b2026266b5116..04267ccba669f 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -321,10 +321,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } else if let ty::Param(p) = lhs_ty.kind { suggest_constraining_param( self.tcx, + self.body_id, &mut err, lhs_ty, rhs_ty, - &expr, missing_trait, p, false, @@ -481,10 +481,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } else if let ty::Param(p) = lhs_ty.kind { suggest_constraining_param( self.tcx, + self.body_id, &mut err, lhs_ty, rhs_ty, - &expr, missing_trait, p, use_output, @@ -938,10 +938,10 @@ fn suggest_impl_missing(err: &mut DiagnosticBuilder<'_>, ty: Ty<'_>, missing_tra fn suggest_constraining_param( tcx: TyCtxt<'_>, + body_id: hir::HirId, mut err: &mut DiagnosticBuilder<'_>, lhs_ty: Ty<'_>, rhs_ty: Ty<'_>, - expr: &hir::Expr<'_>, missing_trait: &str, p: ty::ParamTy, set_output: bool, @@ -951,33 +951,26 @@ fn suggest_constraining_param( // Try to find the def-id and details for the parameter p. We have only the index, // so we have to find the enclosing function's def-id, then look through its declared // generic parameters to get the declaration. - if let Some(def_id) = hir - .find(hir.get_parent_item(expr.hir_id)) - .and_then(|node| node.hir_id()) - .and_then(|hir_id| hir.opt_local_def_id(hir_id)) + let def_id = hir.body_owner_def_id(hir::BodyId { hir_id: body_id }); + let generics = tcx.generics_of(def_id); + let param_def_id = generics.type_param(&p, tcx).def_id; + if let Some(generics) = hir + .as_local_hir_id(param_def_id) + .and_then(|id| hir.find(hir.get_parent_item(id))) + .as_ref() + .and_then(|node| node.generics()) { - let generics = tcx.generics_of(def_id); - let param_def_id = generics.type_param(&p, tcx).def_id; - if let Some(generics) = hir - .as_local_hir_id(param_def_id) - .and_then(|id| hir.find(hir.get_parent_item(id))) - .as_ref() - .and_then(|node| node.generics()) - { - let output = if set_output { format!("", rhs_ty) } else { String::new() }; - suggest_constraining_type_param( - tcx, - generics, - &mut err, - &format!("{}", lhs_ty), - &format!("{}{}", missing_trait, output), - None, - ); - } else { - let span = tcx.def_span(param_def_id); - err.span_label(span, msg); - } + let output = if set_output { format!("", rhs_ty) } else { String::new() }; + suggest_constraining_type_param( + tcx, + generics, + &mut err, + &format!("{}", lhs_ty), + &format!("{}{}", missing_trait, output), + None, + ); } else { - err.note(&msg); + let span = tcx.def_span(param_def_id); + err.span_label(span, msg); } } From b13f234a1bb99b0cde82fb5c4a9de2eaf7b5dc87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 4 May 2020 11:02:20 -0700 Subject: [PATCH 08/23] fix rebase --- src/librustc_middle/ty/error.rs | 7 +++++-- src/librustc_typeck/check/op.rs | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/librustc_middle/ty/error.rs b/src/librustc_middle/ty/error.rs index 329fd928c8717..f3b6a53dfeb82 100644 --- a/src/librustc_middle/ty/error.rs +++ b/src/librustc_middle/ty/error.rs @@ -410,8 +410,11 @@ impl<'tcx> TyCtxt<'tcx> { } let hir = self.hir(); let mut note = true; - if let Some(generics) = hir - .as_local_hir_id(generics.type_param(p, self).def_id) + if let Some(generics) = generics + .type_param(p, self) + .def_id + .as_local() + .map(|id| hir.as_local_hir_id(id)) .and_then(|id| self.hir().find(self.hir().get_parent_node(id))) .as_ref() .and_then(|node| node.generics()) diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 04267ccba669f..688145fd0deda 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -954,8 +954,9 @@ fn suggest_constraining_param( let def_id = hir.body_owner_def_id(hir::BodyId { hir_id: body_id }); let generics = tcx.generics_of(def_id); let param_def_id = generics.type_param(&p, tcx).def_id; - if let Some(generics) = hir - .as_local_hir_id(param_def_id) + if let Some(generics) = param_def_id + .as_local() + .map(|id| hir.as_local_hir_id(id)) .and_then(|id| hir.find(hir.get_parent_item(id))) .as_ref() .and_then(|node| node.generics()) From b17b20cafc87193dace2beb1cdb0e126d944f8e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 4 May 2020 11:09:10 -0700 Subject: [PATCH 09/23] Add docstring to `deny_equality_constraints` --- src/librustc_ast_passes/ast_validation.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/librustc_ast_passes/ast_validation.rs b/src/librustc_ast_passes/ast_validation.rs index 5f47c9eb2c2a6..cc88fbb295c68 100644 --- a/src/librustc_ast_passes/ast_validation.rs +++ b/src/librustc_ast_passes/ast_validation.rs @@ -1291,6 +1291,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } } +/// When encountering an equality constraint in a `where` clause, emit an error. If the code seems +/// like it's setting an associated type, provide an appropriate suggestion. fn deny_equality_constraints( this: &mut AstValidator<'_>, predicate: &WhereEqPredicate, From ab7360d98f802ce32e9e97ec4aaa180140169f92 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Mon, 4 May 2020 20:48:17 +0200 Subject: [PATCH 10/23] refactor suggest_traits_to_import --- src/librustc_typeck/check/method/suggest.rs | 104 +++++++++----------- 1 file changed, 49 insertions(+), 55 deletions(-) diff --git a/src/librustc_typeck/check/method/suggest.rs b/src/librustc_typeck/check/method/suggest.rs index 228c40ac8538b..857cc972559e4 100644 --- a/src/librustc_typeck/check/method/suggest.rs +++ b/src/librustc_typeck/check/method/suggest.rs @@ -947,65 +947,59 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // this isn't perfect (that is, there are cases when // implementing a trait would be legal but is rejected // here). - !unsatisfied_predicates.iter().any(|(p, _)| match p { - // Hide traits if they are present in predicates as they can be fixed without - // having to implement them. - ty::Predicate::Trait(t, _) => t.def_id() != info.def_id, - ty::Predicate::Projection(p) => p.item_def_id() != info.def_id, - _ => true, - }) && (type_is_local || info.def_id.is_local()) - && self - .associated_item(info.def_id, item_name, Namespace::ValueNS) - .filter(|item| { - if let ty::AssocKind::Fn = item.kind { - let id = item.def_id.as_local().map(|def_id| { - self.tcx.hir().as_local_hir_id(def_id) - }); - if let Some(hir::Node::TraitItem(hir::TraitItem { - kind: hir::TraitItemKind::Fn(fn_sig, method), - .. - })) = id.map(|id| self.tcx.hir().get(id)) + unsatisfied_predicates.iter().all(|(p, _)| match p { + // Hide traits if they are present in predicates as they can be fixed without + // having to implement them. + ty::Predicate::Trait(t, _) => t.def_id() == info.def_id, + ty::Predicate::Projection(p) => p.item_def_id() == info.def_id, + _ => false, + }) && (type_is_local || info.def_id.is_local()) + && self + .associated_item(info.def_id, item_name, Namespace::ValueNS) + .filter(|item| { + if let ty::AssocKind::Fn = item.kind { + let id = item + .def_id + .as_local() + .map(|def_id| self.tcx.hir().as_local_hir_id(def_id)); + if let Some(hir::Node::TraitItem(hir::TraitItem { + kind: hir::TraitItemKind::Fn(fn_sig, method), + .. + })) = id.map(|id| self.tcx.hir().get(id)) + { + let self_first_arg = match method { + hir::TraitFn::Required([ident, ..]) => { + ident.name == kw::SelfLower + } + hir::TraitFn::Provided(body_id) => { + self.tcx.hir().body(*body_id).params.first().map_or( + false, + |param| { + matches!( + param.pat.kind, + hir::PatKind::Binding(_, _, ident, _) + if ident.name == kw::SelfLower + ) + }, + ) + } + _ => false, + }; + + if !fn_sig.decl.implicit_self.has_implicit_self() + && self_first_arg { - let self_first_arg = match method { - hir::TraitFn::Required([ident, ..]) => { - ident.name == kw::SelfLower - } - hir::TraitFn::Provided(body_id) => { - match &self.tcx.hir().body(*body_id).params[..] { - [hir::Param { - pat: - hir::Pat { - kind: - hir::PatKind::Binding( - _, - _, - ident, - .., - ), - .. - }, - .. - }, ..] => ident.name == kw::SelfLower, - _ => false, - } - } - _ => false, - }; - - if !fn_sig.decl.implicit_self.has_implicit_self() - && self_first_arg - { - if let Some(ty) = fn_sig.decl.inputs.get(0) { - arbitrary_rcvr.push(ty.span); - } - return false; + if let Some(ty) = fn_sig.decl.inputs.get(0) { + arbitrary_rcvr.push(ty.span); } + return false; } } - // We only want to suggest public or local traits (#45781). - item.vis == ty::Visibility::Public || info.def_id.is_local() - }) - .is_some() + } + // We only want to suggest public or local traits (#45781). + item.vis == ty::Visibility::Public || info.def_id.is_local() + }) + .is_some() }) .collect::>(); for span in &arbitrary_rcvr { From e17f36b82ee90caff7e7e854ee49d0f0b525e20b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 5 May 2020 13:48:32 +0200 Subject: [PATCH 11/23] Replace title "Methods" with "Implementations" --- src/librustdoc/html/render.rs | 22 +++++++++++----------- src/librustdoc/html/static/main.js | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 387ef03f06773..666e59b9a045e 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -3413,8 +3413,8 @@ fn render_assoc_items( write!( w, "\ -

\ + Implementations\

\ " ); @@ -3475,10 +3475,10 @@ fn render_assoc_items( write!( w, "\ -

\ - Trait Implementations\ +

\ + Trait Implementations\

\ -
{}
", +
{}
", impls ); } @@ -4097,8 +4097,8 @@ fn sidebar_assoc_items(it: &clean::Item) -> String { ret.sort(); if !ret.is_empty() { out.push_str(&format!( - "Methods\ -
{}
", + "Methods\ +
{}
", ret.join("") )); } @@ -4191,8 +4191,8 @@ fn sidebar_assoc_items(it: &clean::Item) -> String { if !concrete_format.is_empty() { out.push_str( - "\ - Trait Implementations", + "\ + Trait Implementations", ); out.push_str(&format!("
{}
", concrete_format)); } @@ -4200,7 +4200,7 @@ fn sidebar_assoc_items(it: &clean::Item) -> String { if !synthetic_format.is_empty() { out.push_str( "\ - Auto Trait Implementations", + Auto Trait Implementations", ); out.push_str(&format!("
{}
", synthetic_format)); } @@ -4208,7 +4208,7 @@ fn sidebar_assoc_items(it: &clean::Item) -> String { if !blanket_format.is_empty() { out.push_str( "\ - Blanket Implementations", + Blanket Implementations", ); out.push_str(&format!("
{}
", blanket_format)); } diff --git a/src/librustdoc/html/static/main.js b/src/librustdoc/html/static/main.js index 3f12fb893a440..a023d5a2d95f1 100644 --- a/src/librustdoc/html/static/main.js +++ b/src/librustdoc/html/static/main.js @@ -2180,7 +2180,7 @@ function getSearchElement() { if (collapse) { toggleAllDocs(pageId, true); } else if (getCurrentValue("rustdoc-auto-hide-trait-implementations") !== "false") { - var impl_list = document.getElementById("implementations-list"); + var impl_list = document.getElementById("trait-implementations-list"); if (impl_list !== null) { onEachLazy(impl_list.getElementsByClassName("collapse-toggle"), function(e) { From cf184823d1d5b290e051ae1d6344865dfed97d1e Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 5 May 2020 13:48:47 +0200 Subject: [PATCH 12/23] Update tests --- src/test/rustdoc/const-generics/add-impl.rs | 2 +- src/test/rustdoc/duplicate_impls/issue-33054.rs | 2 +- src/test/rustdoc/issue-21474.rs | 2 +- src/test/rustdoc/issue-45584.rs | 4 ++-- src/test/rustdoc/issue-55321.rs | 4 ++-- src/test/rustdoc/negative-impl-sidebar.rs | 2 +- src/test/rustdoc/synthetic_auto/manual.rs | 4 ++-- src/test/rustdoc/typedef.rs | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/test/rustdoc/const-generics/add-impl.rs b/src/test/rustdoc/const-generics/add-impl.rs index 54bdd768f8a73..905f958826897 100644 --- a/src/test/rustdoc/const-generics/add-impl.rs +++ b/src/test/rustdoc/const-generics/add-impl.rs @@ -11,7 +11,7 @@ pub struct Simd { inner: T, } -// @has foo/struct.Simd.html '//div[@id="implementations-list"]/h3/code' 'impl Add> for Simd' +// @has foo/struct.Simd.html '//div[@id="trait-implementations-list"]/h3/code' 'impl Add> for Simd' impl Add for Simd { type Output = Self; diff --git a/src/test/rustdoc/duplicate_impls/issue-33054.rs b/src/test/rustdoc/duplicate_impls/issue-33054.rs index 3f7cec1856331..112d632971a5f 100644 --- a/src/test/rustdoc/duplicate_impls/issue-33054.rs +++ b/src/test/rustdoc/duplicate_impls/issue-33054.rs @@ -1,7 +1,7 @@ // @has issue_33054/impls/struct.Foo.html // @has - '//code' 'impl Foo' // @has - '//code' 'impl Bar for Foo' -// @count - '//*[@id="implementations-list"]/*[@class="impl"]' 1 +// @count - '//*[@id="trait-implementations-list"]/*[@class="impl"]' 1 // @count - '//*[@id="main"]/*[@class="impl"]' 1 // @has issue_33054/impls/bar/trait.Bar.html // @has - '//code' 'impl Bar for Foo' diff --git a/src/test/rustdoc/issue-21474.rs b/src/test/rustdoc/issue-21474.rs index 4c530f72b8ab6..896fc1a78f13f 100644 --- a/src/test/rustdoc/issue-21474.rs +++ b/src/test/rustdoc/issue-21474.rs @@ -7,5 +7,5 @@ mod inner { pub trait Blah { } // @count issue_21474/struct.What.html \ -// '//*[@id="implementations-list"]/*[@class="impl"]' 1 +// '//*[@id="trait-implementations-list"]/*[@class="impl"]' 1 pub struct What; diff --git a/src/test/rustdoc/issue-45584.rs b/src/test/rustdoc/issue-45584.rs index cd8c275d8527b..0225c0c5c2fa7 100644 --- a/src/test/rustdoc/issue-45584.rs +++ b/src/test/rustdoc/issue-45584.rs @@ -4,12 +4,12 @@ pub trait Bar {} // @has 'foo/struct.Foo1.html' pub struct Foo1; -// @count - '//*[@id="implementations-list"]/*[@class="impl"]' 1 +// @count - '//*[@id="trait-implementations-list"]/*[@class="impl"]' 1 // @has - '//*[@class="impl"]' "impl Bar for Foo1" impl Bar for Foo1 {} // @has 'foo/struct.Foo2.html' pub struct Foo2; -// @count - '//*[@id="implementations-list"]/*[@class="impl"]' 1 +// @count - '//*[@id="trait-implementations-list"]/*[@class="impl"]' 1 // @has - '//*[@class="impl"]' "impl Bar<&'static Foo2, Foo2> for u8" impl Bar<&'static Foo2, Foo2> for u8 {} diff --git a/src/test/rustdoc/issue-55321.rs b/src/test/rustdoc/issue-55321.rs index 8c001db06c5bf..d312a5114595a 100644 --- a/src/test/rustdoc/issue-55321.rs +++ b/src/test/rustdoc/issue-55321.rs @@ -1,8 +1,8 @@ #![feature(negative_impls)] // @has issue_55321/struct.A.html -// @has - '//*[@id="implementations-list"]/*[@class="impl"]//code' "impl !Send for A" -// @has - '//*[@id="implementations-list"]/*[@class="impl"]//code' "impl !Sync for A" +// @has - '//*[@id="trait-implementations-list"]/*[@class="impl"]//code' "impl !Send for A" +// @has - '//*[@id="trait-implementations-list"]/*[@class="impl"]//code' "impl !Sync for A" pub struct A(); impl !Send for A {} diff --git a/src/test/rustdoc/negative-impl-sidebar.rs b/src/test/rustdoc/negative-impl-sidebar.rs index cb46d1778d924..3414d9540776a 100644 --- a/src/test/rustdoc/negative-impl-sidebar.rs +++ b/src/test/rustdoc/negative-impl-sidebar.rs @@ -4,6 +4,6 @@ pub struct Foo; // @has foo/struct.Foo.html -// @has - '//*[@class="sidebar-title"][@href="#implementations"]' 'Trait Implementations' +// @has - '//*[@class="sidebar-title"][@href="#trait-implementations"]' 'Trait Implementations' // @has - '//*[@class="sidebar-links"]/a' '!Sync' impl !Sync for Foo {} diff --git a/src/test/rustdoc/synthetic_auto/manual.rs b/src/test/rustdoc/synthetic_auto/manual.rs index 458403462d64a..d20b4744af15b 100644 --- a/src/test/rustdoc/synthetic_auto/manual.rs +++ b/src/test/rustdoc/synthetic_auto/manual.rs @@ -2,10 +2,10 @@ // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//code' 'impl Sync for \ // Foo where T: Sync' // -// @has - '//*[@id="implementations-list"]/*[@class="impl"]//code' \ +// @has - '//*[@id="trait-implementations-list"]/*[@class="impl"]//code' \ // 'impl Send for Foo' // -// @count - '//*[@id="implementations-list"]/*[@class="impl"]' 1 +// @count - '//*[@id="trait-implementations-list"]/*[@class="impl"]' 1 // @count - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]' 4 pub struct Foo { field: T, diff --git a/src/test/rustdoc/typedef.rs b/src/test/rustdoc/typedef.rs index 80351ff52f5cc..7f834d3d5a512 100644 --- a/src/test/rustdoc/typedef.rs +++ b/src/test/rustdoc/typedef.rs @@ -13,8 +13,8 @@ impl MyStruct { // @has - '//*[@class="impl"]//code' 'impl MyTrait for MyAlias' // @has - 'Alias docstring' // @has - '//*[@class="sidebar"]//p[@class="location"]' 'Type Definition MyAlias' -// @has - '//*[@class="sidebar"]//a[@href="#methods"]' 'Methods' -// @has - '//*[@class="sidebar"]//a[@href="#implementations"]' 'Trait Implementations' +// @has - '//*[@class="sidebar"]//a[@href="#implementations"]' 'Methods' +// @has - '//*[@class="sidebar"]//a[@href="#trait-implementations"]' 'Trait Implementations' /// Alias docstring pub type MyAlias = MyStruct; From 4ade6eb2a27d0973a9912d609e8e46dd4bde04fb Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 5 May 2020 13:53:42 +0200 Subject: [PATCH 13/23] Add test for new implementations section title --- src/test/rustdoc/struct-implementations-title.rs | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 src/test/rustdoc/struct-implementations-title.rs diff --git a/src/test/rustdoc/struct-implementations-title.rs b/src/test/rustdoc/struct-implementations-title.rs new file mode 100644 index 0000000000000..96eb11311d6b4 --- /dev/null +++ b/src/test/rustdoc/struct-implementations-title.rs @@ -0,0 +1,9 @@ +#![crate_name = "foo"] + +pub struct Struc; + +// @has foo/struct.Struc.html +// @has - '//*[@id="main"]/h2[@id="implementations"]' "Implementations" +impl Struc { + pub const S: u64 = 0; +} From 758519c5f0d876a91e3c9a44d473cd01e4c031bb Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 5 May 2020 23:41:59 +0200 Subject: [PATCH 14/23] Index IDs already used by rustdoc template --- src/librustdoc/html/markdown.rs | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 4bb50f7579197..550f672ed4cf6 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -1129,9 +1129,36 @@ pub struct IdMap { map: FxHashMap, } +fn init_id_map() -> FxHashMap { + let mut map = FxHashMap::default(); + // This is the list of IDs used by rustdoc templates. + map.insert("mainThemeStyle".to_owned(), 1); + map.insert("themeStyle".to_owned(), 1); + map.insert("theme-picker".to_owned(), 1); + map.insert("theme-choices".to_owned(), 1); + map.insert("settings-menu".to_owned(), 1); + map.insert("main".to_owned(), 1); + map.insert("search".to_owned(), 1); + map.insert("crate-search".to_owned(), 1); + map.insert("render-detail".to_owned(), 1); + map.insert("toggle-all-docs".to_owned(), 1); + map.insert("all-types".to_owned(), 1); + // This is the list of IDs used by rustdoc sections. + map.insert("fields".to_owned(), 1); + map.insert("variants".to_owned(), 1); + map.insert("implementors-list".to_owned(), 1); + map.insert("synthetic-implementors-list".to_owned(), 1); + map.insert("implementations".to_owned(), 1); + map.insert("trait-implementations".to_owned(), 1); + map.insert("synthetic-implementations".to_owned(), 1); + map.insert("blanket-implementations".to_owned(), 1); + map.insert("deref-methods".to_owned(), 1); + map +} + impl IdMap { pub fn new() -> Self { - IdMap::default() + IdMap { map: init_id_map() } } pub fn populate>(&mut self, ids: I) { @@ -1141,7 +1168,7 @@ impl IdMap { } pub fn reset(&mut self) { - self.map = FxHashMap::default(); + self.map = init_id_map(); } pub fn derive(&mut self, candidate: String) -> String { From e0320b5f58ad96561e39c20c7da05d4bc1c84325 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 00:07:53 +0200 Subject: [PATCH 15/23] validation: port more checks to the pattern-based macro (and give it the shorter name) --- src/librustc_middle/mir/interpret/error.rs | 5 ++ src/librustc_mir/interpret/operand.rs | 2 +- src/librustc_mir/interpret/validity.rs | 77 ++++++++++++++-------- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/librustc_middle/mir/interpret/error.rs b/src/librustc_middle/mir/interpret/error.rs index 4b88467ac110f..1c4309dbe3a5b 100644 --- a/src/librustc_middle/mir/interpret/error.rs +++ b/src/librustc_middle/mir/interpret/error.rs @@ -380,6 +380,8 @@ pub enum UndefinedBehaviorInfo { InvalidDiscriminant(ScalarMaybeUndef), /// Using a pointer-not-to-a-function as function pointer. InvalidFunctionPointer(Pointer), + /// Using a string that is not valid UTF-8, + InvalidStr(std::str::Utf8Error), /// Using uninitialized data where it is not allowed. InvalidUndefBytes(Option), /// Working with a local that is not currently live. @@ -446,6 +448,9 @@ impl fmt::Display for UndefinedBehaviorInfo { InvalidFunctionPointer(p) => { write!(f, "using {} as function pointer but it does not point to a function", p) } + InvalidStr(err) => { + write!(f, "this string is not valid UTF-8: {}", err) + } InvalidUndefBytes(Some(p)) => write!( f, "reading uninitialized memory at {}, but this operation requires initialized memory", diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index cc9fc170f6e79..e8ef330c76cf0 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -328,7 +328,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let len = mplace.len(self)?; let bytes = self.memory.read_bytes(mplace.ptr, Size::from_bytes(len))?; let str = ::std::str::from_utf8(bytes) - .map_err(|err| err_ub_format!("this string is not valid UTF-8: {}", err))?; + .map_err(|err| err_ub!(InvalidStr(err)))?; Ok(str) } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index b6991349ff4dc..620333f430429 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -38,12 +38,12 @@ macro_rules! throw_validation_failure { } /// Returns a validation failure for any Err value of $e. -// FIXME: Replace all usages of try_validation! with try_validation_pat!. -macro_rules! try_validation { +// FIXME: Replace all usages of try_validation_catchall! with try_validation!. +macro_rules! try_validation_catchall { ($e:expr, $what:expr, $where:expr $(, $expected:expr )?) => {{ - try_validation_pat!($e, $where, { + try_validation!($e, $where, _ => { "{}", $what } $( expected { "{}", $expected } )?, - }) + ) }}; } /// Like try_validation, but will throw a validation error if any of the patterns in $p are @@ -51,7 +51,7 @@ macro_rules! try_validation { /// as a kind of validation blacklist: /// /// ``` -/// let v = try_validation_pat!(some_fn(), some_path, { +/// let v = try_validation!(some_fn(), some_path, { /// Foo | Bar | Baz => { "some failure" }, /// }); /// // Failures that match $p are thrown up as validation errors, but other errors are passed back @@ -61,7 +61,7 @@ macro_rules! try_validation { /// An additional expected parameter can also be added to the failure message: /// /// ``` -/// let v = try_validation_pat!(some_fn(), some_path, { +/// let v = try_validation!(some_fn(), some_path, { /// Foo | Bar | Baz => { "some failure" } expected { "something that wasn't a failure" }, /// }); /// ``` @@ -70,14 +70,15 @@ macro_rules! try_validation { /// the format string in directly: /// /// ``` -/// let v = try_validation_pat!(some_fn(), some_path, { +/// let v = try_validation!(some_fn(), some_path, { /// Foo | Bar | Baz => { "{:?}", some_failure } expected { "{}", expected_value }, /// }); /// ``` /// -macro_rules! try_validation_pat { - ($e:expr, $where:expr, { $( $p:pat )|+ => - { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )? $( , )?}) => {{ +macro_rules! try_validation { + ($e:expr, $where:expr, + $( $p:pat )|+ => { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )? $( , )? + ) => {{ match $e { Ok(x) => x, // We catch the error and turn it into a validation failure. We are okay with @@ -303,21 +304,28 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' match tail.kind { ty::Dynamic(..) => { let vtable = meta.unwrap_meta(); + // Direct call to `check_ptr_access_align` checks alignment even on CTFE machines. try_validation!( - self.ecx.memory.check_ptr_access( + self.ecx.memory.check_ptr_access_align( vtable, 3 * self.ecx.tcx.data_layout.pointer_size, // drop, size, align - self.ecx.tcx.data_layout.pointer_align.abi, + Some(self.ecx.tcx.data_layout.pointer_align.abi), + CheckInAllocMsg::InboundsTest, ), - "dangling or unaligned vtable pointer in wide pointer or too small vtable", - self.path + self.path, + err_ub!(PointerOutOfBounds { .. }) | + err_ub!(AlignmentCheckFailed { .. }) | + err_ub!(DanglingIntPointer(..)) | + err_unsup!(ReadBytesAsPointer) => { + "dangling or unaligned vtable pointer in wide pointer or too small vtable" + }, ); - try_validation!( + try_validation_catchall!( self.ecx.read_drop_type_from_vtable(vtable), "invalid drop fn in vtable", self.path ); - try_validation!( + try_validation_catchall!( self.ecx.read_size_and_align_from_vtable(vtable), "invalid size or align in vtable", self.path @@ -327,8 +335,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' ty::Slice(..) | ty::Str => { let _len = try_validation!( meta.unwrap_meta().to_machine_usize(self.ecx), - "non-integer slice length in wide pointer", - self.path + self.path, + err_unsup!(ReadPointerAsBytes) => { "non-integer slice length in wide pointer" }, ); // We do not check that `len * elem_size <= isize::MAX`: // that is only required for references, and there it falls out of the @@ -354,8 +362,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // Check metadata early, for better diagnostics let place = try_validation!( self.ecx.ref_to_mplace(value), - format_args!("uninitialized {}", kind), - self.path + self.path, + err_ub!(InvalidUndefBytes(..)) => { "uninitialized {}", kind }, ); if place.layout.is_unsized() { self.check_wide_ptr_meta(place.meta, place.layout)?; @@ -376,6 +384,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // alignment and size determined by the layout (size will be 0, // alignment should take attributes into account). .unwrap_or_else(|| (place.layout.size, place.layout.align.abi)); + // Direct call to `check_ptr_access_align` checks alignment even on CTFE machines. let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align( place.ptr, size, @@ -489,12 +498,20 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' match ty.kind { ty::Bool => { let value = self.ecx.read_scalar(value)?; - try_validation!(value.to_bool(), value, self.path, "a boolean"); + try_validation!( + value.to_bool(), + self.path, + err_ub!(InvalidBool(..)) => { "{}", value } expected { "a boolean" }, + ); Ok(true) } ty::Char => { let value = self.ecx.read_scalar(value)?; - try_validation!(value.to_char(), value, self.path, "a valid unicode codepoint"); + try_validation!( + value.to_char(), + self.path, + err_ub!(InvalidChar(..)) => { "{}", value } expected { "a valid unicode codepoint" }, + ); Ok(true) } ty::Float(_) | ty::Int(_) | ty::Uint(_) => { @@ -521,9 +538,11 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // We are conservative with undef for integers, but try to // actually enforce the strict rules for raw pointers (mostly because // that lets us re-use `ref_to_mplace`). - let place = try_validation_pat!(self.ecx.ref_to_mplace(self.ecx.read_immediate(value)?), self.path, { + let place = try_validation!( + self.ecx.ref_to_mplace(self.ecx.read_immediate(value)?), + self.path, err_ub!(InvalidUndefBytes(..)) => { "uninitialized raw pointer" }, - }); + ); if place.layout.is_unsized() { self.check_wide_ptr_meta(place.meta, place.layout)?; } @@ -539,7 +558,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' } ty::FnPtr(_sig) => { let value = self.ecx.read_scalar(value)?; - let _fn = try_validation!( + let _fn = try_validation_catchall!( value.not_undef().and_then(|ptr| self.ecx.memory.get_fn(ptr)), value, self.path, @@ -598,9 +617,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // At least one value is excluded. Get the bits. let value = try_validation!( value.not_undef(), - value, self.path, - format_args!("something {}", wrapping_range_format(valid_range, max_hi),) + err_ub!(InvalidUndefBytes(..)) => { "{}", value } + expected { "something {}", wrapping_range_format(valid_range, max_hi) }, ); let bits = match value.to_bits_or_ptr(op.layout.size, self.ecx) { Err(ptr) => { @@ -761,8 +780,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> let mplace = op.assert_mem_place(self.ecx); // strings are never immediate try_validation!( self.ecx.read_str(mplace), - "uninitialized or non-UTF-8 data in str", - self.path + self.path, + err_ub!(InvalidStr(..)) => { "uninitialized or non-UTF-8 data in str" }, ); } ty::Array(tys, ..) | ty::Slice(tys) From aa2eaca443e47c6e10c4e15f9aee5c995dda5272 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 00:13:41 +0200 Subject: [PATCH 16/23] add test for insufficiently aligned vtable --- src/test/ui/consts/const-eval/ub-wide-ptr.rs | 2 ++ .../ui/consts/const-eval/ub-wide-ptr.stderr | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.rs b/src/test/ui/consts/const-eval/ub-wide-ptr.rs index 0200bfe9f08f8..50d254c1c080c 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.rs +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.rs @@ -104,6 +104,8 @@ const TRAIT_OBJ_SHORT_VTABLE_2: &dyn Trait = unsafe { mem::transmute((&92u8, &3u // bad trait object const TRAIT_OBJ_INT_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, 4usize)) }; //~^ ERROR it is undefined behavior to use this value +const TRAIT_OBJ_UNALIGNED_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, &[0u8; 128])) }; +//~^ ERROR it is undefined behavior to use this value // bad data *inside* the trait object const TRAIT_OBJ_CONTENT_INVALID: &dyn Trait = unsafe { mem::transmute::<_, &bool>(&3u8) }; diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr index e56459a7bdeb5..bed124f6c087f 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr @@ -159,7 +159,15 @@ LL | const TRAIT_OBJ_INT_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, 4u = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-wide-ptr.rs:109:1 + --> $DIR/ub-wide-ptr.rs:107:1 + | +LL | const TRAIT_OBJ_UNALIGNED_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, &[0u8; 128])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-wide-ptr.rs:111:1 | LL | const TRAIT_OBJ_CONTENT_INVALID: &dyn Trait = unsafe { mem::transmute::<_, &bool>(&3u8) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x03 at .., but expected a boolean @@ -167,7 +175,7 @@ LL | const TRAIT_OBJ_CONTENT_INVALID: &dyn Trait = unsafe { mem::transmute::<_, = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-wide-ptr.rs:113:1 + --> $DIR/ub-wide-ptr.rs:115:1 | LL | const RAW_TRAIT_OBJ_VTABLE_NULL: *const dyn Trait = unsafe { mem::transmute((&92u8, 0usize)) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable @@ -175,7 +183,7 @@ LL | const RAW_TRAIT_OBJ_VTABLE_NULL: *const dyn Trait = unsafe { mem::transmute = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-wide-ptr.rs:115:1 + --> $DIR/ub-wide-ptr.rs:117:1 | LL | const RAW_TRAIT_OBJ_VTABLE_INVALID: *const dyn Trait = unsafe { mem::transmute((&92u8, &3u64)) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable @@ -183,17 +191,17 @@ LL | const RAW_TRAIT_OBJ_VTABLE_INVALID: *const dyn Trait = unsafe { mem::transm = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: could not evaluate static initializer - --> $DIR/ub-wide-ptr.rs:121:5 + --> $DIR/ub-wide-ptr.rs:123:5 | LL | mem::transmute::<_, &dyn Trait>((&92u8, 0usize)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ inbounds test failed: 0x0 is not a valid pointer error[E0080]: could not evaluate static initializer - --> $DIR/ub-wide-ptr.rs:125:5 + --> $DIR/ub-wide-ptr.rs:127:5 | LL | mem::transmute::<_, &dyn Trait>((&92u8, &3u64)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: pointer must be in-bounds at offset N, but is outside bounds of allocN which has size N -error: aborting due to 24 previous errors +error: aborting due to 25 previous errors For more information about this error, try `rustc --explain E0080`. From 837c16ba2ac8931b821f2fcd0f8549a46c88b342 Mon Sep 17 00:00:00 2001 From: mark Date: Tue, 5 May 2020 21:46:12 -0500 Subject: [PATCH 17/23] comment out rustc-dev-guide in NIGHTLY_TOOLS --- src/bootstrap/toolstate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/toolstate.rs b/src/bootstrap/toolstate.rs index e6560771c0ee7..490c74e4f037b 100644 --- a/src/bootstrap/toolstate.rs +++ b/src/bootstrap/toolstate.rs @@ -89,7 +89,7 @@ static STABLE_TOOLS: &[(&str, &str)] = &[ static NIGHTLY_TOOLS: &[(&str, &str)] = &[ ("miri", "src/tools/miri"), ("embedded-book", "src/doc/embedded-book"), - ("rustc-dev-guide", "src/doc/rustc-dev-guide"), + // ("rustc-dev-guide", "src/doc/rustc-dev-guide"), ]; fn print_error(tool: &str, submodule: &str) { From 19bd72e62390740afd8d6afd5fd57c04e8469b4f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 09:22:52 +0200 Subject: [PATCH 18/23] convert remaining try_validation to new macro --- src/librustc_mir/interpret/validity.rs | 52 +++++++++---------- src/test/ui/consts/const-eval/ub-wide-ptr.rs | 6 +++ .../ui/consts/const-eval/ub-wide-ptr.stderr | 34 ++++++++++-- 3 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 620333f430429..2e1138d1d0708 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -37,25 +37,16 @@ macro_rules! throw_validation_failure { }}; } -/// Returns a validation failure for any Err value of $e. -// FIXME: Replace all usages of try_validation_catchall! with try_validation!. -macro_rules! try_validation_catchall { - ($e:expr, $what:expr, $where:expr $(, $expected:expr )?) => {{ - try_validation!($e, $where, - _ => { "{}", $what } $( expected { "{}", $expected } )?, - ) - }}; -} -/// Like try_validation, but will throw a validation error if any of the patterns in $p are -/// matched. Other errors are passed back to the caller, unchanged. This lets you use the patterns -/// as a kind of validation blacklist: +/// If $e throws an error matching the pattern, throw a validation failure. +/// Other errors are passed back to the caller, unchanged -- and if they reach the root of +/// the visitor, we make sure only validation errors and `InvalidProgram` errors are left. +/// This lets you use the patterns as a kind of validation whitelist, asserting which errors +/// can possibly happen: /// /// ``` /// let v = try_validation!(some_fn(), some_path, { /// Foo | Bar | Baz => { "some failure" }, /// }); -/// // Failures that match $p are thrown up as validation errors, but other errors are passed back -/// // unchanged. /// ``` /// /// An additional expected parameter can also be added to the failure message: @@ -316,19 +307,21 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' err_ub!(PointerOutOfBounds { .. }) | err_ub!(AlignmentCheckFailed { .. }) | err_ub!(DanglingIntPointer(..)) | - err_unsup!(ReadBytesAsPointer) => { - "dangling or unaligned vtable pointer in wide pointer or too small vtable" - }, + err_unsup!(ReadBytesAsPointer) => + { "dangling or unaligned vtable pointer in wide pointer or too small vtable" }, ); - try_validation_catchall!( + try_validation!( self.ecx.read_drop_type_from_vtable(vtable), - "invalid drop fn in vtable", - self.path + self.path, + err_ub!(DanglingIntPointer(..)) | + err_ub!(InvalidFunctionPointer(..)) | + err_unsup!(ReadBytesAsPointer) => + { "invalid drop fn in vtable" }, ); - try_validation_catchall!( + try_validation!( self.ecx.read_size_and_align_from_vtable(vtable), - "invalid size or align in vtable", - self.path + self.path, + err_unsup!(ReadPointerAsBytes) => { "invalid size or align in vtable" }, ); // FIXME: More checks for the vtable. } @@ -558,11 +551,13 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' } ty::FnPtr(_sig) => { let value = self.ecx.read_scalar(value)?; - let _fn = try_validation_catchall!( + let _fn = try_validation!( value.not_undef().and_then(|ptr| self.ecx.memory.get_fn(ptr)), - value, self.path, - "a function pointer" + err_ub!(DanglingIntPointer(..)) | + err_ub!(InvalidFunctionPointer(..)) | + err_unsup!(ReadBytesAsPointer) => + { "{}", value } expected { "a function pointer" }, ); // FIXME: Check if the signature matches Ok(true) @@ -895,7 +890,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // validate and each caller will know best what to do with them. Err(err) if matches!(err.kind, InterpError::InvalidProgram(_)) => Err(err), // Avoid other errors as those do not show *where* in the value the issue lies. - Err(err) => bug!("Unexpected error during validation: {}", err), + Err(err) => { + err.print_backtrace(); + bug!("Unexpected error during validation: {}", err); + } } } diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.rs b/src/test/ui/consts/const-eval/ub-wide-ptr.rs index 50d254c1c080c..29ac32fcf2204 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.rs +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.rs @@ -106,6 +106,12 @@ const TRAIT_OBJ_INT_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, 4usize) //~^ ERROR it is undefined behavior to use this value const TRAIT_OBJ_UNALIGNED_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, &[0u8; 128])) }; //~^ ERROR it is undefined behavior to use this value +const TRAIT_OBJ_BAD_DROP_FN_NULL: &dyn Trait = unsafe { mem::transmute((&92u8, &[0usize; 8])) }; +//~^ ERROR it is undefined behavior to use this value +const TRAIT_OBJ_BAD_DROP_FN_INT: &dyn Trait = unsafe { mem::transmute((&92u8, &[1usize; 8])) }; +//~^ ERROR it is undefined behavior to use this value +const TRAIT_OBJ_BAD_DROP_FN_NOT_FN_PTR: &dyn Trait = unsafe { mem::transmute((&92u8, &[&42u8; 8])) }; +//~^ ERROR it is undefined behavior to use this value // bad data *inside* the trait object const TRAIT_OBJ_CONTENT_INVALID: &dyn Trait = unsafe { mem::transmute::<_, &bool>(&3u8) }; diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr index bed124f6c087f..bfeaac88ab190 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr @@ -166,16 +166,40 @@ LL | const TRAIT_OBJ_UNALIGNED_VTABLE: &dyn Trait = unsafe { mem::transmute((&92 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-wide-ptr.rs:109:1 + | +LL | const TRAIT_OBJ_BAD_DROP_FN_NULL: &dyn Trait = unsafe { mem::transmute((&92u8, &[0usize; 8])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. + error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:111:1 | +LL | const TRAIT_OBJ_BAD_DROP_FN_INT: &dyn Trait = unsafe { mem::transmute((&92u8, &[1usize; 8])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-wide-ptr.rs:113:1 + | +LL | const TRAIT_OBJ_BAD_DROP_FN_NOT_FN_PTR: &dyn Trait = unsafe { mem::transmute((&92u8, &[&42u8; 8])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-wide-ptr.rs:117:1 + | LL | const TRAIT_OBJ_CONTENT_INVALID: &dyn Trait = unsafe { mem::transmute::<_, &bool>(&3u8) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x03 at .., but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-wide-ptr.rs:115:1 + --> $DIR/ub-wide-ptr.rs:121:1 | LL | const RAW_TRAIT_OBJ_VTABLE_NULL: *const dyn Trait = unsafe { mem::transmute((&92u8, 0usize)) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable @@ -183,7 +207,7 @@ LL | const RAW_TRAIT_OBJ_VTABLE_NULL: *const dyn Trait = unsafe { mem::transmute = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-wide-ptr.rs:117:1 + --> $DIR/ub-wide-ptr.rs:123:1 | LL | const RAW_TRAIT_OBJ_VTABLE_INVALID: *const dyn Trait = unsafe { mem::transmute((&92u8, &3u64)) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable @@ -191,17 +215,17 @@ LL | const RAW_TRAIT_OBJ_VTABLE_INVALID: *const dyn Trait = unsafe { mem::transm = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. error[E0080]: could not evaluate static initializer - --> $DIR/ub-wide-ptr.rs:123:5 + --> $DIR/ub-wide-ptr.rs:129:5 | LL | mem::transmute::<_, &dyn Trait>((&92u8, 0usize)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ inbounds test failed: 0x0 is not a valid pointer error[E0080]: could not evaluate static initializer - --> $DIR/ub-wide-ptr.rs:127:5 + --> $DIR/ub-wide-ptr.rs:133:5 | LL | mem::transmute::<_, &dyn Trait>((&92u8, &3u64)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: pointer must be in-bounds at offset N, but is outside bounds of allocN which has size N -error: aborting due to 25 previous errors +error: aborting due to 28 previous errors For more information about this error, try `rustc --explain E0080`. From a64d643956c3cfc90632951ae7ec2b3b84d04c1f Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 6 May 2020 09:59:47 +0200 Subject: [PATCH 19/23] Update librustdoc ID tests --- src/librustdoc/html/markdown/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustdoc/html/markdown/tests.rs b/src/librustdoc/html/markdown/tests.rs index c871587a0a919..bf0451a1d9d65 100644 --- a/src/librustdoc/html/markdown/tests.rs +++ b/src/librustdoc/html/markdown/tests.rs @@ -29,8 +29,8 @@ fn test_unique_id() { "examples-2", "method.into_iter-1", "foo-1", - "main", - "search", + "main-1", + "search-1", "methods", "examples-3", "method.into_iter-2", @@ -191,8 +191,8 @@ fn test_header_ids_multiple_blocks() { t( &mut map, "# Main", - "

\ - Main

", + "

\ + Main

", ); t( &mut map, From 441419a9237ca3ffeb8bcaca9f3c406991c2fa75 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 11:31:05 +0200 Subject: [PATCH 20/23] properly catch invalid-drop-fn errors --- src/librustc_middle/mir/interpret/error.rs | 20 ++++++++++++-------- src/librustc_mir/interpret/operand.rs | 3 +-- src/librustc_mir/interpret/traits.rs | 6 ++---- src/librustc_mir/interpret/validity.rs | 1 + 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/librustc_middle/mir/interpret/error.rs b/src/librustc_middle/mir/interpret/error.rs index 1c4309dbe3a5b..ffe71eb3a0924 100644 --- a/src/librustc_middle/mir/interpret/error.rs +++ b/src/librustc_middle/mir/interpret/error.rs @@ -3,8 +3,7 @@ use super::{AllocId, Pointer, RawConst, ScalarMaybeUndef}; use crate::mir::interpret::ConstValue; use crate::ty::layout::LayoutError; use crate::ty::query::TyCtxtAt; -use crate::ty::tls; -use crate::ty::{self, layout, Ty}; +use crate::ty::{self, layout, tls, FnSig, Ty}; use rustc_data_structures::sync::Lock; use rustc_errors::{struct_span_err, DiagnosticBuilder, ErrorReported}; @@ -329,7 +328,7 @@ impl fmt::Display for CheckInAllocMsg { } /// Error information for when the program caused Undefined Behavior. -pub enum UndefinedBehaviorInfo { +pub enum UndefinedBehaviorInfo<'tcx> { /// Free-form case. Only for errors that are never caught! Ub(String), /// Unreachable code was executed. @@ -347,6 +346,8 @@ pub enum UndefinedBehaviorInfo { PointerArithOverflow, /// Invalid metadata in a wide pointer (using `str` to avoid allocations). InvalidMeta(&'static str), + /// Invalid drop function in vtable. + InvalidDropFn(FnSig<'tcx>), /// Reading a C string that does not end within its allocation. UnterminatedCString(Pointer), /// Dereferencing a dangling pointer after it got freed. @@ -393,7 +394,7 @@ pub enum UndefinedBehaviorInfo { }, } -impl fmt::Display for UndefinedBehaviorInfo { +impl fmt::Display for UndefinedBehaviorInfo<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use UndefinedBehaviorInfo::*; match self { @@ -406,6 +407,11 @@ impl fmt::Display for UndefinedBehaviorInfo { RemainderByZero => write!(f, "calculating the remainder with a divisor of zero"), PointerArithOverflow => write!(f, "overflowing in-bounds pointer arithmetic"), InvalidMeta(msg) => write!(f, "invalid metadata in wide pointer: {}", msg), + InvalidDropFn(sig) => write!( + f, + "invalid drop function signature: got {}, expected exactly one argument which must be a pointer type", + sig + ), UnterminatedCString(p) => write!( f, "reading a null-terminated string starting at {} with no null found before end of allocation", @@ -448,9 +454,7 @@ impl fmt::Display for UndefinedBehaviorInfo { InvalidFunctionPointer(p) => { write!(f, "using {} as function pointer but it does not point to a function", p) } - InvalidStr(err) => { - write!(f, "this string is not valid UTF-8: {}", err) - } + InvalidStr(err) => write!(f, "this string is not valid UTF-8: {}", err), InvalidUndefBytes(Some(p)) => write!( f, "reading uninitialized memory at {}, but this operation requires initialized memory", @@ -554,7 +558,7 @@ impl dyn MachineStopType { pub enum InterpError<'tcx> { /// The program caused undefined behavior. - UndefinedBehavior(UndefinedBehaviorInfo), + UndefinedBehavior(UndefinedBehaviorInfo<'tcx>), /// The program did something the interpreter does not support (some of these *might* be UB /// but the interpreter is not sure). Unsupported(UnsupportedOpInfo), diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index e8ef330c76cf0..db836d88dd05e 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -327,8 +327,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { pub fn read_str(&self, mplace: MPlaceTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, &str> { let len = mplace.len(self)?; let bytes = self.memory.read_bytes(mplace.ptr, Size::from_bytes(len))?; - let str = ::std::str::from_utf8(bytes) - .map_err(|err| err_ub!(InvalidStr(err)))?; + let str = ::std::str::from_utf8(bytes).map_err(|err| err_ub!(InvalidStr(err)))?; Ok(str) } diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index 7edd787c986bd..23673487d2b23 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -147,13 +147,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // The drop function takes `*mut T` where `T` is the type being dropped, so get that. let args = fn_sig.inputs(); if args.len() != 1 { - throw_ub_format!("drop fn should have 1 argument, but signature is {:?}", fn_sig); + throw_ub!(InvalidDropFn(fn_sig)); } let ty = args[0] .builtin_deref(true) - .ok_or_else(|| { - err_ub_format!("drop fn argument type {} is not a pointer type", args[0]) - })? + .ok_or_else(|| err_ub!(InvalidDropFn(fn_sig)))? .ty; Ok((drop_instance, ty)) } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 2e1138d1d0708..2c62070b1b1d9 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -313,6 +313,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' try_validation!( self.ecx.read_drop_type_from_vtable(vtable), self.path, + err_ub!(InvalidDropFn(..)) | err_ub!(DanglingIntPointer(..)) | err_ub!(InvalidFunctionPointer(..)) | err_unsup!(ReadBytesAsPointer) => From 7c4422654a3d207709a766813f2e4ca190776a2a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 11:42:18 +0200 Subject: [PATCH 21/23] convert throw_validation_failure macro to same syntax as try_validation --- src/librustc_mir/interpret/validity.rs | 107 ++++++++++++------------- 1 file changed, 51 insertions(+), 56 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 2c62070b1b1d9..143447ee566bd 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -25,14 +25,19 @@ use super::{ }; macro_rules! throw_validation_failure { - ($what:expr, $where:expr $(, $expected:expr )?) => {{ - let mut msg = format!("encountered {}", $what); + ($where:expr, { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )?) => {{ + let mut msg = String::new(); + msg.push_str("encountered "); + write!(&mut msg, $($what_fmt),+).unwrap(); let where_ = &$where; if !where_.is_empty() { msg.push_str(" at "); write_path(&mut msg, where_); } - $( write!(&mut msg, ", but expected {}", $expected).unwrap(); )? + $( + msg.push_str(", but expected "); + write!(&mut msg, $($expected_fmt),+).unwrap(); + )? throw_ub!(ValidationFailure(msg)) }}; } @@ -76,9 +81,8 @@ macro_rules! try_validation { // allocation here as this can only slow down builds that fail anyway. $( Err(InterpErrorInfo { kind: $p, .. }) )|+ => throw_validation_failure!( - format_args!($( $what_fmt ),+), - $where - $(, format_args!($( $expected_fmt ),+))? + $where, + { $( $what_fmt ),+ } $( expected { $( $expected_fmt ),+ } )? ), #[allow(unreachable_patterns)] Err(e) => Err::(e)?, @@ -366,9 +370,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' let size_and_align = match self.ecx.size_and_align_of(place.meta, place.layout) { Ok(res) => res, Err(err) => match err.kind { - err_ub!(InvalidMeta(msg)) => throw_validation_failure!( - format_args!("invalid {} metadata: {}", kind, msg), - self.path + err_ub!(InvalidMeta(msg)) => throw_validation_failure!(self.path, + { "invalid {} metadata: {}", kind, msg } ), _ => bug!("unexpected error during ptr size_and_align_of: {}", err), }, @@ -393,37 +396,32 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' ); match err.kind { err_ub!(DanglingIntPointer(0, _)) => { - throw_validation_failure!(format_args!("a NULL {}", kind), self.path) + throw_validation_failure!(self.path, + { "a NULL {}", kind } + ) } - err_ub!(DanglingIntPointer(i, _)) => throw_validation_failure!( - format_args!("a {} to unallocated address {}", kind, i), - self.path + err_ub!(DanglingIntPointer(i, _)) => throw_validation_failure!(self.path, + { "a {} to unallocated address {}", kind, i } ), err_ub!(AlignmentCheckFailed { required, has }) => throw_validation_failure!( - format_args!( + self.path, + { "an unaligned {} (required {} byte alignment but found {})", kind, required.bytes(), has.bytes() - ), - self.path + } ), - err_unsup!(ReadBytesAsPointer) => throw_validation_failure!( - format_args!("a dangling {} (created from integer)", kind), - self.path + err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(self.path, + { "a dangling {} (created from integer)", kind } ), - err_ub!(PointerOutOfBounds { .. }) => throw_validation_failure!( - format_args!( - "a dangling {} (going beyond the bounds of its allocation)", - kind - ), - self.path + err_ub!(PointerOutOfBounds { .. }) => throw_validation_failure!(self.path, + { "a dangling {} (going beyond the bounds of its allocation)", kind } ), // This cannot happen during const-eval (because interning already detects // dangling pointers), but it can happen in Miri. - err_ub!(PointerUseAfterFree(_)) => throw_validation_failure!( - format_args!("a dangling {} (use-after-free)", kind), - self.path + err_ub!(PointerUseAfterFree(_)) => throw_validation_failure!(self.path, + { "a dangling {} (use-after-free)", kind } ), _ => bug!("Unexpected error during ptr inbounds test: {}", err), } @@ -443,9 +441,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // We also need to do it here instead of going on to avoid running // into the `before_access_global` check during validation. if !self.may_ref_to_static && self.ecx.tcx.is_static(did) { - throw_validation_failure!( - format_args!("a {} pointing to a static variable", kind), - self.path + throw_validation_failure!(self.path, + { "a {} pointing to a static variable", kind } ); } // `extern static` cannot be validated as they have no body. @@ -516,10 +513,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous let is_bits = value.not_undef().map_or(false, |v| v.is_bits()); if !is_bits { - throw_validation_failure!( - value, - self.path, - "initialized plain (non-pointer) bytes" + throw_validation_failure!(self.path, + { "{}", value } expected { "initialized plain (non-pointer) bytes" } ) } } else { @@ -563,7 +558,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' // FIXME: Check if the signature matches Ok(true) } - ty::Never => throw_validation_failure!("a value of the never type `!`", self.path), + ty::Never => throw_validation_failure!(self.path, { "a value of the never type `!`" }), ty::Foreign(..) | ty::FnDef(..) => { // Nothing to check. Ok(true) @@ -622,26 +617,24 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' if lo == 1 && hi == max_hi { // Only NULL is the niche. So make sure the ptr is NOT NULL. if self.ecx.memory.ptr_may_be_null(ptr) { - throw_validation_failure!( - "a potentially NULL pointer", - self.path, - format_args!( + throw_validation_failure!(self.path, + { "a potentially NULL pointer" } + expected { "something that cannot possibly fail to be {}", wrapping_range_format(valid_range, max_hi) - ) + } ) } return Ok(()); } else { // Conservatively, we reject, because the pointer *could* have a bad // value. - throw_validation_failure!( - "a pointer", - self.path, - format_args!( + throw_validation_failure!(self.path, + { "a pointer" } + expected { "something that cannot possibly fail to be {}", wrapping_range_format(valid_range, max_hi) - ) + } ) } } @@ -651,10 +644,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' if wrapping_range_contains(&valid_range, bits) { Ok(()) } else { - throw_validation_failure!( - bits, - self.path, - format_args!("something {}", wrapping_range_format(valid_range, max_hi)) + throw_validation_failure!(self.path, + { "{}", bits } + expected { "something {}", wrapping_range_format(valid_range, max_hi) } ) } } @@ -722,10 +714,14 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> Ok(()) => {} Err(err) => match err.kind { err_ub!(InvalidDiscriminant(val)) => { - throw_validation_failure!(val, self.path, "a valid enum discriminant") + throw_validation_failure!(self.path, + { "{}", val } expected { "a valid enum discriminant" } + ) } err_unsup!(ReadPointerAsBytes) => { - throw_validation_failure!("a pointer", self.path, "plain (non-pointer) bytes") + throw_validation_failure!(self.path, + { "a pointer" } expected { "plain (non-pointer) bytes" } + ) } // Propagate upwards (that will also check for unexpected errors). _ => return Err(err), @@ -744,9 +740,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // MyNewtype and then the scalar in there). match op.layout.abi { Abi::Uninhabited => { - throw_validation_failure!( - format_args!("a value of uninhabited type {:?}", op.layout.ty), - self.path + throw_validation_failure!(self.path, + { "a value of uninhabited type {:?}", op.layout.ty } ); } Abi::Scalar(ref scalar_layout) => { @@ -840,7 +835,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> .unwrap(); self.path.push(PathElem::ArrayElem(i)); - throw_validation_failure!("uninitialized bytes", self.path) + throw_validation_failure!(self.path, { "uninitialized bytes" }) } // Propagate upwards (that will also check for unexpected errors). _ => return Err(err), From 8998c7afe8b9d8fea20b9b8de06cf1cf0b436e19 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 13:26:24 +0200 Subject: [PATCH 22/23] try_validation: handle multi-branching, and use macro for most remaining manual throw_validation_failure sites --- src/librustc_mir/interpret/traits.rs | 5 +- src/librustc_mir/interpret/validity.rs | 120 ++++++++++--------------- 2 files changed, 47 insertions(+), 78 deletions(-) diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index 23673487d2b23..b9f9d37df7645 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -149,10 +149,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { if args.len() != 1 { throw_ub!(InvalidDropFn(fn_sig)); } - let ty = args[0] - .builtin_deref(true) - .ok_or_else(|| err_ub!(InvalidDropFn(fn_sig)))? - .ty; + let ty = args[0].builtin_deref(true).ok_or_else(|| err_ub!(InvalidDropFn(fn_sig)))?.ty; Ok((drop_instance, ty)) } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 143447ee566bd..a5f012c8f4efb 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -73,17 +73,18 @@ macro_rules! throw_validation_failure { /// macro_rules! try_validation { ($e:expr, $where:expr, - $( $p:pat )|+ => { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )? $( , )? + $( $( $p:pat )|+ => { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )? ),+ $(,)? ) => {{ match $e { Ok(x) => x, // We catch the error and turn it into a validation failure. We are okay with // allocation here as this can only slow down builds that fail anyway. - $( Err(InterpErrorInfo { kind: $p, .. }) )|+ => + $( $( Err(InterpErrorInfo { kind: $p, .. }) )|+ => throw_validation_failure!( $where, { $( $what_fmt ),+ } $( expected { $( $expected_fmt ),+ } )? ), + )+ #[allow(unreachable_patterns)] Err(e) => Err::(e)?, } @@ -367,66 +368,45 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' self.check_wide_ptr_meta(place.meta, place.layout)?; } // Make sure this is dereferenceable and all. - let size_and_align = match self.ecx.size_and_align_of(place.meta, place.layout) { - Ok(res) => res, - Err(err) => match err.kind { - err_ub!(InvalidMeta(msg)) => throw_validation_failure!(self.path, - { "invalid {} metadata: {}", kind, msg } - ), - _ => bug!("unexpected error during ptr size_and_align_of: {}", err), - }, - }; + let size_and_align = try_validation!( + self.ecx.size_and_align_of(place.meta, place.layout), + self.path, + err_ub!(InvalidMeta(msg)) => { "invalid {} metadata: {}", kind, msg }, + ); let (size, align) = size_and_align // for the purpose of validity, consider foreign types to have // alignment and size determined by the layout (size will be 0, // alignment should take attributes into account). .unwrap_or_else(|| (place.layout.size, place.layout.align.abi)); // Direct call to `check_ptr_access_align` checks alignment even on CTFE machines. - let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align( - place.ptr, - size, - Some(align), - CheckInAllocMsg::InboundsTest, - ) { - Ok(ptr) => ptr, - Err(err) => { - info!( - "{:?} did not pass access check for size {:?}, align {:?}", - place.ptr, size, align - ); - match err.kind { - err_ub!(DanglingIntPointer(0, _)) => { - throw_validation_failure!(self.path, - { "a NULL {}", kind } - ) - } - err_ub!(DanglingIntPointer(i, _)) => throw_validation_failure!(self.path, - { "a {} to unallocated address {}", kind, i } - ), - err_ub!(AlignmentCheckFailed { required, has }) => throw_validation_failure!( - self.path, - { - "an unaligned {} (required {} byte alignment but found {})", - kind, - required.bytes(), - has.bytes() - } - ), - err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(self.path, - { "a dangling {} (created from integer)", kind } - ), - err_ub!(PointerOutOfBounds { .. }) => throw_validation_failure!(self.path, - { "a dangling {} (going beyond the bounds of its allocation)", kind } - ), - // This cannot happen during const-eval (because interning already detects - // dangling pointers), but it can happen in Miri. - err_ub!(PointerUseAfterFree(_)) => throw_validation_failure!(self.path, - { "a dangling {} (use-after-free)", kind } - ), - _ => bug!("Unexpected error during ptr inbounds test: {}", err), - } - } - }; + let ptr: Option<_> = try_validation!( + self.ecx.memory.check_ptr_access_align( + place.ptr, + size, + Some(align), + CheckInAllocMsg::InboundsTest, + ), + self.path, + err_ub!(DanglingIntPointer(0, _)) => + { "a NULL {}", kind }, + err_ub!(DanglingIntPointer(i, _)) => + { "a {} to unallocated address {}", kind, i }, + err_ub!(AlignmentCheckFailed { required, has }) => + { + "an unaligned {} (required {} byte alignment but found {})", + kind, + required.bytes(), + has.bytes() + }, + err_unsup!(ReadBytesAsPointer) => + { "a dangling {} (created from integer)", kind }, + err_ub!(PointerOutOfBounds { .. }) => + { "a dangling {} (going beyond the bounds of its allocation)", kind }, + // This cannot happen during const-eval (because interning already detects + // dangling pointers), but it can happen in Miri. + err_ub!(PointerUseAfterFree(_)) => + { "a dangling {} (use-after-free)", kind }, + ); // Recursive checking if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts { if let Some(ptr) = ptr { @@ -710,23 +690,14 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> assert!(op.layout.ty.builtin_deref(true).is_none()); // Recursively walk the type. Translate some possible errors to something nicer. - match self.walk_value(op) { - Ok(()) => {} - Err(err) => match err.kind { - err_ub!(InvalidDiscriminant(val)) => { - throw_validation_failure!(self.path, - { "{}", val } expected { "a valid enum discriminant" } - ) - } - err_unsup!(ReadPointerAsBytes) => { - throw_validation_failure!(self.path, - { "a pointer" } expected { "plain (non-pointer) bytes" } - ) - } - // Propagate upwards (that will also check for unexpected errors). - _ => return Err(err), - }, - } + try_validation!( + self.walk_value(op), + self.path, + err_ub!(InvalidDiscriminant(val)) => + { "{}", val } expected { "a valid enum discriminant" }, + err_unsup!(ReadPointerAsBytes) => + { "a pointer" } expected { "plain (non-pointer) bytes" }, + ); // *After* all of this, check the ABI. We need to check the ABI to handle // types like `NonNull` where the `Scalar` info is more restrictive than what @@ -825,7 +796,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> Ok(()) => {} // Some error happened, try to provide a more detailed description. Err(err) => { - // For some errors we might be able to provide extra information + // For some errors we might be able to provide extra information. + // (This custom logic does not fit the `try_validation!` macro.) match err.kind { err_ub!(InvalidUndefBytes(Some(ptr))) => { // Some byte was uninitialized, determine which From 0e2a712743513f9b8e1c7cc4abdd287432534206 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 May 2020 13:46:01 +0200 Subject: [PATCH 23/23] more precise vtable errors --- src/librustc_mir/interpret/validity.rs | 25 +++++++++++-------- .../ui/consts/const-eval/ub-wide-ptr.stderr | 18 ++++++------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index a5f012c8f4efb..9f2e79bbee31e 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -309,11 +309,14 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' CheckInAllocMsg::InboundsTest, ), self.path, - err_ub!(PointerOutOfBounds { .. }) | - err_ub!(AlignmentCheckFailed { .. }) | err_ub!(DanglingIntPointer(..)) | + err_ub!(PointerUseAfterFree(..)) | err_unsup!(ReadBytesAsPointer) => - { "dangling or unaligned vtable pointer in wide pointer or too small vtable" }, + { "dangling vtable pointer in wide pointer" }, + err_ub!(AlignmentCheckFailed { .. }) => + { "unaligned vtable pointer in wide pointer" }, + err_ub!(PointerOutOfBounds { .. }) => + { "too small vtable" }, ); try_validation!( self.ecx.read_drop_type_from_vtable(vtable), @@ -322,7 +325,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' err_ub!(DanglingIntPointer(..)) | err_ub!(InvalidFunctionPointer(..)) | err_unsup!(ReadBytesAsPointer) => - { "invalid drop fn in vtable" }, + { "invalid drop function pointer in vtable" }, ); try_validation!( self.ecx.read_size_and_align_from_vtable(vtable), @@ -387,10 +390,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' CheckInAllocMsg::InboundsTest, ), self.path, - err_ub!(DanglingIntPointer(0, _)) => - { "a NULL {}", kind }, - err_ub!(DanglingIntPointer(i, _)) => - { "a {} to unallocated address {}", kind, i }, err_ub!(AlignmentCheckFailed { required, has }) => { "an unaligned {} (required {} byte alignment but found {})", @@ -398,13 +397,17 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' required.bytes(), has.bytes() }, - err_unsup!(ReadBytesAsPointer) => - { "a dangling {} (created from integer)", kind }, + err_ub!(DanglingIntPointer(0, _)) => + { "a NULL {}", kind }, + err_ub!(DanglingIntPointer(i, _)) => + { "a dangling {} (address {} is unallocated)", kind, i }, err_ub!(PointerOutOfBounds { .. }) => { "a dangling {} (going beyond the bounds of its allocation)", kind }, + err_unsup!(ReadBytesAsPointer) => + { "a dangling {} (created from integer)", kind }, // This cannot happen during const-eval (because interning already detects // dangling pointers), but it can happen in Miri. - err_ub!(PointerUseAfterFree(_)) => + err_ub!(PointerUseAfterFree(..)) => { "a dangling {} (use-after-free)", kind }, ); // Recursive checking diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr index bfeaac88ab190..94ca60596d61b 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr @@ -138,7 +138,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:99:1 | LL | const TRAIT_OBJ_SHORT_VTABLE_1: &dyn Trait = unsafe { mem::transmute((&92u8, &3u8)) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered too small vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -146,7 +146,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:102:1 | LL | const TRAIT_OBJ_SHORT_VTABLE_2: &dyn Trait = unsafe { mem::transmute((&92u8, &3u64)) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered too small vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -154,7 +154,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:105:1 | LL | const TRAIT_OBJ_INT_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, 4usize)) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling vtable pointer in wide pointer | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -162,7 +162,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:107:1 | LL | const TRAIT_OBJ_UNALIGNED_VTABLE: &dyn Trait = unsafe { mem::transmute((&92u8, &[0u8; 128])) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered unaligned vtable pointer in wide pointer | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -170,7 +170,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:109:1 | LL | const TRAIT_OBJ_BAD_DROP_FN_NULL: &dyn Trait = unsafe { mem::transmute((&92u8, &[0usize; 8])) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop function pointer in vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -178,7 +178,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:111:1 | LL | const TRAIT_OBJ_BAD_DROP_FN_INT: &dyn Trait = unsafe { mem::transmute((&92u8, &[1usize; 8])) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop function pointer in vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -186,7 +186,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:113:1 | LL | const TRAIT_OBJ_BAD_DROP_FN_NOT_FN_PTR: &dyn Trait = unsafe { mem::transmute((&92u8, &[&42u8; 8])) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop function pointer in vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -202,7 +202,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:121:1 | LL | const RAW_TRAIT_OBJ_VTABLE_NULL: *const dyn Trait = unsafe { mem::transmute((&92u8, 0usize)) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling vtable pointer in wide pointer | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -210,7 +210,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:123:1 | LL | const RAW_TRAIT_OBJ_VTABLE_INVALID: *const dyn Trait = unsafe { mem::transmute((&92u8, &3u64)) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer in wide pointer or too small vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered too small vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.

\ - Methods\ +