From dd481d5f462d8c7bd5e81438ccc1ef3013343408 Mon Sep 17 00:00:00 2001 From: Taylor Cramer Date: Wed, 17 Jan 2018 17:22:40 -0800 Subject: [PATCH 01/23] fix nested impl trait lifetimes --- src/librustc/middle/resolve_lifetime.rs | 82 ++++++++++++++++++++--- src/test/run-pass/impl-trait/lifetimes.rs | 8 +++ 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/src/librustc/middle/resolve_lifetime.rs b/src/librustc/middle/resolve_lifetime.rs index 59460141166b1..5201df2119dac 100644 --- a/src/librustc/middle/resolve_lifetime.rs +++ b/src/librustc/middle/resolve_lifetime.rs @@ -270,6 +270,19 @@ enum Scope<'a> { /// we should use for an early-bound region? next_early_index: u32, + /// Whether or not this binder would serve as the parent + /// binder for abstract types introduced within. For example: + /// + /// fn foo<'a>() -> impl for<'b> Trait> + /// + /// Here, the abstract types we create for the `impl Trait` + /// and `impl Trait2` references will both have the `foo` item + /// as their parent. When we get to `impl Trait2`, we find + /// that it is nested within the `for<>` binder -- this flag + /// allows us to skip that when looking for the parent binder + /// of the resulting abstract type. + abstract_type_parent: bool, + s: ScopeRef<'a>, }, @@ -498,6 +511,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { let scope = Scope::Binder { lifetimes, next_early_index, + abstract_type_parent: true, s: ROOT_SCOPE, }; self.with(scope, |old_scope, this| { @@ -541,6 +555,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { .collect(), s: self.scope, next_early_index, + abstract_type_parent: false, }; self.with(scope, |old_scope, this| { // a bare fn has no bounds, so everything @@ -614,7 +629,10 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { ref generics, ref bounds, } = *exist_ty; - let mut index = self.next_early_index(); + + // We want to start our early-bound indices at the end of the parent scope, + // not including any parent `impl Trait`s. + let mut index = self.next_early_index_for_abstract_type(); debug!("visit_ty: index = {}", index); let mut elision = None; @@ -638,7 +656,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { s: self.scope }; self.with(scope, |_old_scope, this| { - let scope = Scope::Binder { lifetimes, next_early_index, s: this.scope }; + let scope = Scope::Binder { + lifetimes, + next_early_index, + s: this.scope, + abstract_type_parent: false, + }; this.with(scope, |_old_scope, this| { this.visit_generics(generics); for bound in bounds { @@ -647,7 +670,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { }); }); } else { - let scope = Scope::Binder { lifetimes, next_early_index, s: self.scope }; + let scope = Scope::Binder { + lifetimes, + next_early_index, + s: self.scope, + abstract_type_parent: false, + }; self.with(scope, |_old_scope, this| { this.visit_generics(generics); for bound in bounds { @@ -681,7 +709,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { .collect(); let next_early_index = index + generics.ty_params().count() as u32; - let scope = Scope::Binder { lifetimes, next_early_index, s: self.scope }; + let scope = Scope::Binder { + lifetimes, + next_early_index, + s: self.scope, + abstract_type_parent: true, + }; self.with(scope, |_old_scope, this| { this.visit_generics(generics); for bound in bounds { @@ -721,7 +754,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { .collect(); let next_early_index = index + generics.ty_params().count() as u32; - let scope = Scope::Binder { lifetimes, next_early_index, s: self.scope }; + let scope = Scope::Binder { + lifetimes, + next_early_index, + s: self.scope, + abstract_type_parent: true, + }; self.with(scope, |_old_scope, this| { this.visit_generics(generics); this.visit_ty(ty); @@ -792,6 +830,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { .collect(), s: self.scope, next_early_index, + abstract_type_parent: false, }; let result = self.with(scope, |old_scope, this| { this.check_lifetime_params(old_scope, &bound_generic_params); @@ -853,6 +892,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> { .collect(), s: self.scope, next_early_index, + abstract_type_parent: false, }; self.with(scope, |old_scope, this| { this.check_lifetime_params(old_scope, &trait_ref.bound_generic_params); @@ -1046,6 +1086,7 @@ fn extract_labels(ctxt: &mut LifetimeContext<'_, '_>, body: &hir::Body) { ref lifetimes, s, next_early_index: _, + abstract_type_parent: _, } => { // FIXME (#24278): non-hygienic comparison if let Some(def) = lifetimes.get(&hir::LifetimeName::Name(label)) { @@ -1303,6 +1344,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { lifetimes, next_early_index, s: self.scope, + abstract_type_parent: true, }; self.with(scope, move |old_scope, this| { this.check_lifetime_params(old_scope, &generics.params); @@ -1310,25 +1352,41 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { }); } - /// Returns the next index one would use for an early-bound-region - /// if extending the current scope. - fn next_early_index(&self) -> u32 { + fn next_early_index_helper(&self, only_abstract_type_parent: bool) -> u32 { let mut scope = self.scope; loop { match *scope { Scope::Root => return 0, Scope::Binder { - next_early_index, .. - } => return next_early_index, + next_early_index, + abstract_type_parent, + .. + } if (!only_abstract_type_parent || abstract_type_parent) + => return next_early_index, - Scope::Body { s, .. } + Scope::Binder { s, .. } + | Scope::Body { s, .. } | Scope::Elision { s, .. } | Scope::ObjectLifetimeDefault { s, .. } => scope = s, } } } + /// Returns the next index one would use for an early-bound-region + /// if extending the current scope. + fn next_early_index(&self) -> u32 { + self.next_early_index_helper(true) + } + + /// Returns the next index one would use for an `impl Trait` that + /// is being converted into an `abstract type`. This will be the + /// next early index from the enclosing item, for the most + /// part. See the `abstract_type_parent` field for more info. + fn next_early_index_for_abstract_type(&self) -> u32 { + self.next_early_index_helper(false) + } + fn resolve_lifetime_ref(&mut self, lifetime_ref: &'tcx hir::Lifetime) { debug!("resolve_lifetime_ref(lifetime_ref={:?})", lifetime_ref); // Walk up the scope chain, tracking the number of fn scopes @@ -1353,6 +1411,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { ref lifetimes, s, next_early_index: _, + abstract_type_parent: _, } => { if let Some(&def) = lifetimes.get(&lifetime_ref.name) { break Some(def.shifted(late_depth)); @@ -2102,6 +2161,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { ref lifetimes, s, next_early_index: _, + abstract_type_parent: _, } => { if let Some(&def) = lifetimes.get(&lifetime.name) { let node_id = self.tcx.hir.as_local_node_id(def.id().unwrap()).unwrap(); diff --git a/src/test/run-pass/impl-trait/lifetimes.rs b/src/test/run-pass/impl-trait/lifetimes.rs index 1f2d76f289472..213a46ded8e76 100644 --- a/src/test/run-pass/impl-trait/lifetimes.rs +++ b/src/test/run-pass/impl-trait/lifetimes.rs @@ -50,6 +50,14 @@ fn closure_hr_elided_return() -> impl Fn(&u32) -> &u32 { |x| x } fn closure_pass_through_elided_return(x: impl Fn(&u32) -> &u32) -> impl Fn(&u32) -> &u32 { x } fn closure_pass_through_reference_elided(x: &impl Fn(&u32) -> &u32) -> &impl Fn(&u32) -> &u32 { x } +fn nested_lifetime<'a>(input: &'a str) + -> impl Iterator + 'a> + 'a +{ + input.lines().map(|line| { + line.split_whitespace().map(|cell| cell.parse().unwrap()) + }) +} + fn pass_through_elision(x: &u32) -> impl Into<&u32> { x } fn pass_through_elision_with_fn_ptr(x: &fn(&u32) -> &u32) -> impl Into<&fn(&u32) -> &u32> { x } From 4d92fe2bb07a12368b6bb981d08f12f4a446a4d3 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 8 Feb 2018 16:26:43 -0800 Subject: [PATCH 02/23] Fix span bug. --- src/libsyntax/parse/parser.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index dc3745fc4a3ee..c7de0a75817c5 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2630,8 +2630,7 @@ impl<'a> Parser<'a> { // A tuple index may not have a suffix self.expect_no_suffix(sp, "tuple index", suf); - let dot_span = self.prev_span; - hi = self.span; + let idx_span = self.span; self.bump(); let invalid_msg = "invalid tuple or struct index"; @@ -2646,9 +2645,8 @@ impl<'a> Parser<'a> { n.to_string()); err.emit(); } - let id = respan(dot_span.to(hi), n); - let field = self.mk_tup_field(e, id); - e = self.mk_expr(lo.to(hi), field, ThinVec::new()); + let field = self.mk_tup_field(e, respan(idx_span, n)); + e = self.mk_expr(lo.to(idx_span), field, ThinVec::new()); } None => { let prev_span = self.prev_span; From a003cb7cd7d619a5553c5c99e4ee7ce185a1608c Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 8 Feb 2018 16:26:33 -0800 Subject: [PATCH 03/23] Add test. --- src/test/run-pass/hygiene/issue-47312.rs | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 src/test/run-pass/hygiene/issue-47312.rs diff --git a/src/test/run-pass/hygiene/issue-47312.rs b/src/test/run-pass/hygiene/issue-47312.rs new file mode 100644 index 0000000000000..5e83f3808d8cc --- /dev/null +++ b/src/test/run-pass/hygiene/issue-47312.rs @@ -0,0 +1,30 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// ignore-pretty pretty-printing is unhygienic + +#![feature(decl_macro)] +#![allow(unused)] + +mod foo { + pub macro m($s:tt, $i:tt) { + $s.$i + } +} + +mod bar { + struct S(i32); + fn f() { + let s = S(0); + ::foo::m!(s, 0); + } +} + +fn main() {} From 0bb818cc0b2885b01ce670ce192aac1dbc6db16a Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Mon, 12 Feb 2018 01:39:01 -0800 Subject: [PATCH 04/23] Add Iterator::try_for_each The fallible version of for_each and the stateless version of try_fold. --- src/libcore/iter/iterator.rs | 48 +++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/src/libcore/iter/iterator.rs b/src/libcore/iter/iterator.rs index 296fb8733ba6c..a485e2c82bada 100644 --- a/src/libcore/iter/iterator.rs +++ b/src/libcore/iter/iterator.rs @@ -1366,9 +1366,9 @@ pub trait Iterator { /// /// In particular, try to have this call `try_fold()` on the internal parts /// from which this iterator is composed. If multiple calls are needed, - /// the `?` operator be convenient for chaining the accumulator value along, - /// but beware any invariants that need to be upheld before those early - /// returns. This is a `&mut self` method, so iteration needs to be + /// the `?` operator may be convenient for chaining the accumulator value + /// along, but beware any invariants that need to be upheld before those + /// early returns. This is a `&mut self` method, so iteration needs to be /// resumable after hitting an error here. /// /// # Examples @@ -1414,6 +1414,42 @@ pub trait Iterator { Try::from_ok(accum) } + /// An iterator method that applies a fallible function to each item in the + /// iterator, stopping at the first error and returning that error. + /// + /// This can also be thought of as the fallible form of [`for_each()`] + /// or as the stateless version of [`try_fold()`]. + /// + /// [`for_each()`]: #method.for_each + /// [`try_fold()`]: #method.try_fold + /// + /// # Examples + /// + /// ``` + /// #![feature(iterator_try_fold)] + /// use std::fs::rename; + /// use std::io::{stdout, Write}; + /// use std::path::Path; + /// + /// let data = ["no_tea.txt", "stale_bread.json", "torrential_rain.png"]; + /// + /// let res = data.iter().try_for_each(|x| writeln!(stdout(), "{}", x)); + /// assert!(res.is_ok()); + /// + /// let mut it = data.iter().cloned(); + /// let res = it.try_for_each(|x| rename(x, Path::new(x).with_extension("old"))); + /// assert!(res.is_err()); + /// // It short-circuited, so the remaining items are still in the iterator: + /// assert_eq!(it.next(), Some("stale_bread.json")); + /// ``` + #[inline] + #[unstable(feature = "iterator_try_fold", issue = "45594")] + fn try_for_each(&mut self, mut f: F) -> R where + Self: Sized, F: FnMut(Self::Item) -> R, R: Try + { + self.try_fold((), move |(), x| f(x)) + } + /// An iterator method that applies a function, producing a single, final value. /// /// `fold()` takes two arguments: an initial value, and a closure with two @@ -1528,7 +1564,7 @@ pub trait Iterator { fn all(&mut self, mut f: F) -> bool where Self: Sized, F: FnMut(Self::Item) -> bool { - self.try_fold((), move |(), x| { + self.try_for_each(move |x| { if f(x) { LoopState::Continue(()) } else { LoopState::Break(()) } }) == LoopState::Continue(()) @@ -1577,7 +1613,7 @@ pub trait Iterator { Self: Sized, F: FnMut(Self::Item) -> bool { - self.try_fold((), move |(), x| { + self.try_for_each(move |x| { if f(x) { LoopState::Break(()) } else { LoopState::Continue(()) } }) == LoopState::Break(()) @@ -1631,7 +1667,7 @@ pub trait Iterator { Self: Sized, P: FnMut(&Self::Item) -> bool, { - self.try_fold((), move |(), x| { + self.try_for_each(move |x| { if predicate(&x) { LoopState::Break(x) } else { LoopState::Continue(()) } }).break_value() From bd10ef7b27e7c6ab6e4e68898aa6ccd240fc57f3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 10 Feb 2018 09:54:27 -0500 Subject: [PATCH 05/23] rustc_typeck/check/closure: rustfmt --- src/librustc_typeck/check/closure.rs | 51 +++++++++++++++------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/librustc_typeck/check/closure.rs b/src/librustc_typeck/check/closure.rs index df15f781ae8c9..5e9c283a0c6e8 100644 --- a/src/librustc_typeck/check/closure.rs +++ b/src/librustc_typeck/check/closure.rs @@ -42,8 +42,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ) -> Ty<'tcx> { debug!( "check_expr_closure(expr={:?},expected={:?})", - expr, - expected + expr, expected ); // It's always helpful for inference if we know the kind of @@ -68,8 +67,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ) -> Ty<'tcx> { debug!( "check_closure(opt_kind={:?}, expected_sig={:?})", - opt_kind, - expected_sig + opt_kind, expected_sig ); let expr_def_id = self.tcx.hir.local_def_id(expr.id); @@ -109,19 +107,22 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let closure_type = self.tcx.mk_closure(expr_def_id, substs); if let Some(GeneratorTypes { yield_ty, interior }) = generator_types { - self.demand_eqtype(expr.span, - yield_ty, - substs.generator_yield_ty(expr_def_id, self.tcx)); - self.demand_eqtype(expr.span, - liberated_sig.output(), - substs.generator_return_ty(expr_def_id, self.tcx)); + self.demand_eqtype( + expr.span, + yield_ty, + substs.generator_yield_ty(expr_def_id, self.tcx), + ); + self.demand_eqtype( + expr.span, + liberated_sig.output(), + substs.generator_return_ty(expr_def_id, self.tcx), + ); return self.tcx.mk_generator(expr_def_id, substs, interior); } debug!( "check_closure: expr.id={:?} closure_type={:?}", - expr.id, - closure_type + expr.id, closure_type ); // Tuple up the arguments and insert the resulting function type into @@ -138,20 +139,22 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { debug!( "check_closure: expr_def_id={:?}, sig={:?}, opt_kind={:?}", - expr_def_id, - sig, - opt_kind + expr_def_id, sig, opt_kind ); let sig_fn_ptr_ty = self.tcx.mk_fn_ptr(sig); - self.demand_eqtype(expr.span, - sig_fn_ptr_ty, - substs.closure_sig_ty(expr_def_id, self.tcx)); + self.demand_eqtype( + expr.span, + sig_fn_ptr_ty, + substs.closure_sig_ty(expr_def_id, self.tcx), + ); if let Some(kind) = opt_kind { - self.demand_eqtype(expr.span, - kind.to_ty(self.tcx), - substs.closure_kind_ty(expr_def_id, self.tcx)); + self.demand_eqtype( + expr.span, + kind.to_ty(self.tcx), + substs.closure_kind_ty(expr_def_id, self.tcx), + ); } closure_type @@ -314,8 +317,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let self_ty = self.shallow_resolve(trait_ref.self_ty()); debug!( "self_type_matches_expected_vid(trait_ref={:?}, self_ty={:?})", - trait_ref, - self_ty + trait_ref, self_ty ); match self_ty.sty { ty::TyInfer(ty::TyVar(v)) if expected_vid == v => Some(trait_ref), @@ -564,7 +566,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { body: &hir::Body, bound_sig: ty::PolyFnSig<'tcx>, ) -> ClosureSignatures<'tcx> { - let liberated_sig = self.tcx().liberate_late_bound_regions(expr_def_id, &bound_sig); + let liberated_sig = self.tcx() + .liberate_late_bound_regions(expr_def_id, &bound_sig); let liberated_sig = self.inh.normalize_associated_types_in( body.value.span, body.value.id, From cc05561048f07ae5e99b25bbe5db04eebe0c7cd2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 12 Feb 2018 16:00:15 -0500 Subject: [PATCH 06/23] detect wrong number of args when type-checking a closure Instead of creating inference variables for those argument types, use the trait error-reporting code to give a nicer error. --- src/librustc/traits/error_reporting.rs | 42 +++++- src/librustc/traits/mod.rs | 2 +- src/librustc_typeck/check/closure.rs | 131 +++++++++++++++--- ...ure-arg-count-expected-type-issue-47244.rs | 29 ++++ ...arg-count-expected-type-issue-47244.stderr | 14 ++ 5 files changed, 191 insertions(+), 27 deletions(-) create mode 100644 src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs create mode 100644 src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index a290839425ebe..f3aab4020594b 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -747,7 +747,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { ty::TyTuple(ref tys, _) => tys.iter() .map(|t| match t.sty { ty::TypeVariants::TyTuple(ref tys, _) => ArgKind::Tuple( - span, + Some(span), tys.iter() .map(|ty| ("_".to_owned(), format!("{}", ty.sty))) .collect::>() @@ -815,7 +815,11 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } } - fn get_fn_like_arguments(&self, node: hir::map::Node) -> (Span, Vec) { + /// Given some node representing a fn-like thing in the HIR map, + /// returns a span and `ArgKind` information that describes the + /// arguments it expects. This can be supplied to + /// `report_arg_count_mismatch`. + pub fn get_fn_like_arguments(&self, node: hir::map::Node) -> (Span, Vec) { match node { hir::map::NodeExpr(&hir::Expr { node: hir::ExprClosure(_, ref _decl, id, span, _), @@ -829,7 +833,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { .. } = arg.pat.clone().into_inner() { ArgKind::Tuple( - span, + Some(span), args.iter().map(|pat| { let snippet = self.tcx.sess.codemap() .span_to_snippet(pat.span).unwrap(); @@ -862,7 +866,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { (self.tcx.sess.codemap().def_span(span), decl.inputs.iter() .map(|arg| match arg.clone().into_inner().node { hir::TyTup(ref tys) => ArgKind::Tuple( - arg.span, + Some(arg.span), tys.iter() .map(|_| ("_".to_owned(), "_".to_owned())) .collect::>(), @@ -874,7 +878,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } } - fn report_arg_count_mismatch( + /// Reports an error when the number of arguments needed by a + /// trait match doesn't match the number that the expression + /// provides. + pub fn report_arg_count_mismatch( &self, span: Span, found_span: Option, @@ -1363,13 +1370,34 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } } -enum ArgKind { +/// Summarizes information +pub enum ArgKind { + /// An argument of non-tuple type. Parameters are (name, ty) Arg(String, String), - Tuple(Span, Vec<(String, String)>), + + /// An argument of tuple type. For a "found" argument, the span is + /// the locationo in the source of the pattern. For a "expected" + /// argument, it will be None. The vector is a list of (name, ty) + /// strings for the components of the tuple. + Tuple(Option, Vec<(String, String)>), } impl ArgKind { fn empty() -> ArgKind { ArgKind::Arg("_".to_owned(), "_".to_owned()) } + + /// Creates an `ArgKind` from the expected type of an + /// argument. This has no name (`_`) and no source spans.. + pub fn from_expected_ty(t: Ty<'_>) -> ArgKind { + match t.sty { + ty::TyTuple(ref tys, _) => ArgKind::Tuple( + None, + tys.iter() + .map(|ty| ("_".to_owned(), format!("{}", ty.sty))) + .collect::>() + ), + _ => ArgKind::Arg("_".to_owned(), format!("{}", t.sty)), + } + } } diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 80819a86b7c46..a80e91df03de1 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -49,7 +49,7 @@ pub use self::util::SupertraitDefIds; pub use self::util::transitive_bounds; mod coherence; -mod error_reporting; +pub mod error_reporting; mod fulfill; mod project; mod object_safety; diff --git a/src/librustc_typeck/check/closure.rs b/src/librustc_typeck/check/closure.rs index 5e9c283a0c6e8..794d466ee7cdb 100644 --- a/src/librustc_typeck/check/closure.rs +++ b/src/librustc_typeck/check/closure.rs @@ -17,14 +17,24 @@ use rustc::hir::def_id::DefId; use rustc::infer::{InferOk, InferResult}; use rustc::infer::LateBoundRegionConversionTime; use rustc::infer::type_variable::TypeVariableOrigin; +use rustc::traits::error_reporting::ArgKind; use rustc::ty::{self, ToPolyTraitRef, Ty}; use rustc::ty::subst::Substs; use rustc::ty::TypeFoldable; use std::cmp; use std::iter; use syntax::abi::Abi; +use syntax::codemap::Span; use rustc::hir; +/// What signature do we *expect* the closure to have from context? +#[derive(Debug)] +struct ExpectedSig<'tcx> { + /// Span that gave us this expectation, if we know that. + cause_span: Option, + sig: ty::FnSig<'tcx>, +} + struct ClosureSignatures<'tcx> { bound_sig: ty::PolyFnSig<'tcx>, liberated_sig: ty::FnSig<'tcx>, @@ -63,7 +73,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { decl: &'gcx hir::FnDecl, body: &'gcx hir::Body, gen: Option, - expected_sig: Option>, + expected_sig: Option>, ) -> Ty<'tcx> { debug!( "check_closure(opt_kind={:?}, expected_sig={:?})", @@ -160,10 +170,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { closure_type } + /// Given the expected type, figures out what it can about this closure we + /// are about to type check: fn deduce_expectations_from_expected_type( &self, expected_ty: Ty<'tcx>, - ) -> (Option>, Option) { + ) -> (Option>, Option) { debug!( "deduce_expectations_from_expected_type(expected_ty={:?})", expected_ty @@ -175,7 +187,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { .projection_bounds() .filter_map(|pb| { let pb = pb.with_self_ty(self.tcx, self.tcx.types.err); - self.deduce_sig_from_projection(&pb) + self.deduce_sig_from_projection(None, &pb) }) .next(); let kind = object_type @@ -184,7 +196,13 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { (sig, kind) } ty::TyInfer(ty::TyVar(vid)) => self.deduce_expectations_from_obligations(vid), - ty::TyFnPtr(sig) => (Some(sig.skip_binder().clone()), Some(ty::ClosureKind::Fn)), + ty::TyFnPtr(sig) => { + let expected_sig = ExpectedSig { + cause_span: None, + sig: sig.skip_binder().clone(), + }; + (Some(expected_sig), Some(ty::ClosureKind::Fn)) + } _ => (None, None), } } @@ -192,7 +210,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { fn deduce_expectations_from_obligations( &self, expected_vid: ty::TyVid, - ) -> (Option>, Option) { + ) -> (Option>, Option) { let fulfillment_cx = self.fulfillment_cx.borrow(); // Here `expected_ty` is known to be a type inference variable. @@ -212,7 +230,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ty::Predicate::Projection(ref proj_predicate) => { let trait_ref = proj_predicate.to_poly_trait_ref(self.tcx); self.self_type_matches_expected_vid(trait_ref, expected_vid) - .and_then(|_| self.deduce_sig_from_projection(proj_predicate)) + .and_then(|_| { + self.deduce_sig_from_projection( + Some(obligation.cause.span), + proj_predicate, + ) + }) } _ => None, } @@ -262,10 +285,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { /// Given a projection like "::Result == Y", we can deduce /// everything we need to know about a closure. + /// + /// The `cause_span` should be the span that caused us to + /// have this expected signature, or `None` if we can't readily + /// know that. fn deduce_sig_from_projection( &self, + cause_span: Option, projection: &ty::PolyProjectionPredicate<'tcx>, - ) -> Option> { + ) -> Option> { let tcx = self.tcx; debug!("deduce_sig_from_projection({:?})", projection); @@ -297,16 +325,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ret_param_ty ); - let fn_sig = self.tcx.mk_fn_sig( + let sig = self.tcx.mk_fn_sig( input_tys.cloned(), ret_param_ty, false, hir::Unsafety::Normal, Abi::Rust, ); - debug!("deduce_sig_from_projection: fn_sig {:?}", fn_sig); + debug!("deduce_sig_from_projection: sig {:?}", sig); - Some(fn_sig) + Some(ExpectedSig { cause_span, sig }) } fn self_type_matches_expected_vid( @@ -330,7 +358,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr_def_id: DefId, decl: &hir::FnDecl, body: &hir::Body, - expected_sig: Option>, + expected_sig: Option>, ) -> ClosureSignatures<'tcx> { if let Some(e) = expected_sig { self.sig_of_closure_with_expectation(expr_def_id, decl, body, e) @@ -406,7 +434,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr_def_id: DefId, decl: &hir::FnDecl, body: &hir::Body, - expected_sig: ty::FnSig<'tcx>, + expected_sig: ExpectedSig<'tcx>, ) -> ClosureSignatures<'tcx> { debug!( "sig_of_closure_with_expectation(expected_sig={:?})", @@ -416,20 +444,24 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // Watch out for some surprises and just ignore the // expectation if things don't see to match up with what we // expect. - if expected_sig.variadic != decl.variadic { - return self.sig_of_closure_no_expectation(expr_def_id, decl, body); - } else if expected_sig.inputs_and_output.len() != decl.inputs.len() + 1 { - // we could probably handle this case more gracefully + if expected_sig.sig.variadic != decl.variadic { return self.sig_of_closure_no_expectation(expr_def_id, decl, body); + } else if expected_sig.sig.inputs_and_output.len() != decl.inputs.len() + 1 { + return self.sig_of_closure_with_mismatched_number_of_arguments( + expr_def_id, + decl, + body, + expected_sig, + ); } // Create a `PolyFnSig`. Note the oddity that late bound // regions appearing free in `expected_sig` are now bound up // in this binder we are creating. - assert!(!expected_sig.has_regions_escaping_depth(1)); + assert!(!expected_sig.sig.has_regions_escaping_depth(1)); let bound_sig = ty::Binder(self.tcx.mk_fn_sig( - expected_sig.inputs().iter().cloned(), - expected_sig.output(), + expected_sig.sig.inputs().iter().cloned(), + expected_sig.sig.output(), decl.variadic, hir::Unsafety::Normal, Abi::RustCall, @@ -455,6 +487,35 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { closure_sigs } + fn sig_of_closure_with_mismatched_number_of_arguments( + &self, + expr_def_id: DefId, + decl: &hir::FnDecl, + body: &hir::Body, + expected_sig: ExpectedSig<'tcx>, + ) -> ClosureSignatures<'tcx> { + let expr_map_node = self.tcx.hir.get_if_local(expr_def_id).unwrap(); + let expected_args: Vec<_> = expected_sig + .sig + .inputs() + .iter() + .map(|ty| ArgKind::from_expected_ty(ty)) + .collect(); + let (closure_span, found_args) = self.get_fn_like_arguments(expr_map_node); + let expected_span = expected_sig.cause_span.unwrap_or(closure_span); + self.report_arg_count_mismatch( + expected_span, + Some(closure_span), + expected_args, + found_args, + true, + ).emit(); + + let error_sig = self.error_sig_of_closure(decl); + + self.closure_sigs(expr_def_id, body, error_sig) + } + /// Enforce the user's types against the expectation. See /// `sig_of_closure_with_expectation` for details on the overall /// strategy. @@ -560,6 +621,38 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { result } + /// Converts the types that the user supplied, in case that doing + /// so should yield an error, but returns back a signature where + /// all parameters are of type `TyErr`. + fn error_sig_of_closure(&self, decl: &hir::FnDecl) -> ty::PolyFnSig<'tcx> { + let astconv: &AstConv = self; + + let supplied_arguments = decl.inputs.iter().map(|a| { + // Convert the types that the user supplied (if any), but ignore them. + astconv.ast_ty_to_ty(a); + self.tcx.types.err + }); + + match decl.output { + hir::Return(ref output) => { + astconv.ast_ty_to_ty(&output); + } + hir::DefaultReturn(_) => {} + } + + let result = ty::Binder(self.tcx.mk_fn_sig( + supplied_arguments, + self.tcx.types.err, + decl.variadic, + hir::Unsafety::Normal, + Abi::RustCall, + )); + + debug!("supplied_sig_of_closure: result={:?}", result); + + result + } + fn closure_sigs( &self, expr_def_id: DefId, diff --git a/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs new file mode 100644 index 0000000000000..b6463ca067b7f --- /dev/null +++ b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.rs @@ -0,0 +1,29 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Regression test for #47244: in this specific scenario, when the +// expected type indicated 1 argument but the closure takes two, we +// would (early on) create type variables for the type of `b`. If the +// user then attempts to invoke a method on `b`, we would get an error +// saying that the type of `b` must be known, which was not very +// helpful. + +use std::collections::HashMap; +fn main() { + + let m = HashMap::new(); + m.insert( "foo", "bar" ); + + m.iter().map( |_, b| { + //~^ ERROR closure is expected to take a single 2-tuple + + b.to_string() + }); +} diff --git a/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr new file mode 100644 index 0000000000000..34934b8d19598 --- /dev/null +++ b/src/test/ui/mismatched_types/closure-arg-count-expected-type-issue-47244.stderr @@ -0,0 +1,14 @@ +error[E0593]: closure is expected to take a single 2-tuple as argument, but it takes 2 distinct arguments + --> $DIR/closure-arg-count-expected-type-issue-47244.rs:24:14 + | +24 | m.iter().map( |_, b| { + | ^^^ ------ takes 2 distinct arguments + | | + | expected closure that takes a single 2-tuple as argument +help: change the closure to accept a tuple instead of individual arguments + | +24 | m.iter().map( |(_, b)| { + | ^^^^^^^^ + +error: aborting due to previous error + From 1f0e1a043959461afd061be7ff92362492b2c85d Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Thu, 15 Feb 2018 00:06:14 +0100 Subject: [PATCH 07/23] Convert compile-fail/lint-ctypes.rs to ui test --- src/test/{compile-fail => ui}/lint-ctypes.rs | 0 src/test/ui/lint-ctypes.stderr | 128 +++++++++++++++++++ 2 files changed, 128 insertions(+) rename src/test/{compile-fail => ui}/lint-ctypes.rs (100%) create mode 100644 src/test/ui/lint-ctypes.stderr diff --git a/src/test/compile-fail/lint-ctypes.rs b/src/test/ui/lint-ctypes.rs similarity index 100% rename from src/test/compile-fail/lint-ctypes.rs rename to src/test/ui/lint-ctypes.rs diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr new file mode 100644 index 0000000000000..21aadde8ac87c --- /dev/null +++ b/src/test/ui/lint-ctypes.stderr @@ -0,0 +1,128 @@ +error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type + --> $DIR/lint-ctypes.rs:54:28 + | +54 | pub fn ptr_type1(size: *const Foo); //~ ERROR: found struct without + | ^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/lint-ctypes.rs:11:9 + | +11 | #![deny(improper_ctypes)] + | ^^^^^^^^^^^^^^^ + +error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type + --> $DIR/lint-ctypes.rs:55:28 + | +55 | pub fn ptr_type2(size: *const Foo); //~ ERROR: found struct without + | ^^^^^^^^^^ + +error: found Rust slice type in foreign module, consider using a raw pointer instead + --> $DIR/lint-ctypes.rs:56:26 + | +56 | pub fn slice_type(p: &[u32]); //~ ERROR: found Rust slice type + | ^^^^^^ + +error: found Rust type `str` in foreign module; consider using a `*const libc::c_char` + --> $DIR/lint-ctypes.rs:57:24 + | +57 | pub fn str_type(p: &str); //~ ERROR: found Rust type + | ^^^^ + +error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type + --> $DIR/lint-ctypes.rs:58:24 + | +58 | pub fn box_type(p: Box); //~ ERROR found struct without + | ^^^^^^^^ + +error: found Rust type `char` in foreign module, while `u32` or `libc::wchar_t` should be used + --> $DIR/lint-ctypes.rs:59:25 + | +59 | pub fn char_type(p: char); //~ ERROR found Rust type + | ^^^^ + +error: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI + --> $DIR/lint-ctypes.rs:60:25 + | +60 | pub fn i128_type(p: i128); //~ ERROR found Rust type + | ^^^^ + +error: found Rust type `u128` in foreign module, but 128-bit integers don't currently have a known stable ABI + --> $DIR/lint-ctypes.rs:61:25 + | +61 | pub fn u128_type(p: u128); //~ ERROR found Rust type + | ^^^^ + +error: found Rust trait type in foreign module, consider using a raw pointer instead + --> $DIR/lint-ctypes.rs:62:26 + | +62 | pub fn trait_type(p: &Clone); //~ ERROR found Rust trait type + | ^^^^^^ + +error: found Rust tuple type in foreign module; consider using a struct instead + --> $DIR/lint-ctypes.rs:63:26 + | +63 | pub fn tuple_type(p: (i32, i32)); //~ ERROR found Rust tuple type + | ^^^^^^^^^^ + +error: found Rust tuple type in foreign module; consider using a struct instead + --> $DIR/lint-ctypes.rs:64:27 + | +64 | pub fn tuple_type2(p: I32Pair); //~ ERROR found Rust tuple type + | ^^^^^^^ + +error: found zero-size struct in foreign module, consider adding a member to this struct + --> $DIR/lint-ctypes.rs:65:25 + | +65 | pub fn zero_size(p: ZeroSize); //~ ERROR found zero-size struct + | ^^^^^^^^ + +error: found zero-sized type composed only of phantom-data in a foreign-function. + --> $DIR/lint-ctypes.rs:66:33 + | +66 | pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR found zero-sized type + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error: found zero-sized type composed only of phantom-data in a foreign-function. + --> $DIR/lint-ctypes.rs:68:12 + | +68 | -> ::std::marker::PhantomData; //~ ERROR: found zero-sized type + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: found function pointer with Rust calling convention in foreign module; consider using an `extern` function pointer + --> $DIR/lint-ctypes.rs:69:23 + | +69 | pub fn fn_type(p: RustFn); //~ ERROR found function pointer with Rust + | ^^^^^^ + +error: found function pointer with Rust calling convention in foreign module; consider using an `extern` function pointer + --> $DIR/lint-ctypes.rs:70:24 + | +70 | pub fn fn_type2(p: fn()); //~ ERROR found function pointer with Rust + | ^^^^ + +error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type + --> $DIR/lint-ctypes.rs:71:28 + | +71 | pub fn fn_contained(p: RustBadRet); //~ ERROR: found struct without + | ^^^^^^^^^^ + +error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI + --> $DIR/lint-ctypes.rs:72:32 + | +72 | pub fn transparent_i128(p: TransparentI128); //~ ERROR: found Rust type `i128` + | ^^^^^^^^^^^^^^^ + +error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `str` in foreign module; consider using a `*const libc::c_char` + --> $DIR/lint-ctypes.rs:73:31 + | +73 | pub fn transparent_str(p: TransparentStr); //~ ERROR: found Rust type `str` + | ^^^^^^^^^^^^^^ + +error: found non-foreign-function-safe member in struct marked #[repr(C)]: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type + --> $DIR/lint-ctypes.rs:74:30 + | +74 | pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: found struct without + | ^^^^^^^^^^^^^^^^ + +error: aborting due to 20 previous errors + From 7ac5e96f4af8efe4a7f09a873d81006329cb5133 Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Sun, 11 Feb 2018 21:31:42 +0100 Subject: [PATCH 08/23] [improper_ctypes] Use a 'help:' line for possible fixes --- src/librustc_lint/types.rs | 177 +++++++++++++++++++++------------ src/test/ui/lint-ctypes.stderr | 59 ++++++++--- 2 files changed, 156 insertions(+), 80 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index f734f3182a931..b6e8ae9694269 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -353,13 +353,18 @@ struct ImproperCTypesVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, } +struct FfiError { + message: &'static str, + help: Option<&'static str>, +} + enum FfiResult { FfiSafe, FfiPhantom, - FfiUnsafe(&'static str), - FfiBadStruct(DefId, &'static str), - FfiBadUnion(DefId, &'static str), - FfiBadEnum(DefId, &'static str), + FfiUnsafe(FfiError), + FfiBadStruct(DefId, FfiError), + FfiBadUnion(DefId, FfiError), + FfiBadEnum(DefId, FfiError), } /// Check if this enum can be safely exported based on the @@ -434,14 +439,18 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { match def.adt_kind() { AdtKind::Struct => { if !def.repr.c() && !def.repr.transparent() { - return FfiUnsafe("found struct without foreign-function-safe \ - representation annotation in foreign module, \ - consider adding a #[repr(C)] attribute to the type"); + return FfiUnsafe(FfiError { + message: "found struct without foreign-function-safe \ + representation annotation in foreign module", + help: Some("consider adding a #[repr(C)] attribute to the type"), + }); } if def.non_enum_variant().fields.is_empty() { - return FfiUnsafe("found zero-size struct in foreign module, consider \ - adding a member to this struct"); + return FfiUnsafe(FfiError { + message: "found zero-size struct in foreign module", + help: Some("consider adding a member to this struct"), + }); } // We can't completely trust repr(C) and repr(transparent) markings; @@ -471,8 +480,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => { return r; } - FfiUnsafe(s) => { - return FfiBadStruct(def.did, s); + FfiUnsafe(err) => { + return FfiBadStruct(def.did, err); } } } @@ -481,14 +490,18 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } AdtKind::Union => { if !def.repr.c() { - return FfiUnsafe("found union without foreign-function-safe \ - representation annotation in foreign module, \ - consider adding a #[repr(C)] attribute to the type"); + return FfiUnsafe(FfiError { + message: "found union without foreign-function-safe \ + representation annotation in foreign module", + help: Some("consider adding a #[repr(C)] attribute to the type"), + }); } if def.non_enum_variant().fields.is_empty() { - return FfiUnsafe("found zero-size union in foreign module, consider \ - adding a member to this union"); + return FfiUnsafe(FfiError { + message: "found zero-size union in foreign module", + help: Some("consider adding a member to this union"), + }); } let mut all_phantom = true; @@ -505,8 +518,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => { return r; } - FfiUnsafe(s) => { - return FfiBadUnion(def.did, s); + FfiUnsafe(err) => { + return FfiBadUnion(def.did, err); } } } @@ -524,10 +537,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { if !def.repr.c() && def.repr.int.is_none() { // Special-case types like `Option`. if !is_repr_nullable_ptr(cx, def, substs) { - return FfiUnsafe("found enum without foreign-function-safe \ - representation annotation in foreign \ - module, consider adding a #[repr(...)] \ - attribute to the type"); + return FfiUnsafe(FfiError { + message: "found enum without foreign-function-safe \ + representation annotation in foreign module", + help: Some("consider adding a #[repr(...)] attribute \ + to the type"), + }); } } @@ -535,7 +550,10 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { if !is_ffi_safe(int_ty) { // FIXME: This shouldn't be reachable: we should check // this earlier. - return FfiUnsafe("enum has unexpected #[repr(...)] attribute"); + return FfiUnsafe(FfiError { + message: "enum has unexpected #[repr(...)] attribute", + help: None, + }); } // Enum with an explicitly sized discriminant; either @@ -558,11 +576,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { return r; } FfiPhantom => { - return FfiBadEnum(def.did, - "Found phantom data in enum variant"); + return FfiBadEnum(def.did, FfiError { + message: "Found phantom data in enum variant", + help: None, + }); } - FfiUnsafe(s) => { - return FfiBadEnum(def.did, s); + FfiUnsafe(err) => { + return FfiBadEnum(def.did, err); } } } @@ -573,43 +593,57 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } ty::TyChar => { - FfiUnsafe("found Rust type `char` in foreign module, while \ - `u32` or `libc::wchar_t` should be used") + FfiUnsafe(FfiError { + message: "found Rust type `char` in foreign module", + help: Some("consider using `u32` or `libc::wchar_t`"), + }) } ty::TyInt(ast::IntTy::I128) => { - FfiUnsafe("found Rust type `i128` in foreign module, but \ - 128-bit integers don't currently have a known \ - stable ABI") + FfiUnsafe(FfiError { + message: "found Rust type `i128` in foreign module, but 128-bit \ + integers don't currently have a known stable ABI", + help: None, + }) } ty::TyUint(ast::UintTy::U128) => { - FfiUnsafe("found Rust type `u128` in foreign module, but \ - 128-bit integers don't currently have a known \ - stable ABI") + FfiUnsafe(FfiError { + message: "found Rust type `u128` in foreign module, but 128-bit \ + integers don't currently have a known stable ABI", + help: None, + }) } // Primitive types with a stable representation. ty::TyBool | ty::TyInt(..) | ty::TyUint(..) | ty::TyFloat(..) | ty::TyNever => FfiSafe, ty::TySlice(_) => { - FfiUnsafe("found Rust slice type in foreign module, \ - consider using a raw pointer instead") + FfiUnsafe(FfiError { + message: "found Rust slice type in foreign module", + help: Some("consider using a raw pointer instead"), + }) } ty::TyDynamic(..) => { - FfiUnsafe("found Rust trait type in foreign module, \ - consider using a raw pointer instead") + FfiUnsafe(FfiError { + message: "found Rust trait type in foreign module", + help: Some("consider using a raw pointer instead"), + }) } ty::TyStr => { - FfiUnsafe("found Rust type `str` in foreign module; \ - consider using a `*const libc::c_char`") + FfiUnsafe(FfiError { + message: "found Rust type `str` in foreign module", + help: Some("consider using a `*const libc::c_char`"), + }) } ty::TyTuple(..) => { - FfiUnsafe("found Rust tuple type in foreign module; \ - consider using a struct instead") + FfiUnsafe(FfiError { + message: "found Rust tuple type in foreign module", + help: Some("consider using a struct instead"), + }) } ty::TyRawPtr(ref m) | @@ -620,9 +654,11 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { ty::TyFnPtr(sig) => { match sig.abi() { Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic | Abi::RustCall => { - return FfiUnsafe("found function pointer with Rust calling convention in \ - foreign module; consider using an `extern` function \ - pointer") + return FfiUnsafe(FfiError { + message: "found function pointer with Rust calling convention in \ + foreign module", + help: Some("consider using an `extern` function pointer"), + }) } _ => {} } @@ -676,34 +712,45 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { &format!("found zero-sized type composed only \ of phantom-data in a foreign-function.")); } - FfiResult::FfiUnsafe(s) => { - self.cx.span_lint(IMPROPER_CTYPES, sp, s); + FfiResult::FfiUnsafe(err) => { + let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, err.message); + if let Some(s) = err.help { + diag.help(s); + } + diag.emit(); } - FfiResult::FfiBadStruct(_, s) => { + FfiResult::FfiBadStruct(_, err) => { // FIXME: This diagnostic is difficult to read, and doesn't // point at the relevant field. - self.cx.span_lint(IMPROPER_CTYPES, - sp, - &format!("found non-foreign-function-safe member in struct \ - marked #[repr(C)]: {}", - s)); + let msg = format!("found non-foreign-function-safe member in struct \ + marked #[repr(C)]: {}", err.message); + let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); + if let Some(s) = err.help { + diag.help(s); + } + diag.emit(); } - FfiResult::FfiBadUnion(_, s) => { + FfiResult::FfiBadUnion(_, err) => { // FIXME: This diagnostic is difficult to read, and doesn't // point at the relevant field. - self.cx.span_lint(IMPROPER_CTYPES, - sp, - &format!("found non-foreign-function-safe member in union \ - marked #[repr(C)]: {}", - s)); + let msg = format!("found non-foreign-function-safe member in union \ + marked #[repr(C)]: {}", err.message); + let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); + if let Some(s) = err.help { + diag.help(s); + } + diag.emit(); } - FfiResult::FfiBadEnum(_, s) => { + FfiResult::FfiBadEnum(_, err) => { // FIXME: This diagnostic is difficult to read, and doesn't // point at the relevant variant. - self.cx.span_lint(IMPROPER_CTYPES, - sp, - &format!("found non-foreign-function-safe member in enum: {}", - s)); + let msg = format!("found non-foreign-function-safe member in enum: {}", + err.message); + let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); + if let Some(s) = err.help { + diag.help(s); + } + diag.emit(); } } } diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr index 21aadde8ac87c..0f7f7e048e3ab 100644 --- a/src/test/ui/lint-ctypes.stderr +++ b/src/test/ui/lint-ctypes.stderr @@ -1,4 +1,4 @@ -error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type +error: found struct without foreign-function-safe representation annotation in foreign module --> $DIR/lint-ctypes.rs:54:28 | 54 | pub fn ptr_type1(size: *const Foo); //~ ERROR: found struct without @@ -9,36 +9,47 @@ note: lint level defined here | 11 | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ + = help: consider adding a #[repr(C)] attribute to the type -error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type +error: found struct without foreign-function-safe representation annotation in foreign module --> $DIR/lint-ctypes.rs:55:28 | 55 | pub fn ptr_type2(size: *const Foo); //~ ERROR: found struct without | ^^^^^^^^^^ + | + = help: consider adding a #[repr(C)] attribute to the type -error: found Rust slice type in foreign module, consider using a raw pointer instead +error: found Rust slice type in foreign module --> $DIR/lint-ctypes.rs:56:26 | 56 | pub fn slice_type(p: &[u32]); //~ ERROR: found Rust slice type | ^^^^^^ + | + = help: consider using a raw pointer instead -error: found Rust type `str` in foreign module; consider using a `*const libc::c_char` +error: found Rust type `str` in foreign module --> $DIR/lint-ctypes.rs:57:24 | 57 | pub fn str_type(p: &str); //~ ERROR: found Rust type | ^^^^ + | + = help: consider using a `*const libc::c_char` -error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type +error: found struct without foreign-function-safe representation annotation in foreign module --> $DIR/lint-ctypes.rs:58:24 | 58 | pub fn box_type(p: Box); //~ ERROR found struct without | ^^^^^^^^ + | + = help: consider adding a #[repr(C)] attribute to the type -error: found Rust type `char` in foreign module, while `u32` or `libc::wchar_t` should be used +error: found Rust type `char` in foreign module --> $DIR/lint-ctypes.rs:59:25 | 59 | pub fn char_type(p: char); //~ ERROR found Rust type | ^^^^ + | + = help: consider using `u32` or `libc::wchar_t` error: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:60:25 @@ -52,29 +63,37 @@ error: found Rust type `u128` in foreign module, but 128-bit integers don't curr 61 | pub fn u128_type(p: u128); //~ ERROR found Rust type | ^^^^ -error: found Rust trait type in foreign module, consider using a raw pointer instead +error: found Rust trait type in foreign module --> $DIR/lint-ctypes.rs:62:26 | 62 | pub fn trait_type(p: &Clone); //~ ERROR found Rust trait type | ^^^^^^ + | + = help: consider using a raw pointer instead -error: found Rust tuple type in foreign module; consider using a struct instead +error: found Rust tuple type in foreign module --> $DIR/lint-ctypes.rs:63:26 | 63 | pub fn tuple_type(p: (i32, i32)); //~ ERROR found Rust tuple type | ^^^^^^^^^^ + | + = help: consider using a struct instead -error: found Rust tuple type in foreign module; consider using a struct instead +error: found Rust tuple type in foreign module --> $DIR/lint-ctypes.rs:64:27 | 64 | pub fn tuple_type2(p: I32Pair); //~ ERROR found Rust tuple type | ^^^^^^^ + | + = help: consider using a struct instead -error: found zero-size struct in foreign module, consider adding a member to this struct +error: found zero-size struct in foreign module --> $DIR/lint-ctypes.rs:65:25 | 65 | pub fn zero_size(p: ZeroSize); //~ ERROR found zero-size struct | ^^^^^^^^ + | + = help: consider adding a member to this struct error: found zero-sized type composed only of phantom-data in a foreign-function. --> $DIR/lint-ctypes.rs:66:33 @@ -88,23 +107,29 @@ error: found zero-sized type composed only of phantom-data in a foreign-function 68 | -> ::std::marker::PhantomData; //~ ERROR: found zero-sized type | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: found function pointer with Rust calling convention in foreign module; consider using an `extern` function pointer +error: found function pointer with Rust calling convention in foreign module --> $DIR/lint-ctypes.rs:69:23 | 69 | pub fn fn_type(p: RustFn); //~ ERROR found function pointer with Rust | ^^^^^^ + | + = help: consider using an `extern` function pointer -error: found function pointer with Rust calling convention in foreign module; consider using an `extern` function pointer +error: found function pointer with Rust calling convention in foreign module --> $DIR/lint-ctypes.rs:70:24 | 70 | pub fn fn_type2(p: fn()); //~ ERROR found function pointer with Rust | ^^^^ + | + = help: consider using an `extern` function pointer -error: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type +error: found struct without foreign-function-safe representation annotation in foreign module --> $DIR/lint-ctypes.rs:71:28 | 71 | pub fn fn_contained(p: RustBadRet); //~ ERROR: found struct without | ^^^^^^^^^^ + | + = help: consider adding a #[repr(C)] attribute to the type error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:72:32 @@ -112,17 +137,21 @@ error: found non-foreign-function-safe member in struct marked #[repr(C)]: found 72 | pub fn transparent_i128(p: TransparentI128); //~ ERROR: found Rust type `i128` | ^^^^^^^^^^^^^^^ -error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `str` in foreign module; consider using a `*const libc::c_char` +error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `str` in foreign module --> $DIR/lint-ctypes.rs:73:31 | 73 | pub fn transparent_str(p: TransparentStr); //~ ERROR: found Rust type `str` | ^^^^^^^^^^^^^^ + | + = help: consider using a `*const libc::c_char` -error: found non-foreign-function-safe member in struct marked #[repr(C)]: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type +error: found non-foreign-function-safe member in struct marked #[repr(C)]: found struct without foreign-function-safe representation annotation in foreign module --> $DIR/lint-ctypes.rs:74:30 | 74 | pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: found struct without | ^^^^^^^^^^^^^^^^ + | + = help: consider adding a #[repr(C)] attribute to the type error: aborting due to 20 previous errors From ae92dfac5019643b8fb310de9e92f0889b0106ca Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Sun, 11 Feb 2018 21:54:56 +0100 Subject: [PATCH 09/23] [improper_ctypes] Stop complaining about repr(usize) and repr(isize) enums This dates back to at least #26583. At the time, usize and isize were considered ffi-unsafe to nudge people away from them, but this changed in the aforementioned PR, making it inconsistent to complain about it in enum discriminants. In fact, repr(usize) is probably the best way to interface with `enum Foo : size_t { ... }`. --- src/librustc_lint/types.rs | 29 ----------------------- src/test/compile-fail/lint-ctypes-enum.rs | 12 ++++++++++ 2 files changed, 12 insertions(+), 29 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index b6e8ae9694269..f17fa9c7ca1d2 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -26,7 +26,6 @@ use std::{i8, i16, i32, i64, u8, u16, u32, u64, f32, f64}; use syntax::ast; use syntax::abi::Abi; -use syntax::attr; use syntax_pos::Span; use syntax::codemap; @@ -402,17 +401,6 @@ fn is_repr_nullable_ptr<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, false } -fn is_ffi_safe(ty: attr::IntType) -> bool { - match ty { - attr::SignedInt(ast::IntTy::I8) | attr::UnsignedInt(ast::UintTy::U8) | - attr::SignedInt(ast::IntTy::I16) | attr::UnsignedInt(ast::UintTy::U16) | - attr::SignedInt(ast::IntTy::I32) | attr::UnsignedInt(ast::UintTy::U32) | - attr::SignedInt(ast::IntTy::I64) | attr::UnsignedInt(ast::UintTy::U64) | - attr::SignedInt(ast::IntTy::I128) | attr::UnsignedInt(ast::UintTy::U128) => true, - attr::SignedInt(ast::IntTy::Isize) | attr::UnsignedInt(ast::UintTy::Usize) => false - } -} - impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { /// Check if the given type is "ffi-safe" (has a stable, well-defined /// representation which can be exported to C code). @@ -546,23 +534,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } } - if let Some(int_ty) = def.repr.int { - if !is_ffi_safe(int_ty) { - // FIXME: This shouldn't be reachable: we should check - // this earlier. - return FfiUnsafe(FfiError { - message: "enum has unexpected #[repr(...)] attribute", - help: None, - }); - } - - // Enum with an explicitly sized discriminant; either - // a C-style enum or a discriminated union. - - // The layout of enum variants is implicitly repr(C). - // FIXME: Is that correct? - } - // Check the contained variants. for variant in &def.variants { for field in &variant.fields { diff --git a/src/test/compile-fail/lint-ctypes-enum.rs b/src/test/compile-fail/lint-ctypes-enum.rs index e35dadbea9d4d..ce6afe1d219ea 100644 --- a/src/test/compile-fail/lint-ctypes-enum.rs +++ b/src/test/compile-fail/lint-ctypes-enum.rs @@ -16,11 +16,23 @@ enum U { A } enum B { C, D } enum T { E, F, G } +#[repr(C)] +enum ReprC { A, B, C } + +#[repr(u8)] +enum U8 { A, B, C } + +#[repr(isize)] +enum Isize { A, B, C } + extern { fn zf(x: Z); fn uf(x: U); //~ ERROR found enum without foreign-function-safe fn bf(x: B); //~ ERROR found enum without foreign-function-safe fn tf(x: T); //~ ERROR found enum without foreign-function-safe + fn reprc(x: ReprC); + fn u8(x: U8); + fn isize(x: Isize); } pub fn main() { } From 9b5f47ec48e8c12b68cff7cc64afe166358183ef Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Mon, 12 Feb 2018 01:08:48 +0100 Subject: [PATCH 10/23] [improper_ctypes] Overhaul primary label - Always name the non-FFI-safe - Explain *why* the type is not FFI-safe - Stop vaguely gesturing at structs/enums/unions if the non-FFI-safe types occured in a field. The last part is arguably a regression, but it's minor now that the non-FFI-safe type is actually named. Removing it avoids some code duplication. --- src/librustc_lint/types.rs | 238 ++++++++------------ src/test/compile-fail/issue-14309.rs | 10 +- src/test/compile-fail/issue-16250.rs | 2 +- src/test/compile-fail/lint-ctypes-enum.rs | 6 +- src/test/compile-fail/union/union-repr-c.rs | 2 +- src/test/ui/lint-ctypes.rs | 40 ++-- src/test/ui/lint-ctypes.stderr | 100 ++++---- 7 files changed, 171 insertions(+), 227 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index f17fa9c7ca1d2..7203b1b1e7dae 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -10,7 +10,6 @@ #![allow(non_snake_case)] -use rustc::hir::def_id::DefId; use rustc::hir::map as hir_map; use rustc::ty::subst::Substs; use rustc::ty::{self, AdtKind, Ty, TyCtxt}; @@ -352,18 +351,14 @@ struct ImproperCTypesVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, } -struct FfiError { - message: &'static str, - help: Option<&'static str>, -} - -enum FfiResult { +enum FfiResult<'tcx> { FfiSafe, - FfiPhantom, - FfiUnsafe(FfiError), - FfiBadStruct(DefId, FfiError), - FfiBadUnion(DefId, FfiError), - FfiBadEnum(DefId, FfiError), + FfiPhantom(Ty<'tcx>), + FfiUnsafe { + ty: Ty<'tcx>, + reason: &'static str, + help: Option<&'static str>, + }, } /// Check if this enum can be safely exported based on the @@ -406,7 +401,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { /// representation which can be exported to C code). fn check_type_for_ffi(&self, cache: &mut FxHashSet>, - ty: Ty<'tcx>) -> FfiResult { + ty: Ty<'tcx>) -> FfiResult<'tcx> { use self::FfiResult::*; let cx = self.cx.tcx; @@ -422,23 +417,24 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { match ty.sty { ty::TyAdt(def, substs) => { if def.is_phantom_data() { - return FfiPhantom; + return FfiPhantom(ty); } match def.adt_kind() { AdtKind::Struct => { if !def.repr.c() && !def.repr.transparent() { - return FfiUnsafe(FfiError { - message: "found struct without foreign-function-safe \ - representation annotation in foreign module", - help: Some("consider adding a #[repr(C)] attribute to the type"), - }); + return FfiUnsafe { + ty: ty, + reason: "this struct has unspecified layout", + help: Some("consider adding a #[repr(C)] attribute to this struct"), + }; } if def.non_enum_variant().fields.is_empty() { - return FfiUnsafe(FfiError { - message: "found zero-size struct in foreign module", + return FfiUnsafe { + ty: ty, + reason: "this struct has no fields", help: Some("consider adding a member to this struct"), - }); + }; } // We can't completely trust repr(C) and repr(transparent) markings; @@ -464,32 +460,30 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiSafe => { all_phantom = false; } - FfiPhantom => {} - FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => { + FfiPhantom(..) => {} + FfiUnsafe { .. } => { return r; } - FfiUnsafe(err) => { - return FfiBadStruct(def.did, err); - } } } - if all_phantom { FfiPhantom } else { FfiSafe } + if all_phantom { FfiPhantom(ty) } else { FfiSafe } } AdtKind::Union => { if !def.repr.c() { - return FfiUnsafe(FfiError { - message: "found union without foreign-function-safe \ - representation annotation in foreign module", - help: Some("consider adding a #[repr(C)] attribute to the type"), - }); + return FfiUnsafe { + ty: ty, + reason: "this union has unspecified layout", + help: Some("consider adding a #[repr(C)] attribute to this union"), + }; } if def.non_enum_variant().fields.is_empty() { - return FfiUnsafe(FfiError { - message: "found zero-size union in foreign module", - help: Some("consider adding a member to this union"), - }); + return FfiUnsafe { + ty: ty, + reason: "this union has no fields", + help: Some("consider adding a field to this union"), + }; } let mut all_phantom = true; @@ -502,17 +496,14 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiSafe => { all_phantom = false; } - FfiPhantom => {} - FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => { + FfiPhantom(..) => {} + FfiUnsafe { .. } => { return r; } - FfiUnsafe(err) => { - return FfiBadUnion(def.did, err); - } } } - if all_phantom { FfiPhantom } else { FfiSafe } + if all_phantom { FfiPhantom(ty) } else { FfiSafe } } AdtKind::Enum => { if def.variants.is_empty() { @@ -525,12 +516,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { if !def.repr.c() && def.repr.int.is_none() { // Special-case types like `Option`. if !is_repr_nullable_ptr(cx, def, substs) { - return FfiUnsafe(FfiError { - message: "found enum without foreign-function-safe \ - representation annotation in foreign module", + return FfiUnsafe { + ty: ty, + reason: "enum has no representation hint", help: Some("consider adding a #[repr(...)] attribute \ - to the type"), - }); + to this enum"), + }; } } @@ -543,17 +534,15 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { let r = self.check_type_for_ffi(cache, arg); match r { FfiSafe => {} - FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => { + FfiUnsafe { .. } => { return r; } - FfiPhantom => { - return FfiBadEnum(def.did, FfiError { - message: "Found phantom data in enum variant", + FfiPhantom(..) => { + return FfiUnsafe { + ty: ty, + reason: "this enum contains a PhantomData field", help: None, - }); - } - FfiUnsafe(err) => { - return FfiBadEnum(def.did, err); + }; } } } @@ -563,59 +552,44 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } } - ty::TyChar => { - FfiUnsafe(FfiError { - message: "found Rust type `char` in foreign module", - help: Some("consider using `u32` or `libc::wchar_t`"), - }) - } + ty::TyChar => FfiUnsafe { + ty: ty, + reason: "the `char` type has no C equivalent", + help: Some("consider using `u32` or `libc::wchar_t` instead"), + }, - ty::TyInt(ast::IntTy::I128) => { - FfiUnsafe(FfiError { - message: "found Rust type `i128` in foreign module, but 128-bit \ - integers don't currently have a known stable ABI", - help: None, - }) - } - - ty::TyUint(ast::UintTy::U128) => { - FfiUnsafe(FfiError { - message: "found Rust type `u128` in foreign module, but 128-bit \ - integers don't currently have a known stable ABI", - help: None, - }) - } + ty::TyInt(ast::IntTy::I128) | ty::TyUint(ast::UintTy::U128) => FfiUnsafe { + ty: ty, + reason: "128-bit integers don't currently have a known stable ABI", + help: None, + }, // Primitive types with a stable representation. ty::TyBool | ty::TyInt(..) | ty::TyUint(..) | ty::TyFloat(..) | ty::TyNever => FfiSafe, - ty::TySlice(_) => { - FfiUnsafe(FfiError { - message: "found Rust slice type in foreign module", - help: Some("consider using a raw pointer instead"), - }) - } - - ty::TyDynamic(..) => { - FfiUnsafe(FfiError { - message: "found Rust trait type in foreign module", - help: Some("consider using a raw pointer instead"), - }) - } - - ty::TyStr => { - FfiUnsafe(FfiError { - message: "found Rust type `str` in foreign module", - help: Some("consider using a `*const libc::c_char`"), - }) - } - - ty::TyTuple(..) => { - FfiUnsafe(FfiError { - message: "found Rust tuple type in foreign module", - help: Some("consider using a struct instead"), - }) - } + ty::TySlice(_) => FfiUnsafe { + ty: ty, + reason: "slices have no C equivalent", + help: Some("consider using a raw pointer instead"), + }, + + ty::TyDynamic(..) => FfiUnsafe { + ty: ty, + reason: "trait objects have no C equivalent", + help: Some("consider using a raw pointer instead"), + }, + + ty::TyStr => FfiUnsafe { + ty: ty, + reason: "string slices have no C equivalent", + help: Some("consider using `*const u8` and a length instead"), + }, + + ty::TyTuple(..) => FfiUnsafe { + ty: ty, + reason: "tuples have unspecified layout", + help: Some("consider using a struct instead"), + }, ty::TyRawPtr(ref m) | ty::TyRef(_, ref m) => self.check_type_for_ffi(cache, m.ty), @@ -625,11 +599,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { ty::TyFnPtr(sig) => { match sig.abi() { Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic | Abi::RustCall => { - return FfiUnsafe(FfiError { - message: "found function pointer with Rust calling convention in \ - foreign module", - help: Some("consider using an `extern` function pointer"), - }) + return FfiUnsafe { + ty: ty, + reason: "this function pointer has Rust-specific calling convention", + help: Some("consider using an `fn \"extern\"(...) -> ...` \ + function pointer instead"), + } } _ => {} } @@ -677,48 +652,17 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { match self.check_type_for_ffi(&mut FxHashSet(), ty) { FfiResult::FfiSafe => {} - FfiResult::FfiPhantom => { + FfiResult::FfiPhantom(ty) => { self.cx.span_lint(IMPROPER_CTYPES, sp, - &format!("found zero-sized type composed only \ - of phantom-data in a foreign-function.")); - } - FfiResult::FfiUnsafe(err) => { - let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, err.message); - if let Some(s) = err.help { - diag.help(s); - } - diag.emit(); - } - FfiResult::FfiBadStruct(_, err) => { - // FIXME: This diagnostic is difficult to read, and doesn't - // point at the relevant field. - let msg = format!("found non-foreign-function-safe member in struct \ - marked #[repr(C)]: {}", err.message); - let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); - if let Some(s) = err.help { - diag.help(s); - } - diag.emit(); - } - FfiResult::FfiBadUnion(_, err) => { - // FIXME: This diagnostic is difficult to read, and doesn't - // point at the relevant field. - let msg = format!("found non-foreign-function-safe member in union \ - marked #[repr(C)]: {}", err.message); - let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); - if let Some(s) = err.help { - diag.help(s); - } - diag.emit(); + &format!("`extern` block uses type `{}` which is not FFI-safe: \ + composed only of PhantomData", ty)); } - FfiResult::FfiBadEnum(_, err) => { - // FIXME: This diagnostic is difficult to read, and doesn't - // point at the relevant variant. - let msg = format!("found non-foreign-function-safe member in enum: {}", - err.message); + FfiResult::FfiUnsafe { ty: unsafe_ty, reason, help } => { + let msg = format!("`extern` block uses type `{}` which is not FFI-safe: {}", + unsafe_ty, reason); let mut diag = self.cx.struct_span_lint(IMPROPER_CTYPES, sp, &msg); - if let Some(s) = err.help { + if let Some(s) = help { diag.help(s); } diag.emit(); diff --git a/src/test/compile-fail/issue-14309.rs b/src/test/compile-fail/issue-14309.rs index 56261c34a0308..f76fa3e4a8ece 100644 --- a/src/test/compile-fail/issue-14309.rs +++ b/src/test/compile-fail/issue-14309.rs @@ -37,13 +37,13 @@ struct D { } extern "C" { - fn foo(x: A); //~ ERROR found struct without foreign-function-safe - fn bar(x: B); //~ ERROR foreign-function-safe + fn foo(x: A); //~ ERROR type `A` which is not FFI-safe + fn bar(x: B); //~ ERROR type `A` fn baz(x: C); - fn qux(x: A2); //~ ERROR foreign-function-safe - fn quux(x: B2); //~ ERROR foreign-function-safe + fn qux(x: A2); //~ ERROR type `A` + fn quux(x: B2); //~ ERROR type `A` fn corge(x: C2); - fn fred(x: D); //~ ERROR foreign-function-safe + fn fred(x: D); //~ ERROR type `A` } fn main() { } diff --git a/src/test/compile-fail/issue-16250.rs b/src/test/compile-fail/issue-16250.rs index 288fe4a9abb82..f9d01003005e4 100644 --- a/src/test/compile-fail/issue-16250.rs +++ b/src/test/compile-fail/issue-16250.rs @@ -13,7 +13,7 @@ pub struct Foo; extern { - pub fn foo(x: (Foo)); //~ ERROR found struct without + pub fn foo(x: (Foo)); //~ ERROR unspecified layout } fn main() { diff --git a/src/test/compile-fail/lint-ctypes-enum.rs b/src/test/compile-fail/lint-ctypes-enum.rs index ce6afe1d219ea..7b7ffd8fc107c 100644 --- a/src/test/compile-fail/lint-ctypes-enum.rs +++ b/src/test/compile-fail/lint-ctypes-enum.rs @@ -27,9 +27,9 @@ enum Isize { A, B, C } extern { fn zf(x: Z); - fn uf(x: U); //~ ERROR found enum without foreign-function-safe - fn bf(x: B); //~ ERROR found enum without foreign-function-safe - fn tf(x: T); //~ ERROR found enum without foreign-function-safe + fn uf(x: U); //~ ERROR enum has no representation hint + fn bf(x: B); //~ ERROR enum has no representation hint + fn tf(x: T); //~ ERROR enum has no representation hint fn reprc(x: ReprC); fn u8(x: U8); fn isize(x: Isize); diff --git a/src/test/compile-fail/union/union-repr-c.rs b/src/test/compile-fail/union/union-repr-c.rs index 15a4197fe94c9..36c42ce1104e0 100644 --- a/src/test/compile-fail/union/union-repr-c.rs +++ b/src/test/compile-fail/union/union-repr-c.rs @@ -22,7 +22,7 @@ union W { extern "C" { static FOREIGN1: U; // OK - static FOREIGN2: W; //~ ERROR found union without foreign-function-safe representation + static FOREIGN2: W; //~ ERROR union has unspecified layout } fn main() {} diff --git a/src/test/ui/lint-ctypes.rs b/src/test/ui/lint-ctypes.rs index c22239dee0a80..77cb1ef0f5130 100644 --- a/src/test/ui/lint-ctypes.rs +++ b/src/test/ui/lint-ctypes.rs @@ -51,27 +51,27 @@ pub struct TransparentCustomZst(i32, ZeroSize); pub struct ZeroSizeWithPhantomData(::std::marker::PhantomData); extern { - pub fn ptr_type1(size: *const Foo); //~ ERROR: found struct without - pub fn ptr_type2(size: *const Foo); //~ ERROR: found struct without - pub fn slice_type(p: &[u32]); //~ ERROR: found Rust slice type - pub fn str_type(p: &str); //~ ERROR: found Rust type - pub fn box_type(p: Box); //~ ERROR found struct without - pub fn char_type(p: char); //~ ERROR found Rust type - pub fn i128_type(p: i128); //~ ERROR found Rust type - pub fn u128_type(p: u128); //~ ERROR found Rust type - pub fn trait_type(p: &Clone); //~ ERROR found Rust trait type - pub fn tuple_type(p: (i32, i32)); //~ ERROR found Rust tuple type - pub fn tuple_type2(p: I32Pair); //~ ERROR found Rust tuple type - pub fn zero_size(p: ZeroSize); //~ ERROR found zero-size struct - pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR found zero-sized type + pub fn ptr_type1(size: *const Foo); //~ ERROR: uses type `Foo` + pub fn ptr_type2(size: *const Foo); //~ ERROR: uses type `Foo` + pub fn slice_type(p: &[u32]); //~ ERROR: uses type `[u32]` + pub fn str_type(p: &str); //~ ERROR: uses type `str` + pub fn box_type(p: Box); //~ ERROR uses type `std::boxed::Box` + pub fn char_type(p: char); //~ ERROR uses type `char` + pub fn i128_type(p: i128); //~ ERROR uses type `i128` + pub fn u128_type(p: u128); //~ ERROR uses type `u128` + pub fn trait_type(p: &Clone); //~ ERROR uses type `std::clone::Clone` + pub fn tuple_type(p: (i32, i32)); //~ ERROR uses type `(i32, i32)` + pub fn tuple_type2(p: I32Pair); //~ ERROR uses type `(i32, i32)` + pub fn zero_size(p: ZeroSize); //~ ERROR struct has no fields + pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR composed only of PhantomData pub fn zero_size_phantom_toplevel() - -> ::std::marker::PhantomData; //~ ERROR: found zero-sized type - pub fn fn_type(p: RustFn); //~ ERROR found function pointer with Rust - pub fn fn_type2(p: fn()); //~ ERROR found function pointer with Rust - pub fn fn_contained(p: RustBadRet); //~ ERROR: found struct without - pub fn transparent_i128(p: TransparentI128); //~ ERROR: found Rust type `i128` - pub fn transparent_str(p: TransparentStr); //~ ERROR: found Rust type `str` - pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: found struct without + -> ::std::marker::PhantomData; //~ ERROR: composed only of PhantomData + pub fn fn_type(p: RustFn); //~ ERROR function pointer has Rust-specific + pub fn fn_type2(p: fn()); //~ ERROR function pointer has Rust-specific + pub fn fn_contained(p: RustBadRet); //~ ERROR: uses type `std::boxed::Box` + pub fn transparent_i128(p: TransparentI128); //~ ERROR: uses type `i128` + pub fn transparent_str(p: TransparentStr); //~ ERROR: uses type `str` + pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: uses type `std::boxed::Box` pub fn good3(fptr: Option); pub fn good4(aptr: &[u8; 4 as usize]); diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr index 0f7f7e048e3ab..8ecdae07a5329 100644 --- a/src/test/ui/lint-ctypes.stderr +++ b/src/test/ui/lint-ctypes.stderr @@ -1,7 +1,7 @@ -error: found struct without foreign-function-safe representation annotation in foreign module +error: `extern` block uses type `Foo` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:54:28 | -54 | pub fn ptr_type1(size: *const Foo); //~ ERROR: found struct without +54 | pub fn ptr_type1(size: *const Foo); //~ ERROR: uses type `Foo` | ^^^^^^^^^^ | note: lint level defined here @@ -9,149 +9,149 @@ note: lint level defined here | 11 | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ - = help: consider adding a #[repr(C)] attribute to the type + = help: consider adding a #[repr(C)] attribute to this struct -error: found struct without foreign-function-safe representation annotation in foreign module +error: `extern` block uses type `Foo` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:55:28 | -55 | pub fn ptr_type2(size: *const Foo); //~ ERROR: found struct without +55 | pub fn ptr_type2(size: *const Foo); //~ ERROR: uses type `Foo` | ^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to the type + = help: consider adding a #[repr(C)] attribute to this struct -error: found Rust slice type in foreign module +error: `extern` block uses type `[u32]` which is not FFI-safe: slices have no C equivalent --> $DIR/lint-ctypes.rs:56:26 | -56 | pub fn slice_type(p: &[u32]); //~ ERROR: found Rust slice type +56 | pub fn slice_type(p: &[u32]); //~ ERROR: uses type `[u32]` | ^^^^^^ | = help: consider using a raw pointer instead -error: found Rust type `str` in foreign module +error: `extern` block uses type `str` which is not FFI-safe: string slices have no C equivalent --> $DIR/lint-ctypes.rs:57:24 | -57 | pub fn str_type(p: &str); //~ ERROR: found Rust type +57 | pub fn str_type(p: &str); //~ ERROR: uses type `str` | ^^^^ | - = help: consider using a `*const libc::c_char` + = help: consider using `*const u8` and a length instead -error: found struct without foreign-function-safe representation annotation in foreign module +error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:58:24 | -58 | pub fn box_type(p: Box); //~ ERROR found struct without +58 | pub fn box_type(p: Box); //~ ERROR uses type `std::boxed::Box` | ^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to the type + = help: consider adding a #[repr(C)] attribute to this struct -error: found Rust type `char` in foreign module +error: `extern` block uses type `char` which is not FFI-safe: the `char` type has no C equivalent --> $DIR/lint-ctypes.rs:59:25 | -59 | pub fn char_type(p: char); //~ ERROR found Rust type +59 | pub fn char_type(p: char); //~ ERROR uses type `char` | ^^^^ | - = help: consider using `u32` or `libc::wchar_t` + = help: consider using `u32` or `libc::wchar_t` instead -error: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI +error: `extern` block uses type `i128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:60:25 | -60 | pub fn i128_type(p: i128); //~ ERROR found Rust type +60 | pub fn i128_type(p: i128); //~ ERROR uses type `i128` | ^^^^ -error: found Rust type `u128` in foreign module, but 128-bit integers don't currently have a known stable ABI +error: `extern` block uses type `u128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:61:25 | -61 | pub fn u128_type(p: u128); //~ ERROR found Rust type +61 | pub fn u128_type(p: u128); //~ ERROR uses type `u128` | ^^^^ -error: found Rust trait type in foreign module +error: `extern` block uses type `std::clone::Clone` which is not FFI-safe: trait objects have no C equivalent --> $DIR/lint-ctypes.rs:62:26 | -62 | pub fn trait_type(p: &Clone); //~ ERROR found Rust trait type +62 | pub fn trait_type(p: &Clone); //~ ERROR uses type `std::clone::Clone` | ^^^^^^ | = help: consider using a raw pointer instead -error: found Rust tuple type in foreign module +error: `extern` block uses type `(i32, i32)` which is not FFI-safe: tuples have unspecified layout --> $DIR/lint-ctypes.rs:63:26 | -63 | pub fn tuple_type(p: (i32, i32)); //~ ERROR found Rust tuple type +63 | pub fn tuple_type(p: (i32, i32)); //~ ERROR uses type `(i32, i32)` | ^^^^^^^^^^ | = help: consider using a struct instead -error: found Rust tuple type in foreign module +error: `extern` block uses type `(i32, i32)` which is not FFI-safe: tuples have unspecified layout --> $DIR/lint-ctypes.rs:64:27 | -64 | pub fn tuple_type2(p: I32Pair); //~ ERROR found Rust tuple type +64 | pub fn tuple_type2(p: I32Pair); //~ ERROR uses type `(i32, i32)` | ^^^^^^^ | = help: consider using a struct instead -error: found zero-size struct in foreign module +error: `extern` block uses type `ZeroSize` which is not FFI-safe: this struct has no fields --> $DIR/lint-ctypes.rs:65:25 | -65 | pub fn zero_size(p: ZeroSize); //~ ERROR found zero-size struct +65 | pub fn zero_size(p: ZeroSize); //~ ERROR struct has no fields | ^^^^^^^^ | = help: consider adding a member to this struct -error: found zero-sized type composed only of phantom-data in a foreign-function. +error: `extern` block uses type `ZeroSizeWithPhantomData` which is not FFI-safe: composed only of PhantomData --> $DIR/lint-ctypes.rs:66:33 | -66 | pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR found zero-sized type +66 | pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR composed only of PhantomData | ^^^^^^^^^^^^^^^^^^^^^^^ -error: found zero-sized type composed only of phantom-data in a foreign-function. +error: `extern` block uses type `std::marker::PhantomData` which is not FFI-safe: composed only of PhantomData --> $DIR/lint-ctypes.rs:68:12 | -68 | -> ::std::marker::PhantomData; //~ ERROR: found zero-sized type +68 | -> ::std::marker::PhantomData; //~ ERROR: composed only of PhantomData | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: found function pointer with Rust calling convention in foreign module +error: `extern` block uses type `fn()` which is not FFI-safe: this function pointer has Rust-specific calling convention --> $DIR/lint-ctypes.rs:69:23 | -69 | pub fn fn_type(p: RustFn); //~ ERROR found function pointer with Rust +69 | pub fn fn_type(p: RustFn); //~ ERROR function pointer has Rust-specific | ^^^^^^ | - = help: consider using an `extern` function pointer + = help: consider using an `fn "extern"(...) -> ...` function pointer instead -error: found function pointer with Rust calling convention in foreign module +error: `extern` block uses type `fn()` which is not FFI-safe: this function pointer has Rust-specific calling convention --> $DIR/lint-ctypes.rs:70:24 | -70 | pub fn fn_type2(p: fn()); //~ ERROR found function pointer with Rust +70 | pub fn fn_type2(p: fn()); //~ ERROR function pointer has Rust-specific | ^^^^ | - = help: consider using an `extern` function pointer + = help: consider using an `fn "extern"(...) -> ...` function pointer instead -error: found struct without foreign-function-safe representation annotation in foreign module +error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:71:28 | -71 | pub fn fn_contained(p: RustBadRet); //~ ERROR: found struct without +71 | pub fn fn_contained(p: RustBadRet); //~ ERROR: uses type `std::boxed::Box` | ^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to the type + = help: consider adding a #[repr(C)] attribute to this struct -error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `i128` in foreign module, but 128-bit integers don't currently have a known stable ABI +error: `extern` block uses type `i128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:72:32 | -72 | pub fn transparent_i128(p: TransparentI128); //~ ERROR: found Rust type `i128` +72 | pub fn transparent_i128(p: TransparentI128); //~ ERROR: uses type `i128` | ^^^^^^^^^^^^^^^ -error: found non-foreign-function-safe member in struct marked #[repr(C)]: found Rust type `str` in foreign module +error: `extern` block uses type `str` which is not FFI-safe: string slices have no C equivalent --> $DIR/lint-ctypes.rs:73:31 | -73 | pub fn transparent_str(p: TransparentStr); //~ ERROR: found Rust type `str` +73 | pub fn transparent_str(p: TransparentStr); //~ ERROR: uses type `str` | ^^^^^^^^^^^^^^ | - = help: consider using a `*const libc::c_char` + = help: consider using `*const u8` and a length instead -error: found non-foreign-function-safe member in struct marked #[repr(C)]: found struct without foreign-function-safe representation annotation in foreign module +error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:74:30 | -74 | pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: found struct without +74 | pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: uses type `std::boxed::Box` | ^^^^^^^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to the type + = help: consider adding a #[repr(C)] attribute to this struct error: aborting due to 20 previous errors From 22a171609bf555833b277a70420a24696dd8805d Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Wed, 14 Feb 2018 19:39:04 +0100 Subject: [PATCH 11/23] [improper_ctypes] Suggest repr(transparent) for structs The suggestion is unconditional, so following it could lead to further errors. This is already the case for the repr(C) suggestion, which makes this acceptable, though not *good*. Checking up-front whether the suggestion can help would be great but applies more broadly (and would require some refactoring to avoid duplicating the checks). --- src/librustc_lint/types.rs | 3 ++- src/test/ui/lint-ctypes.stderr | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 7203b1b1e7dae..378fe99bf3150 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -425,7 +425,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { return FfiUnsafe { ty: ty, reason: "this struct has unspecified layout", - help: Some("consider adding a #[repr(C)] attribute to this struct"), + help: Some("consider adding a #[repr(C)] or #[repr(transparent)] \ + attribute to this struct"), }; } diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr index 8ecdae07a5329..a8628c8b3d2d9 100644 --- a/src/test/ui/lint-ctypes.stderr +++ b/src/test/ui/lint-ctypes.stderr @@ -9,7 +9,7 @@ note: lint level defined here | 11 | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ - = help: consider adding a #[repr(C)] attribute to this struct + = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct error: `extern` block uses type `Foo` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:55:28 @@ -17,7 +17,7 @@ error: `extern` block uses type `Foo` which is not FFI-safe: this struct has uns 55 | pub fn ptr_type2(size: *const Foo); //~ ERROR: uses type `Foo` | ^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to this struct + = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct error: `extern` block uses type `[u32]` which is not FFI-safe: slices have no C equivalent --> $DIR/lint-ctypes.rs:56:26 @@ -41,7 +41,7 @@ error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: th 58 | pub fn box_type(p: Box); //~ ERROR uses type `std::boxed::Box` | ^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to this struct + = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct error: `extern` block uses type `char` which is not FFI-safe: the `char` type has no C equivalent --> $DIR/lint-ctypes.rs:59:25 @@ -129,7 +129,7 @@ error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: th 71 | pub fn fn_contained(p: RustBadRet); //~ ERROR: uses type `std::boxed::Box` | ^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to this struct + = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct error: `extern` block uses type `i128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI --> $DIR/lint-ctypes.rs:72:32 @@ -151,7 +151,7 @@ error: `extern` block uses type `std::boxed::Box` which is not FFI-safe: th 74 | pub fn transparent_fn(p: TransparentBadFn); //~ ERROR: uses type `std::boxed::Box` | ^^^^^^^^^^^^^^^^ | - = help: consider adding a #[repr(C)] attribute to this struct + = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct error: aborting due to 20 previous errors From 9d493c897b4382dc145b9448b3fafdfbbaecf528 Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Wed, 14 Feb 2018 23:01:39 +0100 Subject: [PATCH 12/23] [improper_ctypes] Point at definition of non-FFI-safe type if possible --- src/librustc_lint/types.rs | 5 +++++ src/test/ui/lint-ctypes.stderr | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 378fe99bf3150..396d830d85be1 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -666,6 +666,11 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { if let Some(s) = help { diag.help(s); } + if let ty::TyAdt(def, _) = unsafe_ty.sty { + if let Some(sp) = self.cx.tcx.hir.span_if_local(def.did) { + diag.span_note(sp, "type defined here"); + } + } diag.emit(); } } diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr index a8628c8b3d2d9..2abf08d55f7b3 100644 --- a/src/test/ui/lint-ctypes.stderr +++ b/src/test/ui/lint-ctypes.stderr @@ -10,6 +10,11 @@ note: lint level defined here 11 | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct +note: type defined here + --> $DIR/lint-ctypes.rs:32:1 + | +32 | pub struct Foo; + | ^^^^^^^^^^^^^^^ error: `extern` block uses type `Foo` which is not FFI-safe: this struct has unspecified layout --> $DIR/lint-ctypes.rs:55:28 @@ -18,6 +23,11 @@ error: `extern` block uses type `Foo` which is not FFI-safe: this struct has uns | ^^^^^^^^^^ | = help: consider adding a #[repr(C)] or #[repr(transparent)] attribute to this struct +note: type defined here + --> $DIR/lint-ctypes.rs:32:1 + | +32 | pub struct Foo; + | ^^^^^^^^^^^^^^^ error: `extern` block uses type `[u32]` which is not FFI-safe: slices have no C equivalent --> $DIR/lint-ctypes.rs:56:26 @@ -94,6 +104,11 @@ error: `extern` block uses type `ZeroSize` which is not FFI-safe: this struct ha | ^^^^^^^^ | = help: consider adding a member to this struct +note: type defined here + --> $DIR/lint-ctypes.rs:28:1 + | +28 | pub struct ZeroSize; + | ^^^^^^^^^^^^^^^^^^^^ error: `extern` block uses type `ZeroSizeWithPhantomData` which is not FFI-safe: composed only of PhantomData --> $DIR/lint-ctypes.rs:66:33 From 051ea5cc9bac00c7f588b4eae72b8a382d8ceb68 Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Wed, 14 Feb 2018 23:11:29 +0100 Subject: [PATCH 13/23] [improper_ctypes] Don't suggest raw pointers when encountering trait objects It's unhelpful since raw pointers to trait objects are also FFI-unsafe and casting to a thin raw pointer loses the vtable. There are working solutions that _involve_ raw pointers but they're too complex to explain in one line and have serious trade offs. --- src/librustc_lint/types.rs | 2 +- src/test/ui/lint-ctypes.stderr | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 396d830d85be1..ef9b3d38c637c 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -577,7 +577,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { ty::TyDynamic(..) => FfiUnsafe { ty: ty, reason: "trait objects have no C equivalent", - help: Some("consider using a raw pointer instead"), + help: None, }, ty::TyStr => FfiUnsafe { diff --git a/src/test/ui/lint-ctypes.stderr b/src/test/ui/lint-ctypes.stderr index 2abf08d55f7b3..748c311055fa9 100644 --- a/src/test/ui/lint-ctypes.stderr +++ b/src/test/ui/lint-ctypes.stderr @@ -78,8 +78,6 @@ error: `extern` block uses type `std::clone::Clone` which is not FFI-safe: trait | 62 | pub fn trait_type(p: &Clone); //~ ERROR uses type `std::clone::Clone` | ^^^^^^ - | - = help: consider using a raw pointer instead error: `extern` block uses type `(i32, i32)` which is not FFI-safe: tuples have unspecified layout --> $DIR/lint-ctypes.rs:63:26 From 7041ef3cf437acd39262eed4cec0d4aa3c1ff1f9 Mon Sep 17 00:00:00 2001 From: andjo403 Date: Wed, 14 Feb 2018 21:08:21 +0100 Subject: [PATCH 14/23] lookup exported symbols only when needed. reduces the time to emit dep-info and metadata. --- src/librustc_trans/back/lto.rs | 9 ++++++--- src/librustc_trans/back/write.rs | 25 ++++++++++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/librustc_trans/back/lto.rs b/src/librustc_trans/back/lto.rs index 9ff5bcf7a33ca..c440393a25f0a 100644 --- a/src/librustc_trans/back/lto.rs +++ b/src/librustc_trans/back/lto.rs @@ -122,8 +122,9 @@ pub(crate) fn run(cgcx: &CodegenContext, None } }; - - let mut symbol_white_list = cgcx.exported_symbols[&LOCAL_CRATE] + let exported_symbols = cgcx.exported_symbols + .as_ref().expect("needs exported symbols for LTO"); + let mut symbol_white_list = exported_symbols[&LOCAL_CRATE] .iter() .filter_map(symbol_filter) .collect::>(); @@ -156,8 +157,10 @@ pub(crate) fn run(cgcx: &CodegenContext, } for &(cnum, ref path) in cgcx.each_linked_rlib_for_lto.iter() { + let exported_symbols = cgcx.exported_symbols + .as_ref().expect("needs exported symbols for LTO"); symbol_white_list.extend( - cgcx.exported_symbols[&cnum] + exported_symbols[&cnum] .iter() .filter_map(symbol_filter)); diff --git a/src/librustc_trans/back/write.rs b/src/librustc_trans/back/write.rs index ded9a296817b3..af5178eb565db 100644 --- a/src/librustc_trans/back/write.rs +++ b/src/librustc_trans/back/write.rs @@ -333,7 +333,7 @@ pub struct CodegenContext { pub no_landing_pads: bool, pub save_temps: bool, pub fewer_names: bool, - pub exported_symbols: Arc, + pub exported_symbols: Option>, pub opts: Arc, pub crate_types: Vec, pub each_linked_rlib_for_lto: Vec<(CrateNum, PathBuf)>, @@ -1394,14 +1394,25 @@ fn start_executing_work(tcx: TyCtxt, allocator_config: Arc) -> thread::JoinHandle> { let coordinator_send = tcx.tx_to_llvm_workers.clone(); - let mut exported_symbols = FxHashMap(); - exported_symbols.insert(LOCAL_CRATE, tcx.exported_symbols(LOCAL_CRATE)); - for &cnum in tcx.crates().iter() { - exported_symbols.insert(cnum, tcx.exported_symbols(cnum)); - } - let exported_symbols = Arc::new(exported_symbols); let sess = tcx.sess; + let exported_symbols = match sess.lto() { + Lto::No => None, + Lto::ThinLocal => { + let mut exported_symbols = FxHashMap(); + exported_symbols.insert(LOCAL_CRATE, tcx.exported_symbols(LOCAL_CRATE)); + Some(Arc::new(exported_symbols)) + } + Lto::Yes | Lto::Fat | Lto::Thin => { + let mut exported_symbols = FxHashMap(); + exported_symbols.insert(LOCAL_CRATE, tcx.exported_symbols(LOCAL_CRATE)); + for &cnum in tcx.crates().iter() { + exported_symbols.insert(cnum, tcx.exported_symbols(cnum)); + } + Some(Arc::new(exported_symbols)) + } + }; + // First up, convert our jobserver into a helper thread so we can use normal // mpsc channels to manage our messages and such. // After we've requested tokens then we'll, when we can, From 9d3719bcfa45c977dcfd12a2d5f188652f56bdaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Thu, 1 Feb 2018 18:10:56 +0100 Subject: [PATCH 15/23] Do not run the default panic hook inside procedural macros. Fixes #47812 --- src/Cargo.lock | 2 ++ src/libproc_macro/lib.rs | 6 ++++++ src/librustc/Cargo.toml | 2 ++ src/librustc/lib.rs | 3 +++ src/librustc/util/common.rs | 21 +++++++++++++++++++ src/librustc_driver/driver.rs | 4 +++- src/test/ui-fulldeps/proc-macro/load-panic.rs | 1 + .../ui-fulldeps/proc-macro/load-panic.stderr | 4 ++-- 8 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/Cargo.lock b/src/Cargo.lock index d8306c66daf84..5b56c6e12d624 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -1623,7 +1623,9 @@ dependencies = [ "fmt_macros 0.0.0", "graphviz 0.0.0", "jobserver 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)", + "lazy_static 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", + "proc_macro 0.0.0", "rustc_apfloat 0.0.0", "rustc_back 0.0.0", "rustc_const_math 0.0.0", diff --git a/src/libproc_macro/lib.rs b/src/libproc_macro/lib.rs index 6768e0ade4304..878a536836d23 100644 --- a/src/libproc_macro/lib.rs +++ b/src/libproc_macro/lib.rs @@ -844,6 +844,12 @@ pub mod __internal { }) } + pub fn in_sess() -> bool + { + let p = CURRENT_SESS.with(|p| p.get()); + !p.0.is_null() + } + pub fn with_sess(f: F) -> R where F: FnOnce((&ParseSess, Mark)) -> R { diff --git a/src/librustc/Cargo.toml b/src/librustc/Cargo.toml index 2c4898cb2c006..7e84a69dd7913 100644 --- a/src/librustc/Cargo.toml +++ b/src/librustc/Cargo.toml @@ -14,7 +14,9 @@ bitflags = "1.0" fmt_macros = { path = "../libfmt_macros" } graphviz = { path = "../libgraphviz" } jobserver = "0.1" +lazy_static = "1.0.0" log = { version = "0.4", features = ["release_max_level_info", "std"] } +proc_macro = { path = "../libproc_macro" } rustc_apfloat = { path = "../librustc_apfloat" } rustc_back = { path = "../librustc_back" } rustc_const_math = { path = "../librustc_const_math" } diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs index a7a2619505931..79333d1018ab1 100644 --- a/src/librustc/lib.rs +++ b/src/librustc/lib.rs @@ -60,6 +60,7 @@ #![feature(never_type)] #![feature(non_exhaustive)] #![feature(nonzero)] +#![feature(proc_macro_internals)] #![feature(quote)] #![feature(refcell_replace_swap)] #![feature(rustc_diagnostic_macros)] @@ -80,6 +81,7 @@ extern crate core; extern crate fmt_macros; extern crate getopts; extern crate graphviz; +#[macro_use] extern crate lazy_static; #[cfg(windows)] extern crate libc; extern crate rustc_back; @@ -91,6 +93,7 @@ extern crate rustc_errors as errors; #[macro_use] extern crate syntax; extern crate syntax_pos; extern crate jobserver; +extern crate proc_macro; extern crate serialize as rustc_serialize; // used by deriving diff --git a/src/librustc/util/common.rs b/src/librustc/util/common.rs index 2971f3e853a99..55e9a98e7ef13 100644 --- a/src/librustc/util/common.rs +++ b/src/librustc/util/common.rs @@ -16,6 +16,7 @@ use std::ffi::CString; use std::fmt::Debug; use std::hash::{Hash, BuildHasher}; use std::iter::repeat; +use std::panic; use std::path::Path; use std::time::{Duration, Instant}; @@ -23,6 +24,8 @@ use std::sync::mpsc::{Sender}; use syntax_pos::{SpanData}; use ty::maps::{QueryMsg}; use dep_graph::{DepNode}; +use proc_macro; +use lazy_static; // The name of the associated type for `Fn` return types pub const FN_OUTPUT_NAME: &'static str = "Output"; @@ -34,6 +37,24 @@ pub struct ErrorReported; thread_local!(static TIME_DEPTH: Cell = Cell::new(0)); +lazy_static! { + static ref DEFAULT_HOOK: Box = { + let hook = panic::take_hook(); + panic::set_hook(Box::new(panic_hook)); + hook + }; +} + +fn panic_hook(info: &panic::PanicInfo) { + if !proc_macro::__internal::in_sess() { + (*DEFAULT_HOOK)(info) + } +} + +pub fn install_panic_hook() { + lazy_static::initialize(&DEFAULT_HOOK); +} + /// Initialized for -Z profile-queries thread_local!(static PROFQ_CHAN: RefCell>> = RefCell::new(None)); diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index b8a1fe9910540..eb67c9ce4b7d8 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -24,7 +24,7 @@ use rustc::middle::cstore::CrateStore; use rustc::middle::privacy::AccessLevels; use rustc::ty::{self, TyCtxt, Resolutions, AllArenas}; use rustc::traits; -use rustc::util::common::{ErrorReported, time}; +use rustc::util::common::{ErrorReported, time, install_panic_hook}; use rustc_allocator as allocator; use rustc_borrowck as borrowck; use rustc_incremental; @@ -123,6 +123,8 @@ pub fn compile_input(trans: Box, let outputs = build_output_filenames(input, outdir, output, &krate.attrs, sess); let crate_name = ::rustc_trans_utils::link::find_crate_name(Some(sess), &krate.attrs, input); + install_panic_hook(); + let ExpansionResult { expanded_crate, defs, analysis, resolutions, mut hir_forest } = { phase_2_configure_and_expand( sess, diff --git a/src/test/ui-fulldeps/proc-macro/load-panic.rs b/src/test/ui-fulldeps/proc-macro/load-panic.rs index 328f398efd5c6..462eaf0341704 100644 --- a/src/test/ui-fulldeps/proc-macro/load-panic.rs +++ b/src/test/ui-fulldeps/proc-macro/load-panic.rs @@ -9,6 +9,7 @@ // except according to those terms. // aux-build:derive-panic.rs +// compile-flags:--error-format human #[macro_use] extern crate derive_panic; diff --git a/src/test/ui-fulldeps/proc-macro/load-panic.stderr b/src/test/ui-fulldeps/proc-macro/load-panic.stderr index 1be1609d45b2b..ab2ab84315a93 100644 --- a/src/test/ui-fulldeps/proc-macro/load-panic.stderr +++ b/src/test/ui-fulldeps/proc-macro/load-panic.stderr @@ -1,7 +1,7 @@ error: proc-macro derive panicked - --> $DIR/load-panic.rs:16:10 + --> $DIR/load-panic.rs:17:10 | -16 | #[derive(A)] +17 | #[derive(A)] | ^ | = help: message: nope! From 4d8b25183573e5ffbb254049702f3d2343a58919 Mon Sep 17 00:00:00 2001 From: toidiu Date: Thu, 22 Feb 2018 09:13:44 -0500 Subject: [PATCH 16/23] update tracking issue for nll Point to the new tracing issue for nll --- src/libsyntax/feature_gate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 3b137f9570a39..036984a4ebc32 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -186,7 +186,7 @@ declare_features! ( (active, rustc_attrs, "1.0.0", Some(29642)), // Allows the use of non lexical lifetimes; RFC 2094 - (active, nll, "1.0.0", Some(44928)), + (active, nll, "1.0.0", Some(43234)), // Allows the use of #[allow_internal_unstable]. This is an // attribute on macro_rules! and can't use the attribute handling From e88fe1d5199b8704342e80e8effcdfbdf3ca020a Mon Sep 17 00:00:00 2001 From: Anthony Deschamps Date: Thu, 22 Feb 2018 14:21:54 -0500 Subject: [PATCH 17/23] Small grammar fix to docs for String::new(). --- src/liballoc/string.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/string.rs b/src/liballoc/string.rs index 8d99d0bc8f4dc..409d2ab287e7c 100644 --- a/src/liballoc/string.rs +++ b/src/liballoc/string.rs @@ -364,7 +364,7 @@ impl String { /// /// Given that the `String` is empty, this will not allocate any initial /// buffer. While that means that this initial operation is very - /// inexpensive, but may cause excessive allocation later, when you add + /// inexpensive, it may cause excessive allocation later when you add /// data. If you have an idea of how much data the `String` will hold, /// consider the [`with_capacity`] method to prevent excessive /// re-allocation. From 311fbc9265ea1ebe803449c8d7e083f9edf605d9 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Thu, 22 Feb 2018 12:05:30 -0800 Subject: [PATCH 18/23] [docs] Minor wording changes to drain_filter docs The docs currently say, "If the closure returns false, it will try again, and call the closure on the next element." But this happens regardless of whether the closure returns true or false. --- src/liballoc/linked_list.rs | 4 ++-- src/liballoc/vec.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/liballoc/linked_list.rs b/src/liballoc/linked_list.rs index 65be087b35e5e..ec579e3fd68d6 100644 --- a/src/liballoc/linked_list.rs +++ b/src/liballoc/linked_list.rs @@ -747,8 +747,8 @@ impl LinkedList { /// Creates an iterator which uses a closure to determine if an element should be removed. /// /// If the closure returns true, then the element is removed and yielded. - /// If the closure returns false, it will try again, and call the closure on the next element, - /// seeing if it passes the test. + /// If the closure returns false, the element will remain in the list and will not be yielded + /// by the iterator. /// /// Note that `drain_filter` lets you mutate every element in the filter closure, regardless of /// whether you choose to keep or remove it. diff --git a/src/liballoc/vec.rs b/src/liballoc/vec.rs index 39a4d271bd6fe..3c9b6b94b4405 100644 --- a/src/liballoc/vec.rs +++ b/src/liballoc/vec.rs @@ -1966,8 +1966,8 @@ impl Vec { /// Creates an iterator which uses a closure to determine if an element should be removed. /// /// If the closure returns true, then the element is removed and yielded. - /// If the closure returns false, it will try again, and call the closure - /// on the next element, seeing if it passes the test. + /// If the closure returns false, the element will remain in the vector and will not be yielded + /// by the iterator. /// /// Using this method is equivalent to the following code: /// From e5d79c4bc7c46d8d85a18e71caa02bfb6a3c6df3 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Wed, 14 Feb 2018 20:30:49 -0300 Subject: [PATCH 19/23] Move word type and word size usage to constants & make it of 128 bits --- src/librustc_data_structures/bitvec.rs | 49 ++++++++++++++------------ 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index 80cdb0e441790..b565f5ebc7c0e 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -10,16 +10,19 @@ use std::iter::FromIterator; +type Word = u128; +const WORD_BITS: usize = 128; + /// A very simple BitVector type. #[derive(Clone, Debug, PartialEq)] pub struct BitVector { - data: Vec, + data: Vec, } impl BitVector { #[inline] pub fn new(num_bits: usize) -> BitVector { - let num_words = u64s(num_bits); + let num_words = words(num_bits); BitVector { data: vec![0; num_words] } } @@ -78,7 +81,7 @@ impl BitVector { #[inline] pub fn grow(&mut self, num_bits: usize) { - let num_words = u64s(num_bits); + let num_words = words(num_bits); if self.data.len() < num_words { self.data.resize(num_words, 0) } @@ -96,8 +99,8 @@ impl BitVector { } pub struct BitVectorIter<'a> { - iter: ::std::slice::Iter<'a, u64>, - current: u64, + iter: ::std::slice::Iter<'a, Word>, + current: Word, idx: usize, } @@ -107,10 +110,10 @@ impl<'a> Iterator for BitVectorIter<'a> { while self.current == 0 { self.current = if let Some(&i) = self.iter.next() { if i == 0 { - self.idx += 64; + self.idx += WORD_BITS; continue; } else { - self.idx = u64s(self.idx) * 64; + self.idx = words(self.idx) * WORD_BITS; i } } else { @@ -129,9 +132,9 @@ impl FromIterator for BitVector { fn from_iter(iter: I) -> BitVector where I: IntoIterator { let iter = iter.into_iter(); let (len, _) = iter.size_hint(); - // Make the minimum length for the bitvector 64 bits since that's + // Make the minimum length for the bitvector WORD_BITS bits since that's // the smallest non-zero size anyway. - let len = if len < 64 { 64 } else { len }; + let len = if len < WORD_BITS { WORD_BITS } else { len }; let mut bv = BitVector::new(len); for (idx, val) in iter.enumerate() { if idx > len { @@ -152,26 +155,26 @@ impl FromIterator for BitVector { #[derive(Clone, Debug)] pub struct BitMatrix { columns: usize, - vector: Vec, + vector: Vec, } impl BitMatrix { /// Create a new `rows x columns` matrix, initially empty. pub fn new(rows: usize, columns: usize) -> BitMatrix { // For every element, we need one bit for every other - // element. Round up to an even number of u64s. - let u64s_per_row = u64s(columns); + // element. Round up to an even number of words. + let words_per_row = words(columns); BitMatrix { columns, - vector: vec![0; rows * u64s_per_row], + vector: vec![0; rows * words_per_row], } } /// The range of bits for a given row. fn range(&self, row: usize) -> (usize, usize) { - let u64s_per_row = u64s(self.columns); - let start = row * u64s_per_row; - (start, start + u64s_per_row) + let words_per_row = words(self.columns); + let start = row * words_per_row; + (start, start + words_per_row) } /// Sets the cell at `(row, column)` to true. Put another way, add @@ -208,12 +211,12 @@ impl BitMatrix { let mut result = Vec::with_capacity(self.columns); for (base, (i, j)) in (a_start..a_end).zip(b_start..b_end).enumerate() { let mut v = self.vector[i] & self.vector[j]; - for bit in 0..64 { + for bit in 0..WORD_BITS { if v == 0 { break; } if v & 0x1 != 0 { - result.push(base * 64 + bit); + result.push(base * WORD_BITS + bit); } v >>= 1; } @@ -255,14 +258,14 @@ impl BitMatrix { } #[inline] -fn u64s(elements: usize) -> usize { - (elements + 63) / 64 +fn words(elements: usize) -> usize { + (elements + WORD_BITS - 1) / WORD_BITS } #[inline] -fn word_mask(index: usize) -> (usize, u64) { - let word = index / 64; - let mask = 1 << (index % 64); +fn word_mask(index: usize) -> (usize, Word) { + let word = index / WORD_BITS; + let mask = 1 << (index % WORD_BITS); (word, mask) } From ff9eb56c6e4bff6c1c74bd628a7e0dd79321829b Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Thu, 15 Feb 2018 15:47:40 -0300 Subject: [PATCH 20/23] Use Sparse bitsets instead of dense ones for NLL results Fixes #48170 --- src/librustc_data_structures/bitvec.rs | 197 ++++++++++++++++++ src/librustc_data_structures/indexed_vec.rs | 15 ++ .../borrow_check/nll/region_infer/values.rs | 23 +- 3 files changed, 224 insertions(+), 11 deletions(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index b565f5ebc7c0e..688e9ca9e0346 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -8,7 +8,11 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use std::collections::BTreeMap; +use std::collections::btree_map::Entry; +use std::marker::PhantomData; use std::iter::FromIterator; +use indexed_vec::{Idx, IndexVec}; type Word = u128; const WORD_BITS: usize = 128; @@ -257,6 +261,199 @@ impl BitMatrix { } } +#[derive(Clone, Debug)] +pub struct SparseBitMatrix where R: Idx, C: Idx { + vector: IndexVec>, +} + +impl SparseBitMatrix { + /// Create a new `rows x columns` matrix, initially empty. + pub fn new(rows: R, _columns: C) -> SparseBitMatrix { + SparseBitMatrix { + vector: IndexVec::from_elem_n(SparseBitSet::new(), rows.index()), + } + } + + /// Sets the cell at `(row, column)` to true. Put another way, insert + /// `column` to the bitset for `row`. + /// + /// Returns true if this changed the matrix, and false otherwise. + pub fn add(&mut self, row: R, column: C) -> bool { + self.vector[row].insert(column) + } + + /// Do the bits from `row` contain `column`? Put another way, is + /// the matrix cell at `(row, column)` true? Put yet another way, + /// if the matrix represents (transitive) reachability, can + /// `row` reach `column`? + pub fn contains(&self, row: R, column: C) -> bool { + self.vector[row].contains(column) + } + + /// Add the bits from row `read` to the bits from row `write`, + /// return true if anything changed. + /// + /// This is used when computing transitive reachability because if + /// you have an edge `write -> read`, because in that case + /// `write` can reach everything that `read` can (and + /// potentially more). + pub fn merge(&mut self, read: R, write: R) -> bool { + let mut changed = false; + + if read != write { + let (bit_set_read, bit_set_write) = self.vector.pick2_mut(read, write); + + for read_val in bit_set_read.iter() { + changed = changed | bit_set_write.insert(read_val); + } + } + + changed + } + + /// Iterates through all the columns set to true in a given row of + /// the matrix. + pub fn iter<'a>(&'a self, row: R) -> impl Iterator + 'a { + self.vector[row].iter() + } +} + +#[derive(Clone, Debug)] +pub struct SparseBitSet { + chunk_bits: BTreeMap, + _marker: PhantomData, +} + +#[derive(Copy, Clone)] +pub struct SparseChunk { + key: u32, + bits: Word, + _marker: PhantomData, +} + +impl SparseChunk { + pub fn one(index: I) -> Self { + let index = index.index(); + let key_usize = index / 128; + let key = key_usize as u32; + assert_eq!(key as usize, key_usize); + SparseChunk { + key, + bits: 1 << (index % 128), + _marker: PhantomData + } + } + + pub fn any(&self) -> bool { + self.bits != 0 + } + + pub fn iter(&self) -> impl Iterator { + let base = self.key as usize * 128; + let mut bits = self.bits; + (0..128).map(move |i| { + let current_bits = bits; + bits >>= 1; + (i, current_bits) + }).take_while(|&(_, bits)| bits != 0) + .filter_map(move |(i, bits)| { + if (bits & 1) != 0 { + Some(I::new(base + i)) + } else { + None + } + }) + } +} + +impl SparseBitSet { + pub fn new() -> Self { + SparseBitSet { + chunk_bits: BTreeMap::new(), + _marker: PhantomData + } + } + + pub fn capacity(&self) -> usize { + self.chunk_bits.len() * 128 + } + + pub fn contains_chunk(&self, chunk: SparseChunk) -> SparseChunk { + SparseChunk { + bits: self.chunk_bits.get(&chunk.key).map_or(0, |bits| bits & chunk.bits), + ..chunk + } + } + + pub fn insert_chunk(&mut self, chunk: SparseChunk) -> SparseChunk { + if chunk.bits == 0 { + return chunk; + } + let bits = self.chunk_bits.entry(chunk.key).or_insert(0); + let old_bits = *bits; + let new_bits = old_bits | chunk.bits; + *bits = new_bits; + let changed = new_bits ^ old_bits; + SparseChunk { + bits: changed, + ..chunk + } + } + + pub fn remove_chunk(&mut self, chunk: SparseChunk) -> SparseChunk { + if chunk.bits == 0 { + return chunk; + } + let changed = match self.chunk_bits.entry(chunk.key) { + Entry::Occupied(mut bits) => { + let old_bits = *bits.get(); + let new_bits = old_bits & !chunk.bits; + if new_bits == 0 { + bits.remove(); + } else { + bits.insert(new_bits); + } + new_bits ^ old_bits + } + Entry::Vacant(_) => 0 + }; + SparseChunk { + bits: changed, + ..chunk + } + } + + pub fn clear(&mut self) { + self.chunk_bits.clear(); + } + + pub fn chunks<'a>(&'a self) -> impl Iterator> + 'a { + self.chunk_bits.iter().map(|(&key, &bits)| { + SparseChunk { + key, + bits, + _marker: PhantomData + } + }) + } + + pub fn contains(&self, index: I) -> bool { + self.contains_chunk(SparseChunk::one(index)).any() + } + + pub fn insert(&mut self, index: I) -> bool { + self.insert_chunk(SparseChunk::one(index)).any() + } + + pub fn remove(&mut self, index: I) -> bool { + self.remove_chunk(SparseChunk::one(index)).any() + } + + pub fn iter<'a>(&'a self) -> impl Iterator + 'a { + self.chunks().flat_map(|chunk| chunk.iter()) + } +} + #[inline] fn words(elements: usize) -> usize { (elements + WORD_BITS - 1) / WORD_BITS diff --git a/src/librustc_data_structures/indexed_vec.rs b/src/librustc_data_structures/indexed_vec.rs index b11ca107af7dd..3e94b3f4d302a 100644 --- a/src/librustc_data_structures/indexed_vec.rs +++ b/src/librustc_data_structures/indexed_vec.rs @@ -482,6 +482,21 @@ impl IndexVec { pub fn get_mut(&mut self, index: I) -> Option<&mut T> { self.raw.get_mut(index.index()) } + + /// Return mutable references to two distinct elements, a and b. Panics if a == b. + #[inline] + pub fn pick2_mut(&mut self, a: I, b: I) -> (&mut T, &mut T) { + let (ai, bi) = (a.index(), b.index()); + assert!(ai != bi); + + if ai < bi { + let (c1, c2) = self.raw.split_at_mut(bi); + (&mut c1[ai], &mut c2[0]) + } else { + let (c2, c1) = self.pick2_mut(b, a); + (c1, c2) + } + } } impl IndexVec { diff --git a/src/librustc_mir/borrow_check/nll/region_infer/values.rs b/src/librustc_mir/borrow_check/nll/region_infer/values.rs index 45236bbc4aae2..be3d02be876c5 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/values.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/values.rs @@ -9,7 +9,7 @@ // except according to those terms. use std::rc::Rc; -use rustc_data_structures::bitvec::BitMatrix; +use rustc_data_structures::bitvec::SparseBitMatrix; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::indexed_vec::Idx; use rustc_data_structures::indexed_vec::IndexVec; @@ -132,7 +132,7 @@ impl RegionValueElements { } /// A newtype for the integers that represent one of the possible -/// elements in a region. These are the rows in the `BitMatrix` that +/// elements in a region. These are the rows in the `SparseBitMatrix` that /// is used to store the values of all regions. They have the following /// convention: /// @@ -184,18 +184,18 @@ impl ToElementIndex for RegionElementIndex { } /// Stores the values for a set of regions. These are stored in a -/// compact `BitMatrix` representation, with one row per region +/// compact `SparseBitMatrix` representation, with one row per region /// variable. The columns consist of either universal regions or /// points in the CFG. #[derive(Clone)] pub(super) struct RegionValues { elements: Rc, - matrix: BitMatrix, + matrix: SparseBitMatrix, /// If cause tracking is enabled, maps from a pair (r, e) /// consisting of a region `r` that contains some element `e` to /// the reason that the element is contained. There should be an - /// entry for every bit set to 1 in `BitMatrix`. + /// entry for every bit set to 1 in `SparseBitMatrix`. causes: Option, } @@ -214,7 +214,8 @@ impl RegionValues { Self { elements: elements.clone(), - matrix: BitMatrix::new(num_region_variables, elements.num_elements()), + matrix: SparseBitMatrix::new(RegionVid::new(num_region_variables), + RegionElementIndex::new(elements.num_elements())), causes: if track_causes.0 { Some(CauseMap::default()) } else { @@ -238,7 +239,7 @@ impl RegionValues { where F: FnOnce(&CauseMap) -> Cause, { - if self.matrix.add(r.index(), i.index()) { + if self.matrix.add(r, i) { debug!("add(r={:?}, i={:?})", r, self.elements.to_element(i)); if let Some(causes) = &mut self.causes { @@ -289,7 +290,7 @@ impl RegionValues { constraint_location: Location, constraint_span: Span, ) -> bool { - // We could optimize this by improving `BitMatrix::merge` so + // We could optimize this by improving `SparseBitMatrix::merge` so // it does not always merge an entire row. That would // complicate causal tracking though. debug!( @@ -315,7 +316,7 @@ impl RegionValues { /// True if the region `r` contains the given element. pub(super) fn contains(&self, r: RegionVid, elem: E) -> bool { let i = self.elements.index(elem); - self.matrix.contains(r.index(), i.index()) + self.matrix.contains(r, i) } /// Iterate over the value of the region `r`, yielding up element @@ -326,8 +327,8 @@ impl RegionValues { r: RegionVid, ) -> impl Iterator + 'a { self.matrix - .iter(r.index()) - .map(move |i| RegionElementIndex::new(i)) + .iter(r) + .map(move |i| i) } /// Returns just the universal regions that are contained in a given region's value. From aa3409c898b2418c927f3db1743a777fa05bb514 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Thu, 15 Feb 2018 19:35:11 -0300 Subject: [PATCH 21/23] Fix typo otherwies -> otherwise --- src/librustc_data_structures/bitvec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index 688e9ca9e0346..e13ccbbd89cfc 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -184,7 +184,7 @@ impl BitMatrix { /// Sets the cell at `(row, column)` to true. Put another way, add /// `column` to the bitset for `row`. /// - /// Returns true if this changed the matrix, and false otherwies. + /// Returns true if this changed the matrix, and false otherwise. pub fn add(&mut self, row: usize, column: usize) -> bool { let (start, _) = self.range(row); let (word, mask) = word_mask(column); From 6a74615fe3c81fe5cdbf02f9d4c19e235fab556c Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Thu, 22 Feb 2018 15:53:54 -0300 Subject: [PATCH 22/23] Run rustfmt over bitvec.rs and region_infer/values.rs --- src/librustc_data_structures/bitvec.rs | 76 +++++++++++-------- .../borrow_check/nll/region_infer/values.rs | 22 ++---- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index e13ccbbd89cfc..54565afa4c6c7 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -27,7 +27,9 @@ impl BitVector { #[inline] pub fn new(num_bits: usize) -> BitVector { let num_words = words(num_bits); - BitVector { data: vec![0; num_words] } + BitVector { + data: vec![0; num_words], + } } #[inline] @@ -133,7 +135,10 @@ impl<'a> Iterator for BitVectorIter<'a> { } impl FromIterator for BitVector { - fn from_iter(iter: I) -> BitVector where I: IntoIterator { + fn from_iter(iter: I) -> BitVector + where + I: IntoIterator, + { let iter = iter.into_iter(); let (len, _) = iter.size_hint(); // Make the minimum length for the bitvector WORD_BITS bits since that's @@ -262,7 +267,11 @@ impl BitMatrix { } #[derive(Clone, Debug)] -pub struct SparseBitMatrix where R: Idx, C: Idx { +pub struct SparseBitMatrix +where + R: Idx, + C: Idx, +{ vector: IndexVec>, } @@ -340,7 +349,7 @@ impl SparseChunk { SparseChunk { key, bits: 1 << (index % 128), - _marker: PhantomData + _marker: PhantomData, } } @@ -351,18 +360,20 @@ impl SparseChunk { pub fn iter(&self) -> impl Iterator { let base = self.key as usize * 128; let mut bits = self.bits; - (0..128).map(move |i| { - let current_bits = bits; - bits >>= 1; - (i, current_bits) - }).take_while(|&(_, bits)| bits != 0) - .filter_map(move |(i, bits)| { - if (bits & 1) != 0 { - Some(I::new(base + i)) - } else { - None - } - }) + (0..128) + .map(move |i| { + let current_bits = bits; + bits >>= 1; + (i, current_bits) + }) + .take_while(|&(_, bits)| bits != 0) + .filter_map(move |(i, bits)| { + if (bits & 1) != 0 { + Some(I::new(base + i)) + } else { + None + } + }) } } @@ -370,7 +381,7 @@ impl SparseBitSet { pub fn new() -> Self { SparseBitSet { chunk_bits: BTreeMap::new(), - _marker: PhantomData + _marker: PhantomData, } } @@ -380,7 +391,9 @@ impl SparseBitSet { pub fn contains_chunk(&self, chunk: SparseChunk) -> SparseChunk { SparseChunk { - bits: self.chunk_bits.get(&chunk.key).map_or(0, |bits| bits & chunk.bits), + bits: self.chunk_bits + .get(&chunk.key) + .map_or(0, |bits| bits & chunk.bits), ..chunk } } @@ -415,7 +428,7 @@ impl SparseBitSet { } new_bits ^ old_bits } - Entry::Vacant(_) => 0 + Entry::Vacant(_) => 0, }; SparseChunk { bits: changed, @@ -428,12 +441,10 @@ impl SparseBitSet { } pub fn chunks<'a>(&'a self) -> impl Iterator> + 'a { - self.chunk_bits.iter().map(|(&key, &bits)| { - SparseChunk { - key, - bits, - _marker: PhantomData - } + self.chunk_bits.iter().map(|(&key, &bits)| SparseChunk { + key, + bits, + _marker: PhantomData, }) } @@ -478,11 +489,12 @@ fn bitvec_iter_works() { bitvec.insert(65); bitvec.insert(66); bitvec.insert(99); - assert_eq!(bitvec.iter().collect::>(), - [1, 10, 19, 62, 63, 64, 65, 66, 99]); + assert_eq!( + bitvec.iter().collect::>(), + [1, 10, 19, 62, 63, 64, 65, 66, 99] + ); } - #[test] fn bitvec_iter_works_2() { let mut bitvec = BitVector::new(319); @@ -514,24 +526,24 @@ fn union_two_vecs() { #[test] fn grow() { let mut vec1 = BitVector::new(65); - for index in 0 .. 65 { + for index in 0..65 { assert!(vec1.insert(index)); assert!(!vec1.insert(index)); } vec1.grow(128); // Check if the bits set before growing are still set - for index in 0 .. 65 { + for index in 0..65 { assert!(vec1.contains(index)); } // Check if the new bits are all un-set - for index in 65 .. 128 { + for index in 65..128 { assert!(!vec1.contains(index)); } // Check that we can set all new bits without running out of bounds - for index in 65 .. 128 { + for index in 65..128 { assert!(vec1.insert(index)); assert!(!vec1.insert(index)); } diff --git a/src/librustc_mir/borrow_check/nll/region_infer/values.rs b/src/librustc_mir/borrow_check/nll/region_infer/values.rs index be3d02be876c5..e6f2a43bfc8f7 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/values.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/values.rs @@ -69,9 +69,7 @@ impl RegionValueElements { /// Iterates over the `RegionElementIndex` for all points in the CFG. pub(super) fn all_point_indices<'a>(&'a self) -> impl Iterator + 'a { - (0..self.num_points).map(move |i| { - RegionElementIndex::new(i + self.num_universal_regions) - }) + (0..self.num_points).map(move |i| RegionElementIndex::new(i + self.num_universal_regions)) } /// Iterates over the `RegionElementIndex` for all points in the CFG. @@ -154,7 +152,6 @@ pub(super) enum RegionElement { UniversalRegion(RegionVid), } - pub(super) trait ToElementIndex { fn to_element_index(self, elements: &RegionValueElements) -> RegionElementIndex; } @@ -214,8 +211,10 @@ impl RegionValues { Self { elements: elements.clone(), - matrix: SparseBitMatrix::new(RegionVid::new(num_region_variables), - RegionElementIndex::new(elements.num_elements())), + matrix: SparseBitMatrix::new( + RegionVid::new(num_region_variables), + RegionElementIndex::new(elements.num_elements()), + ), causes: if track_causes.0 { Some(CauseMap::default()) } else { @@ -295,8 +294,7 @@ impl RegionValues { // complicate causal tracking though. debug!( "add_universal_regions_outlived_by(from_region={:?}, to_region={:?})", - from_region, - to_region + from_region, to_region ); let mut changed = false; for elem in self.elements.all_universal_region_indices() { @@ -326,9 +324,7 @@ impl RegionValues { &'a self, r: RegionVid, ) -> impl Iterator + 'a { - self.matrix - .iter(r) - .map(move |i| i) + self.matrix.iter(r).map(move |i| i) } /// Returns just the universal regions that are contained in a given region's value. @@ -416,9 +412,7 @@ impl RegionValues { assert_eq!(location1.block, location2.block); str.push_str(&format!( "{:?}[{}..={}]", - location1.block, - location1.statement_index, - location2.statement_index + location1.block, location1.statement_index, location2.statement_index )); } } From 9e6d2d761e10e1db7e07a24a469527fde445fda6 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 23 Feb 2018 08:59:52 -0800 Subject: [PATCH 23/23] Update Clippy --- src/tools/clippy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/clippy b/src/tools/clippy index ce47e529d29f0..d5e233a720495 160000 --- a/src/tools/clippy +++ b/src/tools/clippy @@ -1 +1 @@ -Subproject commit ce47e529d29f0bf19b31ae80b37b467e42fb97e2 +Subproject commit d5e233a720495c52af25d8f6dcc9e55e1193beb9