From 8ba9b1019c6e6c514826c5466e84d93f665f975f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 16:24:58 -0400 Subject: [PATCH 01/21] Fix cycle error with existential types Fixes #61863 We now allow uses of 'existential type's that aren't defining uses - that is, uses which don't constrain the underlying concrete type. To make this work correctly, we also modify eq_opaque_type_and_type to not try to apply additional constraints to an opaque type. If we have code like this: ``` existential type Foo; fn foo1() -> Foo { ... } fn foo2() -> Foo { foo1() } ``` then 'foo2' doesn't end up constraining 'Foo', which means that 'foo2' will end up using the type 'Foo' internally - that is, an actual 'TyKind::Opaque'. We don't want to equate this to the underlying concrete type - we just need to enforce the basic equality constraint between the two types (here, the return type of 'foo1' and the return type of 'foo2') --- .../borrow_check/nll/type_check/mod.rs | 25 +++++++++- src/librustc_typeck/check/writeback.rs | 50 ++++++++++--------- src/test/run-pass/existential_type_const.rs | 17 +++++++ .../run-pass/existential_type_const.stderr | 6 +++ src/test/run-pass/existential_type_fns.rs | 25 ++++++++++ .../existential-types-with-cycle-error.rs | 2 +- .../existential-types-with-cycle-error.stderr | 24 +-------- .../existential-types-with-cycle-error2.rs | 2 +- ...existential-types-with-cycle-error2.stderr | 24 +-------- .../no_inferrable_concrete_type.rs | 6 +-- .../no_inferrable_concrete_type.stderr | 21 +------- 11 files changed, 105 insertions(+), 97 deletions(-) create mode 100644 src/test/run-pass/existential_type_const.rs create mode 100644 src/test/run-pass/existential_type_const.stderr create mode 100644 src/test/run-pass/existential_type_fns.rs diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index cdbbe1d02bd92..8bc377b401e87 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -1253,17 +1253,38 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { &anon_ty, locations.span(body), )); + + let revealed_ty_is_opaque = revealed_ty.is_impl_trait(); + debug!( "eq_opaque_type_and_type: \ instantiated output_ty={:?} \ opaque_type_map={:#?} \ - revealed_ty={:?}", - output_ty, opaque_type_map, revealed_ty + revealed_ty={:?} \ + revealed_ty_is_opaque={}", + output_ty, opaque_type_map, revealed_ty, revealed_ty_is_opaque ); obligations.add(infcx .at(&ObligationCause::dummy(), param_env) .eq(output_ty, revealed_ty)?); + // This is 'true' when we're using an existential + // type without 'revelaing' it. For example, code like this: + // + // existential type Foo: Debug; + // fn foo1() -> Foo { ... } + // fn foo2() -> Foo { foo1() } + // + // In 'foo2', we're not revealing the type of 'Foo' - we're + // just treating it as the opaque type. All of the constraints + // in our 'opaque_type_map' apply to the concrete type, + // not to the opaque type itself. Therefore, it's enough + // to simply equate the output and opque 'revealed_type', + // as we do above + if revealed_ty_is_opaque { + return Ok(InferOk { value: None, obligations: obligations.into_vec() }); + } + for (&opaque_def_id, opaque_decl) in &opaque_type_map { let opaque_defn_ty = tcx.type_of(opaque_def_id); let opaque_defn_ty = opaque_defn_ty.subst(tcx, opaque_decl.substs); diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index a2632b20c2ecb..14bd2f0fa7eb9 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -576,36 +576,38 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { }) }; + let mut skip_add = false; + if let ty::Opaque(defin_ty_def_id, _substs) = definition_ty.sty { if def_id == defin_ty_def_id { - // Concrete type resolved to the existential type itself. - // Force a cycle error. - // FIXME(oli-obk): we could just not insert it into `concrete_existential_types` - // which simply would make this use not a defining use. - self.tcx().at(span).type_of(defin_ty_def_id); + debug!("Skipping adding concrete definition for opaque type {:?} {:?}", + opaque_defn, defin_ty_def_id); + skip_add = true; } } if !opaque_defn.substs.has_local_value() { - let new = ty::ResolvedOpaqueTy { - concrete_type: definition_ty, - substs: opaque_defn.substs, - }; - - let old = self.tables - .concrete_existential_types - .insert(def_id, new); - if let Some(old) = old { - if old.concrete_type != definition_ty || old.substs != opaque_defn.substs { - span_bug!( - span, - "visit_opaque_types tried to write \ - different types for the same existential type: {:?}, {:?}, {:?}, {:?}", - def_id, - definition_ty, - opaque_defn, - old, - ); + if !skip_add { + let new = ty::ResolvedOpaqueTy { + concrete_type: definition_ty, + substs: opaque_defn.substs, + }; + + let old = self.tables + .concrete_existential_types + .insert(def_id, new); + if let Some(old) = old { + if old.concrete_type != definition_ty || old.substs != opaque_defn.substs { + span_bug!( + span, + "visit_opaque_types tried to write different types for the same \ + existential type: {:?}, {:?}, {:?}, {:?}", + def_id, + definition_ty, + opaque_defn, + old, + ); + } } } } else { diff --git a/src/test/run-pass/existential_type_const.rs b/src/test/run-pass/existential_type_const.rs new file mode 100644 index 0000000000000..333e15f3445bf --- /dev/null +++ b/src/test/run-pass/existential_type_const.rs @@ -0,0 +1,17 @@ +#![feature(existential_type)] +#![feature(impl_trait_in_bindings)] +//~^ WARN the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash + +// Ensures that consts can constrain an existential type + +use std::fmt::Debug; + +// Type `Foo` refers to a type that implements the `Debug` trait. +// The concrete type to which `Foo` refers is inferred from this module, +// and this concrete type is hidden from outer modules (but not submodules). +pub existential type Foo: Debug; + +const _FOO: Foo = 5; + +fn main() { +} diff --git a/src/test/run-pass/existential_type_const.stderr b/src/test/run-pass/existential_type_const.stderr new file mode 100644 index 0000000000000..b6d83fb170362 --- /dev/null +++ b/src/test/run-pass/existential_type_const.stderr @@ -0,0 +1,6 @@ +warning: the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash + --> $DIR/existential_type_const.rs:2:12 + | +LL | #![feature(impl_trait_in_bindings)] + | ^^^^^^^^^^^^^^^^^^^^^^ + diff --git a/src/test/run-pass/existential_type_fns.rs b/src/test/run-pass/existential_type_fns.rs new file mode 100644 index 0000000000000..e477dca9aad00 --- /dev/null +++ b/src/test/run-pass/existential_type_fns.rs @@ -0,0 +1,25 @@ +#![feature(existential_type)] + +// Regression test for issue #61863 + +pub trait MyTrait {} + +#[derive(Debug)] +pub struct MyStruct { + v: u64 +} + +impl MyTrait for MyStruct {} + +pub fn bla() -> TE { + return MyStruct {v:1} +} + +pub fn bla2() -> TE { + bla() +} + + +existential type TE: MyTrait; + +fn main() {} diff --git a/src/test/ui/existential_types/existential-types-with-cycle-error.rs b/src/test/ui/existential_types/existential-types-with-cycle-error.rs index 3f0190892ebb3..38fcabb5cc170 100644 --- a/src/test/ui/existential_types/existential-types-with-cycle-error.rs +++ b/src/test/ui/existential_types/existential-types-with-cycle-error.rs @@ -1,7 +1,7 @@ #![feature(existential_type)] existential type Foo: Fn() -> Foo; -//~^ ERROR: cycle detected when processing `Foo` +//~^ ERROR: could not find defining uses fn crash(x: Foo) -> Foo { x diff --git a/src/test/ui/existential_types/existential-types-with-cycle-error.stderr b/src/test/ui/existential_types/existential-types-with-cycle-error.stderr index 56057a9caa4a5..98a269d5271a2 100644 --- a/src/test/ui/existential_types/existential-types-with-cycle-error.stderr +++ b/src/test/ui/existential_types/existential-types-with-cycle-error.stderr @@ -1,30 +1,8 @@ -error[E0391]: cycle detected when processing `Foo` +error: could not find defining uses --> $DIR/existential-types-with-cycle-error.rs:3:1 | LL | existential type Foo: Fn() -> Foo; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -note: ...which requires processing `crash`... - --> $DIR/existential-types-with-cycle-error.rs:6:25 - | -LL | fn crash(x: Foo) -> Foo { - | _________________________^ -LL | | x -LL | | } - | |_^ - = note: ...which again requires processing `Foo`, completing the cycle -note: cycle used when collecting item types in top-level module - --> $DIR/existential-types-with-cycle-error.rs:1:1 - | -LL | / #![feature(existential_type)] -LL | | -LL | | existential type Foo: Fn() -> Foo; -LL | | -... | -LL | | -LL | | } - | |_^ error: aborting due to previous error -For more information about this error, try `rustc --explain E0391`. diff --git a/src/test/ui/existential_types/existential-types-with-cycle-error2.rs b/src/test/ui/existential_types/existential-types-with-cycle-error2.rs index 29410309ef26e..f9e6bdb67d4de 100644 --- a/src/test/ui/existential_types/existential-types-with-cycle-error2.rs +++ b/src/test/ui/existential_types/existential-types-with-cycle-error2.rs @@ -5,7 +5,7 @@ pub trait Bar { } existential type Foo: Bar; -//~^ ERROR: cycle detected when processing `Foo` +//~^ ERROR: could not find defining uses fn crash(x: Foo) -> Foo { x diff --git a/src/test/ui/existential_types/existential-types-with-cycle-error2.stderr b/src/test/ui/existential_types/existential-types-with-cycle-error2.stderr index 8c7bf52470ab2..830305d863119 100644 --- a/src/test/ui/existential_types/existential-types-with-cycle-error2.stderr +++ b/src/test/ui/existential_types/existential-types-with-cycle-error2.stderr @@ -1,30 +1,8 @@ -error[E0391]: cycle detected when processing `Foo` +error: could not find defining uses --> $DIR/existential-types-with-cycle-error2.rs:7:1 | LL | existential type Foo: Bar; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -note: ...which requires processing `crash`... - --> $DIR/existential-types-with-cycle-error2.rs:10:25 - | -LL | fn crash(x: Foo) -> Foo { - | _________________________^ -LL | | x -LL | | } - | |_^ - = note: ...which again requires processing `Foo`, completing the cycle -note: cycle used when collecting item types in top-level module - --> $DIR/existential-types-with-cycle-error2.rs:1:1 - | -LL | / #![feature(existential_type)] -LL | | -LL | | pub trait Bar { -LL | | type Item; -... | -LL | | -LL | | } - | |_^ error: aborting due to previous error -For more information about this error, try `rustc --explain E0391`. diff --git a/src/test/ui/existential_types/no_inferrable_concrete_type.rs b/src/test/ui/existential_types/no_inferrable_concrete_type.rs index 6bbe8bdc0cd6d..eec8a4be63d98 100644 --- a/src/test/ui/existential_types/no_inferrable_concrete_type.rs +++ b/src/test/ui/existential_types/no_inferrable_concrete_type.rs @@ -1,9 +1,9 @@ -// Issue 52985: Cause cycle error if user code provides no use case that allows an existential type -// to be inferred to a concrete type. This results in an infinite cycle during type normalization. +// Issue 52985: user code provides no use case that allows an existential type +// We now emit a 'could not find defining uses' error #![feature(existential_type)] -existential type Foo: Copy; //~ cycle detected +existential type Foo: Copy; //~ could not find defining uses // make compiler happy about using 'Foo' fn bar(x: Foo) -> Foo { x } diff --git a/src/test/ui/existential_types/no_inferrable_concrete_type.stderr b/src/test/ui/existential_types/no_inferrable_concrete_type.stderr index 4605332ef5b35..bc9a883c8365c 100644 --- a/src/test/ui/existential_types/no_inferrable_concrete_type.stderr +++ b/src/test/ui/existential_types/no_inferrable_concrete_type.stderr @@ -1,27 +1,8 @@ -error[E0391]: cycle detected when processing `Foo` +error: could not find defining uses --> $DIR/no_inferrable_concrete_type.rs:6:1 | LL | existential type Foo: Copy; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -note: ...which requires processing `bar`... - --> $DIR/no_inferrable_concrete_type.rs:9:23 - | -LL | fn bar(x: Foo) -> Foo { x } - | ^^^^^ - = note: ...which again requires processing `Foo`, completing the cycle -note: cycle used when collecting item types in top-level module - --> $DIR/no_inferrable_concrete_type.rs:4:1 - | -LL | / #![feature(existential_type)] -LL | | -LL | | existential type Foo: Copy; -LL | | -... | -LL | | let _: Foo = std::mem::transmute(0u8); -LL | | } - | |_^ error: aborting due to previous error -For more information about this error, try `rustc --explain E0391`. From ec626992fe40ed7752145955d2a2e60228335987 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 18:45:56 -0400 Subject: [PATCH 02/21] Fix bug when opaque type was nested in another type. Previously, types like (Foo, u8) would not be handled correctly (where Foo is an 'existential type') --- .../borrow_check/nll/type_check/mod.rs | 63 ++++++++++--------- src/test/run-pass/existential_type_tuple.rs | 32 ++++++++++ 2 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 src/test/run-pass/existential_type_tuple.rs diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index 8bc377b401e87..a505750b1a12d 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -1253,51 +1253,58 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { &anon_ty, locations.span(body), )); - - let revealed_ty_is_opaque = revealed_ty.is_impl_trait(); - debug!( "eq_opaque_type_and_type: \ instantiated output_ty={:?} \ opaque_type_map={:#?} \ - revealed_ty={:?} \ - revealed_ty_is_opaque={}", - output_ty, opaque_type_map, revealed_ty, revealed_ty_is_opaque + revealed_ty={:?}", + output_ty, opaque_type_map, revealed_ty ); obligations.add(infcx .at(&ObligationCause::dummy(), param_env) .eq(output_ty, revealed_ty)?); - // This is 'true' when we're using an existential - // type without 'revelaing' it. For example, code like this: - // - // existential type Foo: Debug; - // fn foo1() -> Foo { ... } - // fn foo2() -> Foo { foo1() } - // - // In 'foo2', we're not revealing the type of 'Foo' - we're - // just treating it as the opaque type. All of the constraints - // in our 'opaque_type_map' apply to the concrete type, - // not to the opaque type itself. Therefore, it's enough - // to simply equate the output and opque 'revealed_type', - // as we do above - if revealed_ty_is_opaque { - return Ok(InferOk { value: None, obligations: obligations.into_vec() }); - } - for (&opaque_def_id, opaque_decl) in &opaque_type_map { let opaque_defn_ty = tcx.type_of(opaque_def_id); let opaque_defn_ty = opaque_defn_ty.subst(tcx, opaque_decl.substs); let opaque_defn_ty = renumber::renumber_regions(infcx, &opaque_defn_ty); + let concrete_is_opaque = infcx + .resolve_vars_if_possible(&opaque_decl.concrete_ty).is_impl_trait(); + debug!( - "eq_opaque_type_and_type: concrete_ty={:?}={:?} opaque_defn_ty={:?}", + "eq_opaque_type_and_type: concrete_ty={:?}={:?} opaque_defn_ty={:?} \ + concrete_is_opaque={}", opaque_decl.concrete_ty, infcx.resolve_vars_if_possible(&opaque_decl.concrete_ty), - opaque_defn_ty + opaque_defn_ty, + concrete_is_opaque ); - obligations.add(infcx - .at(&ObligationCause::dummy(), param_env) - .eq(opaque_decl.concrete_ty, opaque_defn_ty)?); + + // concrete_is_opaque is 'true' when we're using an existential + // type without 'revelaing' it. For example, code like this: + // + // existential type Foo: Debug; + // fn foo1() -> Foo { ... } + // fn foo2() -> Foo { foo1() } + // + // In 'foo2', we're not revealing the type of 'Foo' - we're + // just treating it as the opaque type. + // + // When this occurs, we do *not* want to try to equate + // the concrete type with the underlying defining type + // of the existential type - this will always fail, since + // the defining type of an existential type is always + // some other type (e.g. not itself) + // Essentially, none of the normal obligations apply here - + // we're just passing around some unknown opaque type, + // without actually looking at the underlying type it + // gets 'revealed' into + + if !concrete_is_opaque { + obligations.add(infcx + .at(&ObligationCause::dummy(), param_env) + .eq(opaque_decl.concrete_ty, opaque_defn_ty)?); + } } debug!("eq_opaque_type_and_type: equated"); diff --git a/src/test/run-pass/existential_type_tuple.rs b/src/test/run-pass/existential_type_tuple.rs new file mode 100644 index 0000000000000..31c145ea89a33 --- /dev/null +++ b/src/test/run-pass/existential_type_tuple.rs @@ -0,0 +1,32 @@ +#![feature(existential_type)] + +#![allow(dead_code)] + +pub trait MyTrait {} + +impl MyTrait for bool {} + +struct Blah { + my_foo: Foo, + my_u8: u8 +} + +impl Blah { + fn new() -> Blah { + Blah { + my_foo: make_foo(), + my_u8: 12 + } + } + fn into_inner(self) -> (Foo, u8) { + (self.my_foo, self.my_u8) + } +} + +fn make_foo() -> Foo { + true +} + +existential type Foo: MyTrait; + +fn main() {} From 2f051605f3f5bacfbb66ece3ecfcacd048d38b5b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 18:53:34 -0400 Subject: [PATCH 03/21] Remove unecessary doc comment --- src/test/run-pass/existential_type_const.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/run-pass/existential_type_const.rs b/src/test/run-pass/existential_type_const.rs index 333e15f3445bf..f8b66167967c3 100644 --- a/src/test/run-pass/existential_type_const.rs +++ b/src/test/run-pass/existential_type_const.rs @@ -6,9 +6,6 @@ use std::fmt::Debug; -// Type `Foo` refers to a type that implements the `Debug` trait. -// The concrete type to which `Foo` refers is inferred from this module, -// and this concrete type is hidden from outer modules (but not submodules). pub existential type Foo: Debug; const _FOO: Foo = 5; From 2ab8d618823a51f347407cf7299110ad42f26616 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 18:59:11 -0400 Subject: [PATCH 04/21] s/consts/`const` items/ Co-Authored-By: Mazdak Farrokhzad --- src/test/run-pass/existential_type_const.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/run-pass/existential_type_const.rs b/src/test/run-pass/existential_type_const.rs index f8b66167967c3..1f80475cb7711 100644 --- a/src/test/run-pass/existential_type_const.rs +++ b/src/test/run-pass/existential_type_const.rs @@ -2,7 +2,7 @@ #![feature(impl_trait_in_bindings)] //~^ WARN the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash -// Ensures that consts can constrain an existential type +// Ensures that `const` items can constrain an `existential type`. use std::fmt::Debug; From 36008327cc3214afaf5ce5bacdf2fdf236422caf Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 19:10:29 -0400 Subject: [PATCH 05/21] Address review comments --- src/librustc_mir/borrow_check/nll/type_check/mod.rs | 6 +++--- src/librustc_typeck/check/writeback.rs | 5 +++++ .../existential_types}/existential_type_const.rs | 2 ++ .../existential_types}/existential_type_const.stderr | 0 .../existential_types}/existential_type_fns.rs | 2 ++ .../existential_types}/existential_type_tuple.rs | 3 ++- 6 files changed, 14 insertions(+), 4 deletions(-) rename src/test/{run-pass => ui/existential_types}/existential_type_const.rs (95%) rename src/test/{run-pass => ui/existential_types}/existential_type_const.stderr (100%) rename src/test/{run-pass => ui/existential_types}/existential_type_fns.rs (95%) rename src/test/{run-pass => ui/existential_types}/existential_type_tuple.rs (96%) diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index a505750b1a12d..1194872c0afd4 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -1280,14 +1280,14 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { concrete_is_opaque ); - // concrete_is_opaque is 'true' when we're using an existential - // type without 'revelaing' it. For example, code like this: + // concrete_is_opaque is `true` when we're using an existential + // type without 'revealing' it. For example, code like this: // // existential type Foo: Debug; // fn foo1() -> Foo { ... } // fn foo2() -> Foo { foo1() } // - // In 'foo2', we're not revealing the type of 'Foo' - we're + // In `foo2`, we're not revealing the type of `Foo` - we're // just treating it as the opaque type. // // When this occurs, we do *not* want to try to equate diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 14bd2f0fa7eb9..831f9daa52d91 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -587,6 +587,11 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { } if !opaque_defn.substs.has_local_value() { + // We only want to add an entry into `concrete_existential_types` + // if we actually found a defining usage of this existential type. + // Otherwise, we do nothing - we'll either find a defining usage + // in some other location, or we'll end up emitting an error due + // to the lack of defining usage if !skip_add { let new = ty::ResolvedOpaqueTy { concrete_type: definition_ty, diff --git a/src/test/run-pass/existential_type_const.rs b/src/test/ui/existential_types/existential_type_const.rs similarity index 95% rename from src/test/run-pass/existential_type_const.rs rename to src/test/ui/existential_types/existential_type_const.rs index 1f80475cb7711..55920b85dd730 100644 --- a/src/test/run-pass/existential_type_const.rs +++ b/src/test/ui/existential_types/existential_type_const.rs @@ -1,3 +1,5 @@ +// check-pass + #![feature(existential_type)] #![feature(impl_trait_in_bindings)] //~^ WARN the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash diff --git a/src/test/run-pass/existential_type_const.stderr b/src/test/ui/existential_types/existential_type_const.stderr similarity index 100% rename from src/test/run-pass/existential_type_const.stderr rename to src/test/ui/existential_types/existential_type_const.stderr diff --git a/src/test/run-pass/existential_type_fns.rs b/src/test/ui/existential_types/existential_type_fns.rs similarity index 95% rename from src/test/run-pass/existential_type_fns.rs rename to src/test/ui/existential_types/existential_type_fns.rs index e477dca9aad00..6f22eef284985 100644 --- a/src/test/run-pass/existential_type_fns.rs +++ b/src/test/ui/existential_types/existential_type_fns.rs @@ -1,3 +1,5 @@ +// check-pass + #![feature(existential_type)] // Regression test for issue #61863 diff --git a/src/test/run-pass/existential_type_tuple.rs b/src/test/ui/existential_types/existential_type_tuple.rs similarity index 96% rename from src/test/run-pass/existential_type_tuple.rs rename to src/test/ui/existential_types/existential_type_tuple.rs index 31c145ea89a33..0f134a528979c 100644 --- a/src/test/run-pass/existential_type_tuple.rs +++ b/src/test/ui/existential_types/existential_type_tuple.rs @@ -1,5 +1,6 @@ -#![feature(existential_type)] +// check-pass +#![feature(existential_type)] #![allow(dead_code)] pub trait MyTrait {} From 66b2b97e0774f50f9ba4de3c8806559ce787fe37 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 20:06:41 -0400 Subject: [PATCH 06/21] Fix test stderr --- src/test/ui/existential_types/existential_type_const.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/existential_types/existential_type_const.stderr b/src/test/ui/existential_types/existential_type_const.stderr index b6d83fb170362..3499b6e20d567 100644 --- a/src/test/ui/existential_types/existential_type_const.stderr +++ b/src/test/ui/existential_types/existential_type_const.stderr @@ -1,5 +1,5 @@ warning: the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash - --> $DIR/existential_type_const.rs:2:12 + --> $DIR/existential_type_const.rs:4:12 | LL | #![feature(impl_trait_in_bindings)] | ^^^^^^^^^^^^^^^^^^^^^^ From 2f4196205336df8550a4bfb3045d3d1c350f02bf Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 5 Jul 2019 20:15:31 -0400 Subject: [PATCH 07/21] Add explanation to 'existential_type_const' test --- src/test/ui/existential_types/existential_type_const.rs | 4 ++++ src/test/ui/existential_types/existential_type_const.stderr | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/test/ui/existential_types/existential_type_const.rs b/src/test/ui/existential_types/existential_type_const.rs index 55920b85dd730..646e9a734244e 100644 --- a/src/test/ui/existential_types/existential_type_const.rs +++ b/src/test/ui/existential_types/existential_type_const.rs @@ -1,6 +1,10 @@ // check-pass #![feature(existential_type)] +// Currently, the `existential_type` feature implicitly +// depends on `impl_trait_in_bindings` in order to work properly. +// Specifically, this line requires `impl_trait_in_bindings` to be enabled: +// https://github.com/rust-lang/rust/blob/481068a707679257e2a738b40987246e0420e787/src/librustc_typeck/check/mod.rs#L856 #![feature(impl_trait_in_bindings)] //~^ WARN the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash diff --git a/src/test/ui/existential_types/existential_type_const.stderr b/src/test/ui/existential_types/existential_type_const.stderr index 3499b6e20d567..049b4f75dd204 100644 --- a/src/test/ui/existential_types/existential_type_const.stderr +++ b/src/test/ui/existential_types/existential_type_const.stderr @@ -1,5 +1,5 @@ warning: the feature `impl_trait_in_bindings` is incomplete and may cause the compiler to crash - --> $DIR/existential_type_const.rs:4:12 + --> $DIR/existential_type_const.rs:8:12 | LL | #![feature(impl_trait_in_bindings)] | ^^^^^^^^^^^^^^^^^^^^^^ From 44bf6b6f55be89926d9f18b4d90a9eaefb84d02c Mon Sep 17 00:00:00 2001 From: David Wood Date: Sat, 20 Jul 2019 10:49:02 +0100 Subject: [PATCH 08/21] tests: Add minimal reproduction of #61963. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a reproduction of the error reported in servo which demonstrates the current, incorrect behaviour. Co-authored-by: Rémy Rakić --- .../ui/suggestions/auxiliary/issue-61963-1.rs | 40 ++++++++++++++++++ .../ui/suggestions/auxiliary/issue-61963.rs | 41 +++++++++++++++++++ src/test/ui/suggestions/issue-61963.rs | 25 +++++++++++ src/test/ui/suggestions/issue-61963.stderr | 20 +++++++++ 4 files changed, 126 insertions(+) create mode 100644 src/test/ui/suggestions/auxiliary/issue-61963-1.rs create mode 100644 src/test/ui/suggestions/auxiliary/issue-61963.rs create mode 100644 src/test/ui/suggestions/issue-61963.rs create mode 100644 src/test/ui/suggestions/issue-61963.stderr diff --git a/src/test/ui/suggestions/auxiliary/issue-61963-1.rs b/src/test/ui/suggestions/auxiliary/issue-61963-1.rs new file mode 100644 index 0000000000000..6c2df7e84e07c --- /dev/null +++ b/src/test/ui/suggestions/auxiliary/issue-61963-1.rs @@ -0,0 +1,40 @@ +// force-host +// no-prefer-dynamic +#![crate_type = "proc-macro"] + +extern crate proc_macro; + +use proc_macro::{Group, TokenStream, TokenTree}; + +// This macro exists as part of a reproduction of #61963 but without using quote/syn/proc_macro2. + +#[proc_macro_derive(DomObject)] +pub fn expand_token_stream(input: TokenStream) -> TokenStream { + // Construct a dummy span - `#0 bytes(0..0)` - which is present in the input because + // of the specially crafted generated tokens in the `attribute-crate` proc-macro. + let dummy_span = input.clone().into_iter().nth(0).unwrap().span(); + + // Define what the macro would output if constructed properly from the source using syn/quote. + let output: TokenStream = "impl Bar for ((), Qux >) { } + impl Bar for ((), Box) { }".parse().unwrap(); + + let mut tokens: Vec<_> = output.into_iter().collect(); + // Adjust token spans to match the original crate (which would use `quote`). Some of the + // generated tokens point to the dummy span. + for token in tokens.iter_mut() { + if let TokenTree::Group(group) = token { + let mut tokens: Vec<_> = group.stream().into_iter().collect(); + for token in tokens.iter_mut().skip(2) { + token.set_span(dummy_span); + } + + let mut stream = TokenStream::new(); + stream.extend(tokens); + *group = Group::new(group.delimiter(), stream); + } + } + + let mut output = TokenStream::new(); + output.extend(tokens); + output +} diff --git a/src/test/ui/suggestions/auxiliary/issue-61963.rs b/src/test/ui/suggestions/auxiliary/issue-61963.rs new file mode 100644 index 0000000000000..e86f1610ab0d0 --- /dev/null +++ b/src/test/ui/suggestions/auxiliary/issue-61963.rs @@ -0,0 +1,41 @@ +// force-host +// no-prefer-dynamic +#![crate_type = "proc-macro"] + +extern crate proc_macro; + +use proc_macro::{Group, Spacing, Punct, TokenTree, TokenStream}; + +// This macro exists as part of a reproduction of #61963 but without using quote/syn/proc_macro2. + +#[proc_macro_attribute] +pub fn dom_struct(_: TokenStream, input: TokenStream) -> TokenStream { + // Construct the expected output tokens - the input but with a `#[derive(DomObject)]` applied. + let attributes: TokenStream = + "#[derive(DomObject)]".to_string().parse().unwrap(); + let output: TokenStream = attributes.into_iter() + .chain(input.into_iter()).collect(); + + let mut tokens: Vec<_> = output.into_iter().collect(); + // Adjust the spacing of `>` tokens to match what `quote` would produce. + for token in tokens.iter_mut() { + if let TokenTree::Group(group) = token { + let mut tokens: Vec<_> = group.stream().into_iter().collect(); + for token in tokens.iter_mut() { + if let TokenTree::Punct(p) = token { + if p.as_char() == '>' { + *p = Punct::new('>', Spacing::Alone); + } + } + } + + let mut stream = TokenStream::new(); + stream.extend(tokens); + *group = Group::new(group.delimiter(), stream); + } + } + + let mut output = TokenStream::new(); + output.extend(tokens); + output +} diff --git a/src/test/ui/suggestions/issue-61963.rs b/src/test/ui/suggestions/issue-61963.rs new file mode 100644 index 0000000000000..540a7c42d2fd3 --- /dev/null +++ b/src/test/ui/suggestions/issue-61963.rs @@ -0,0 +1,25 @@ +// aux-build:issue-61963.rs +// aux-build:issue-61963-1.rs +#![deny(bare_trait_objects)] + +#[macro_use] +extern crate issue_61963; +#[macro_use] +extern crate issue_61963_1; + +// This test checks that the bare trait object lint does not trigger on macro attributes that +// generate code which would trigger the lint. + +pub struct Baz; +pub trait Bar { } +pub struct Qux(T); + +#[dom_struct] +//~^ ERROR trait objects without an explicit `dyn` are deprecated [bare_trait_objects] +pub struct Foo { + qux: Qux>, + bar: Box, + //~^ ERROR trait objects without an explicit `dyn` are deprecated [bare_trait_objects] +} + +fn main() {} diff --git a/src/test/ui/suggestions/issue-61963.stderr b/src/test/ui/suggestions/issue-61963.stderr new file mode 100644 index 0000000000000..261b384ca5716 --- /dev/null +++ b/src/test/ui/suggestions/issue-61963.stderr @@ -0,0 +1,20 @@ +error: trait objects without an explicit `dyn` are deprecated + --> $DIR/issue-61963.rs:21:14 + | +LL | bar: Box, + | ^^^ help: use `dyn`: `dyn Bar` + | +note: lint level defined here + --> $DIR/issue-61963.rs:3:9 + | +LL | #![deny(bare_trait_objects)] + | ^^^^^^^^^^^^^^^^^^ + +error: trait objects without an explicit `dyn` are deprecated + --> $DIR/issue-61963.rs:17:1 + | +LL | #[dom_struct] + | ^^^^^^^^^^^^^ help: use `dyn`: `dyn #[dom_struct]` + +error: aborting due to 2 previous errors + From eb4fbda1f2893fe6ec0f917722b3bc853704794c Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Thu, 25 Jul 2019 12:24:48 -0400 Subject: [PATCH 09/21] Simplify save-analysis JSON dumper interface --- src/librustc_save_analysis/dump_visitor.rs | 18 +++---- src/librustc_save_analysis/json_dumper.rs | 59 +++------------------- src/librustc_save_analysis/lib.rs | 24 +++++---- 3 files changed, 29 insertions(+), 72 deletions(-) diff --git a/src/librustc_save_analysis/dump_visitor.rs b/src/librustc_save_analysis/dump_visitor.rs index dfdf560d41906..fed0764ba9aff 100644 --- a/src/librustc_save_analysis/dump_visitor.rs +++ b/src/librustc_save_analysis/dump_visitor.rs @@ -38,7 +38,7 @@ use syntax_pos::*; use crate::{escape, generated_code, id_from_def_id, id_from_node_id, lower_attributes, PathCollector, SaveContext}; -use crate::json_dumper::{Access, DumpOutput, JsonDumper}; +use crate::json_dumper::{Access, JsonDumper}; use crate::span_utils::SpanUtils; use crate::sig; @@ -75,10 +75,10 @@ macro_rules! access_from_vis { }; } -pub struct DumpVisitor<'l, 'tcx, 'll, O: DumpOutput> { +pub struct DumpVisitor<'l, 'tcx, 'll> { save_ctxt: SaveContext<'l, 'tcx>, tcx: TyCtxt<'tcx>, - dumper: &'ll mut JsonDumper, + dumper: &'ll mut JsonDumper, span: SpanUtils<'l>, @@ -92,11 +92,11 @@ pub struct DumpVisitor<'l, 'tcx, 'll, O: DumpOutput> { // macro_calls: FxHashSet, } -impl<'l, 'tcx, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> { +impl<'l, 'tcx, 'll> DumpVisitor<'l, 'tcx, 'll> { pub fn new( save_ctxt: SaveContext<'l, 'tcx>, - dumper: &'ll mut JsonDumper, - ) -> DumpVisitor<'l, 'tcx, 'll, O> { + dumper: &'ll mut JsonDumper, + ) -> DumpVisitor<'l, 'tcx, 'll> { let span_utils = SpanUtils::new(&save_ctxt.tcx.sess); DumpVisitor { tcx: save_ctxt.tcx, @@ -111,7 +111,7 @@ impl<'l, 'tcx, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> { fn nest_scope(&mut self, scope_id: NodeId, f: F) where - F: FnOnce(&mut DumpVisitor<'l, 'tcx, 'll, O>), + F: FnOnce(&mut DumpVisitor<'l, 'tcx, 'll>), { let parent_scope = self.cur_scope; self.cur_scope = scope_id; @@ -121,7 +121,7 @@ impl<'l, 'tcx, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> { fn nest_tables(&mut self, item_id: NodeId, f: F) where - F: FnOnce(&mut DumpVisitor<'l, 'tcx, 'll, O>), + F: FnOnce(&mut DumpVisitor<'l, 'tcx, 'll>), { let item_def_id = self.tcx.hir().local_def_id_from_node_id(item_id); if self.tcx.has_typeck_tables(item_def_id) { @@ -1311,7 +1311,7 @@ impl<'l, 'tcx, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> { } } -impl<'l, 'tcx, 'll, O: DumpOutput + 'll> Visitor<'l> for DumpVisitor<'l, 'tcx, 'll, O> { +impl<'l, 'tcx, 'll> Visitor<'l> for DumpVisitor<'l, 'tcx, 'll> { fn visit_mod(&mut self, m: &'l ast::Mod, span: Span, attrs: &[ast::Attribute], id: NodeId) { // Since we handle explicit modules ourselves in visit_item, this should // only get called for the root module of a crate. diff --git a/src/librustc_save_analysis/json_dumper.rs b/src/librustc_save_analysis/json_dumper.rs index 82b78369e1339..c1437a27e5d22 100644 --- a/src/librustc_save_analysis/json_dumper.rs +++ b/src/librustc_save_analysis/json_dumper.rs @@ -1,80 +1,33 @@ -use std::io::Write; - use rls_data::config::Config; use rls_data::{self, Analysis, CompilationOptions, CratePreludeData, Def, DefKind, Impl, Import, MacroRef, Ref, RefKind, Relation}; use rls_span::{Column, Row}; -use log::error; - #[derive(Debug)] pub struct Access { pub reachable: bool, pub public: bool, } -pub struct JsonDumper { +pub struct JsonDumper { result: Analysis, config: Config, - output: O, -} - -pub trait DumpOutput { - fn dump(&mut self, result: &Analysis); -} - -pub struct WriteOutput<'b, W: Write> { - output: &'b mut W, -} - -impl<'b, W: Write> DumpOutput for WriteOutput<'b, W> { - fn dump(&mut self, result: &Analysis) { - if let Err(e) = serde_json::to_writer(self.output.by_ref(), result) { - error!("Can't serialize save-analysis: {:?}", e); - } - } -} - -pub struct CallbackOutput<'b> { - callback: &'b mut dyn FnMut(&Analysis), } -impl<'b> DumpOutput for CallbackOutput<'b> { - fn dump(&mut self, result: &Analysis) { - (self.callback)(result) - } -} - -impl<'b, W: Write> JsonDumper> { - pub fn new(writer: &'b mut W, config: Config) -> JsonDumper> { +impl JsonDumper { + pub fn new(config: Config) -> JsonDumper { JsonDumper { - output: WriteOutput { output: writer }, config: config.clone(), result: Analysis::new(config), } } -} - -impl<'b> JsonDumper> { - pub fn with_callback( - callback: &'b mut dyn FnMut(&Analysis), - config: Config, - ) -> JsonDumper> { - JsonDumper { - output: CallbackOutput { callback }, - config: config.clone(), - result: Analysis::new(config), - } - } -} -impl Drop for JsonDumper { - fn drop(&mut self) { - self.output.dump(&self.result); + pub fn to_output(self, f: impl FnOnce(&Analysis)) { + f(&self.result) } } -impl<'b, O: DumpOutput + 'b> JsonDumper { +impl JsonDumper { pub fn crate_prelude(&mut self, data: CratePreludeData) { self.result.prelude = Some(data) } diff --git a/src/librustc_save_analysis/lib.rs b/src/librustc_save_analysis/lib.rs index c987a46b56737..edaf4c7df67b6 100644 --- a/src/librustc_save_analysis/lib.rs +++ b/src/librustc_save_analysis/lib.rs @@ -1075,17 +1075,19 @@ impl<'a> SaveHandler for DumpHandler<'a> { input: &'l Input, ) { let sess = &save_ctxt.tcx.sess; - let file_name = { - let (mut output, file_name) = self.output_file(&save_ctxt); - let mut dumper = JsonDumper::new(&mut output, save_ctxt.config.clone()); - let mut visitor = DumpVisitor::new(save_ctxt, &mut dumper); + let (output, file_name) = self.output_file(&save_ctxt); + let mut dumper = JsonDumper::new(save_ctxt.config.clone()); + let mut visitor = DumpVisitor::new(save_ctxt, &mut dumper); - visitor.dump_crate_info(cratename, krate); - visitor.dump_compilation_options(input, cratename); - visit::walk_crate(&mut visitor, krate); + visitor.dump_crate_info(cratename, krate); + visitor.dump_compilation_options(input, cratename); + visit::walk_crate(&mut visitor, krate); - file_name - }; + dumper.to_output(|analysis| { + if let Err(e) = serde_json::to_writer(output, analysis) { + error!("Can't serialize save-analysis: {:?}", e); + } + }); if sess.opts.debugging_opts.emit_artifact_notifications { sess.parse_sess.span_diagnostic @@ -1112,12 +1114,14 @@ impl<'b> SaveHandler for CallbackHandler<'b> { // using the JsonDumper to collect the save-analysis results, but not // actually to dump them to a file. This is all a bit convoluted and // there is certainly a simpler design here trying to get out (FIXME). - let mut dumper = JsonDumper::with_callback(self.callback, save_ctxt.config.clone()); + let mut dumper = JsonDumper::new(save_ctxt.config.clone()); let mut visitor = DumpVisitor::new(save_ctxt, &mut dumper); visitor.dump_crate_info(cratename, krate); visitor.dump_compilation_options(input, cratename); visit::walk_crate(&mut visitor, krate); + + dumper.to_output(|a| (self.callback)(a)) } } From 68c0ba284d3729a02392d7379673fb196c4a3711 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Thu, 25 Jul 2019 12:26:28 -0400 Subject: [PATCH 10/21] Rename JsonDumper to Dumper The Dumper no longer has anything to do specifically with JSON, it merely represents processing into an `Analysis` output. --- src/librustc_save_analysis/dump_visitor.rs | 8 ++++---- .../{json_dumper.rs => dumper.rs} | 10 +++++----- src/librustc_save_analysis/lib.rs | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) rename src/librustc_save_analysis/{json_dumper.rs => dumper.rs} (95%) diff --git a/src/librustc_save_analysis/dump_visitor.rs b/src/librustc_save_analysis/dump_visitor.rs index fed0764ba9aff..2b349613dc54f 100644 --- a/src/librustc_save_analysis/dump_visitor.rs +++ b/src/librustc_save_analysis/dump_visitor.rs @@ -10,7 +10,7 @@ //! //! SpanUtils is used to manipulate spans. In particular, to extract sub-spans //! from spans (e.g., the span for `bar` from the above example path). -//! DumpVisitor walks the AST and processes it, and JsonDumper is used for +//! DumpVisitor walks the AST and processes it, and Dumper is used for //! recording the output. use rustc::hir::def::{Res, DefKind as HirDefKind}; @@ -38,7 +38,7 @@ use syntax_pos::*; use crate::{escape, generated_code, id_from_def_id, id_from_node_id, lower_attributes, PathCollector, SaveContext}; -use crate::json_dumper::{Access, JsonDumper}; +use crate::dumper::{Access, Dumper}; use crate::span_utils::SpanUtils; use crate::sig; @@ -78,7 +78,7 @@ macro_rules! access_from_vis { pub struct DumpVisitor<'l, 'tcx, 'll> { save_ctxt: SaveContext<'l, 'tcx>, tcx: TyCtxt<'tcx>, - dumper: &'ll mut JsonDumper, + dumper: &'ll mut Dumper, span: SpanUtils<'l>, @@ -95,7 +95,7 @@ pub struct DumpVisitor<'l, 'tcx, 'll> { impl<'l, 'tcx, 'll> DumpVisitor<'l, 'tcx, 'll> { pub fn new( save_ctxt: SaveContext<'l, 'tcx>, - dumper: &'ll mut JsonDumper, + dumper: &'ll mut Dumper, ) -> DumpVisitor<'l, 'tcx, 'll> { let span_utils = SpanUtils::new(&save_ctxt.tcx.sess); DumpVisitor { diff --git a/src/librustc_save_analysis/json_dumper.rs b/src/librustc_save_analysis/dumper.rs similarity index 95% rename from src/librustc_save_analysis/json_dumper.rs rename to src/librustc_save_analysis/dumper.rs index c1437a27e5d22..6fb55e6c99055 100644 --- a/src/librustc_save_analysis/json_dumper.rs +++ b/src/librustc_save_analysis/dumper.rs @@ -9,14 +9,14 @@ pub struct Access { pub public: bool, } -pub struct JsonDumper { +pub struct Dumper { result: Analysis, config: Config, } -impl JsonDumper { - pub fn new(config: Config) -> JsonDumper { - JsonDumper { +impl Dumper { + pub fn new(config: Config) -> Dumper { + Dumper { config: config.clone(), result: Analysis::new(config), } @@ -27,7 +27,7 @@ impl JsonDumper { } } -impl JsonDumper { +impl Dumper { pub fn crate_prelude(&mut self, data: CratePreludeData) { self.result.prelude = Some(data) } diff --git a/src/librustc_save_analysis/lib.rs b/src/librustc_save_analysis/lib.rs index edaf4c7df67b6..ade5e2eca60ba 100644 --- a/src/librustc_save_analysis/lib.rs +++ b/src/librustc_save_analysis/lib.rs @@ -7,7 +7,7 @@ #![recursion_limit="256"] -mod json_dumper; +mod dumper; mod dump_visitor; #[macro_use] mod span_utils; @@ -39,7 +39,7 @@ use syntax::visit::{self, Visitor}; use syntax::print::pprust::{arg_to_string, ty_to_string}; use syntax_pos::*; -use json_dumper::JsonDumper; +use dumper::Dumper; use dump_visitor::DumpVisitor; use span_utils::SpanUtils; @@ -1076,7 +1076,7 @@ impl<'a> SaveHandler for DumpHandler<'a> { ) { let sess = &save_ctxt.tcx.sess; let (output, file_name) = self.output_file(&save_ctxt); - let mut dumper = JsonDumper::new(save_ctxt.config.clone()); + let mut dumper = Dumper::new(save_ctxt.config.clone()); let mut visitor = DumpVisitor::new(save_ctxt, &mut dumper); visitor.dump_crate_info(cratename, krate); @@ -1109,12 +1109,12 @@ impl<'b> SaveHandler for CallbackHandler<'b> { cratename: &str, input: &'l Input, ) { - // We're using the JsonDumper here because it has the format of the + // We're using the Dumper here because it has the format of the // save-analysis results that we will pass to the callback. IOW, we are - // using the JsonDumper to collect the save-analysis results, but not + // using the Dumper to collect the save-analysis results, but not // actually to dump them to a file. This is all a bit convoluted and // there is certainly a simpler design here trying to get out (FIXME). - let mut dumper = JsonDumper::new(save_ctxt.config.clone()); + let mut dumper = Dumper::new(save_ctxt.config.clone()); let mut visitor = DumpVisitor::new(save_ctxt, &mut dumper); visitor.dump_crate_info(cratename, krate); From b75dfa8a2bac745d7d09212e3e28cb4f0bc28fdf Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Jul 2019 19:29:48 +0200 Subject: [PATCH 11/21] Don't access a static just for its size and alignment --- src/librustc_mir/interpret/memory.rs | 27 ++++++++++++------------ src/test/ui/consts/static-cycle-error.rs | 11 ++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-) create mode 100644 src/test/ui/consts/static-cycle-error.rs diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 3f2a76a77be36..674ae29070644 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,6 +535,19 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { + // Allocations of `static` items + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + return Ok((layout.size, layout.align.abi)); + } + _ => {} + } // Regular allocations. if let Ok(alloc) = self.get(id) { return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); @@ -548,20 +561,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { Ok((Size::ZERO, Align::from_bytes(1).unwrap())) }; } - // Foreign statics. - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - match alloc { - Some(GlobalAlloc::Static(did)) => { - assert!(self.tcx.is_foreign_item(did)); - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); - } - _ => {} - } // The rest must be dead. if let AllocCheck::MaybeDead = liveness { // Deallocated pointers are allowed, we should be able to find diff --git a/src/test/ui/consts/static-cycle-error.rs b/src/test/ui/consts/static-cycle-error.rs new file mode 100644 index 0000000000000..9ce050aae2181 --- /dev/null +++ b/src/test/ui/consts/static-cycle-error.rs @@ -0,0 +1,11 @@ +// check-pass + +struct Foo { + foo: Option<&'static Foo> +} + +static FOO: Foo = Foo { + foo: Some(&FOO), +}; + +fn main() {} From d9ac0c67ed5a3ea7d708894a4636a6e83c5aec49 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Jul 2019 23:53:05 +0200 Subject: [PATCH 12/21] Rewrite `get_size_and_align` so it doesn't duplicate work --- src/librustc_mir/interpret/memory.rs | 69 ++++++++++++++-------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 674ae29070644..c8e09ca4a1a47 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,40 +535,41 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - // Allocations of `static` items - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - match alloc { - Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); - } - _ => {} - } - // Regular allocations. - if let Ok(alloc) = self.get(id) { - return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); - } - // Function pointers. - if let Ok(_) = self.get_fn_alloc(id) { - return if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - err!(DerefFunctionPointer) - } else { - Ok((Size::ZERO, Align::from_bytes(1).unwrap())) - }; - } - // The rest must be dead. - if let AllocCheck::MaybeDead = liveness { - // Deallocated pointers are allowed, we should be able to find - // them in the map. - Ok(*self.dead_alloc_map.get(&id) - .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) - } else { - err!(DanglingPointerDeref) + let alloc_or_size_align = self.alloc_map.get_or(id, || -> Result<_, InterpResult<'static, (Size, Align)>> { + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + Err(match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + Ok((layout.size, layout.align.abi)) + }, + Some(GlobalAlloc::Memory(alloc)) => + // this duplicates the logic on the `match alloc_or_size_align`, but due to the + // API of `get_or` there's no way around that. + Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + err!(DerefFunctionPointer) + } else { + Ok((Size::ZERO, Align::from_bytes(1).unwrap())) + }, + // The rest must be dead. + None => if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) + } else { + err!(DanglingPointerDeref) + }, + }) + }); + match alloc_or_size_align { + Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Err(done) => done, } } From 34e7a3cc4dcb7ddd404b9566047a78d1e234f137 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 08:10:09 +0200 Subject: [PATCH 13/21] Fix tidy --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index c8e09ca4a1a47..4ac5920c60e1a 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,7 +535,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - let alloc_or_size_align = self.alloc_map.get_or(id, || -> Result<_, InterpResult<'static, (Size, Align)>> { + let alloc_or_size_align = self.alloc_map.get_or(id, || { // Can't do this in the match argument, we may get cycle errors since the lock would // be held throughout the match. let alloc = self.tcx.alloc_map.lock().get(id); From 3bc1d01bb9d81fa3682186d8ace06becaa8cac8c Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 10:34:54 +0200 Subject: [PATCH 14/21] Clear up `get_size_and_align` --- src/librustc_mir/interpret/memory.rs | 71 +++++++++++++++------------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 4ac5920c60e1a..9140c90bed376 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,41 +535,44 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - let alloc_or_size_align = self.alloc_map.get_or(id, || { - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - Err(match alloc { - Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - Ok((layout.size, layout.align.abi)) - }, - Some(GlobalAlloc::Memory(alloc)) => - // this duplicates the logic on the `match alloc_or_size_align`, but due to the - // API of `get_or` there's no way around that. - Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - err!(DerefFunctionPointer) - } else { - Ok((Size::ZERO, Align::from_bytes(1).unwrap())) - }, - // The rest must be dead. - None => if let AllocCheck::MaybeDead = liveness { - // Deallocated pointers are allowed, we should be able to find - // them in the map. - Ok(*self.dead_alloc_map.get(&id) - .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) - } else { - err!(DanglingPointerDeref) - }, - }) - }); - match alloc_or_size_align { + // Don't use `self.get` here as that will + // a) cause cycles in case `id` refers to a static + // b) duplicate a static's allocation in miri + match self.alloc_map.get_or(id, || Err(())) { Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Err(done) => done, + Err(()) => { + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + return Ok((layout.size, layout.align.abi)); + }, + Some(GlobalAlloc::Memory(alloc)) => + // Need to duplicate the logic here, because the global allocations have + // different associated types than the interpreter-local ones + Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + return err!(DerefFunctionPointer); + } else { + return Ok((Size::ZERO, Align::from_bytes(1).unwrap())); + }, + // The rest must be dead. + None => return if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in \ + `dead_alloc_map`")) + } else { + err!(DanglingPointerDeref) + }, + } + } } } From 796e7a8d7ca1b06e07112579cdd74acc7b1c102b Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:10:21 +0200 Subject: [PATCH 15/21] Address review comments --- src/librustc_mir/interpret/memory.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 9140c90bed376..934fa7f6f877b 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -541,6 +541,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { match self.alloc_map.get_or(id, || Err(())) { Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), Err(()) => { + // Not a local allocation, check the global `tcx.alloc_map`. + // Can't do this in the match argument, we may get cycle errors since the lock would // be held throughout the match. let alloc = self.tcx.alloc_map.lock().get(id); @@ -549,20 +551,22 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // Use size and align of the type let ty = self.tcx.type_of(did); let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); + Ok((layout.size, layout.align.abi)) }, Some(GlobalAlloc::Memory(alloc)) => // Need to duplicate the logic here, because the global allocations have // different associated types than the interpreter-local ones Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - return err!(DerefFunctionPointer); - } else { - return Ok((Size::ZERO, Align::from_bytes(1).unwrap())); + Some(GlobalAlloc::Function(_)) => { + if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + err!(DerefFunctionPointer) + } else { + Ok((Size::ZERO, Align::from_bytes(1).unwrap())) + } }, // The rest must be dead. - None => return if let AllocCheck::MaybeDead = liveness { + None => if let AllocCheck::MaybeDead = liveness { // Deallocated pointers are allowed, we should be able to find // them in the map. Ok(*self.dead_alloc_map.get(&id) From 6e04ca7fb6865f1481a7b2e635fd67b586dc0216 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:43:49 +0200 Subject: [PATCH 16/21] Update src/librustc_mir/interpret/memory.rs Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 934fa7f6f877b..1981a239a1196 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -548,7 +548,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { let alloc = self.tcx.alloc_map.lock().get(id); match alloc { Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type + // Use size and align of the type. let ty = self.tcx.type_of(did); let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); Ok((layout.size, layout.align.abi)) From 0cd71678e17973ed40f898101d01588bf6f6757a Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:44:11 +0200 Subject: [PATCH 17/21] Update src/librustc_mir/interpret/memory.rs Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 1981a239a1196..4575784ac3703 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -555,7 +555,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { }, Some(GlobalAlloc::Memory(alloc)) => // Need to duplicate the logic here, because the global allocations have - // different associated types than the interpreter-local ones + // different associated types than the interpreter-local ones. Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), Some(GlobalAlloc::Function(_)) => { if let AllocCheck::Dereferencable = liveness { From d7b211025ef4fface0f3cd3a01ec19d0b08d7cf6 Mon Sep 17 00:00:00 2001 From: Niv Kaminer Date: Fri, 26 Jul 2019 18:56:47 +0300 Subject: [PATCH 18/21] add repr(transparent) to IoSliceMut where missing --- src/libstd/sys/unix/io.rs | 1 + src/libstd/sys/wasi/io.rs | 1 + src/libstd/sys/windows/io.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/libstd/sys/unix/io.rs b/src/libstd/sys/unix/io.rs index 72954ff20ef95..bc854e772e16f 100644 --- a/src/libstd/sys/unix/io.rs +++ b/src/libstd/sys/unix/io.rs @@ -29,6 +29,7 @@ impl<'a> IoSlice<'a> { } } +#[repr(transparent)] pub struct IoSliceMut<'a> { vec: iovec, _p: PhantomData<&'a mut [u8]>, diff --git a/src/libstd/sys/wasi/io.rs b/src/libstd/sys/wasi/io.rs index cc8f1e16fa01d..a5bddad708b19 100644 --- a/src/libstd/sys/wasi/io.rs +++ b/src/libstd/sys/wasi/io.rs @@ -29,6 +29,7 @@ impl<'a> IoSlice<'a> { } } +#[repr(transparent)] pub struct IoSliceMut<'a> { vec: __wasi_iovec_t, _p: PhantomData<&'a mut [u8]>, diff --git a/src/libstd/sys/windows/io.rs b/src/libstd/sys/windows/io.rs index c045a63e9118f..f0da2323f4f57 100644 --- a/src/libstd/sys/windows/io.rs +++ b/src/libstd/sys/windows/io.rs @@ -29,6 +29,7 @@ impl<'a> IoSlice<'a> { } } +#[repr(transparent)] pub struct IoSliceMut<'a> { vec: c::WSABUF, _p: PhantomData<&'a mut [u8]>, From cae8680544418d838344d9c258030592f0461ee9 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 26 Jul 2019 17:31:39 +0100 Subject: [PATCH 19/21] lowering: Omit bare trait lint on macro call sites This commit implements a hacky fix for detecting when a span is pointing at a macro call site so that bare trait lints are not made incorrectly. --- src/librustc/hir/lowering.rs | 22 +++++++++++++++------- src/test/ui/suggestions/issue-61963.rs | 1 - src/test/ui/suggestions/issue-61963.stderr | 10 ++-------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index c228bc2cf6b8c..248a120e741aa 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -5754,13 +5754,21 @@ impl<'a> LoweringContext<'a> { } fn maybe_lint_bare_trait(&self, span: Span, id: NodeId, is_global: bool) { - self.sess.buffer_lint_with_diagnostic( - builtin::BARE_TRAIT_OBJECTS, - id, - span, - "trait objects without an explicit `dyn` are deprecated", - builtin::BuiltinLintDiagnostics::BareTraitObject(span, is_global), - ) + // FIXME(davidtwco): This is a hack to detect macros which produce spans of the + // call site which do not have a macro backtrace. See #61963. + let is_macro_callsite = self.sess.source_map() + .span_to_snippet(span) + .map(|snippet| snippet.starts_with("#[")) + .unwrap_or(true); + if !is_macro_callsite { + self.sess.buffer_lint_with_diagnostic( + builtin::BARE_TRAIT_OBJECTS, + id, + span, + "trait objects without an explicit `dyn` are deprecated", + builtin::BuiltinLintDiagnostics::BareTraitObject(span, is_global), + ) + } } fn wrap_in_try_constructor( diff --git a/src/test/ui/suggestions/issue-61963.rs b/src/test/ui/suggestions/issue-61963.rs index 540a7c42d2fd3..c9d738f5a283e 100644 --- a/src/test/ui/suggestions/issue-61963.rs +++ b/src/test/ui/suggestions/issue-61963.rs @@ -15,7 +15,6 @@ pub trait Bar { } pub struct Qux(T); #[dom_struct] -//~^ ERROR trait objects without an explicit `dyn` are deprecated [bare_trait_objects] pub struct Foo { qux: Qux>, bar: Box, diff --git a/src/test/ui/suggestions/issue-61963.stderr b/src/test/ui/suggestions/issue-61963.stderr index 261b384ca5716..46943f40066ff 100644 --- a/src/test/ui/suggestions/issue-61963.stderr +++ b/src/test/ui/suggestions/issue-61963.stderr @@ -1,5 +1,5 @@ error: trait objects without an explicit `dyn` are deprecated - --> $DIR/issue-61963.rs:21:14 + --> $DIR/issue-61963.rs:20:14 | LL | bar: Box, | ^^^ help: use `dyn`: `dyn Bar` @@ -10,11 +10,5 @@ note: lint level defined here LL | #![deny(bare_trait_objects)] | ^^^^^^^^^^^^^^^^^^ -error: trait objects without an explicit `dyn` are deprecated - --> $DIR/issue-61963.rs:17:1 - | -LL | #[dom_struct] - | ^^^^^^^^^^^^^ help: use `dyn`: `dyn #[dom_struct]` - -error: aborting due to 2 previous errors +error: aborting due to previous error From 13b41000ea7b6fdcc399a817ec4e3e3b9b1281a3 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sat, 27 Jul 2019 17:21:42 +0900 Subject: [PATCH 20/21] Add lib section to rustc_lexer's Cargo.toml --- src/librustc_lexer/Cargo.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/librustc_lexer/Cargo.toml b/src/librustc_lexer/Cargo.toml index 9c0230e83221f..9dcd39ee03965 100644 --- a/src/librustc_lexer/Cargo.toml +++ b/src/librustc_lexer/Cargo.toml @@ -7,3 +7,7 @@ edition = "2018" # Note that this crate purposefully does not depend on other rustc crates [dependencies] unicode-xid = { version = "0.1.0", optional = true } + +[lib] +doctest = false +name = "rustc_lexer" From 98f29f5e38bc385f661fc81d67f980bc0e91d6e4 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sat, 27 Jul 2019 22:10:09 +0900 Subject: [PATCH 21/21] Add comment --- src/librustc_lexer/Cargo.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/librustc_lexer/Cargo.toml b/src/librustc_lexer/Cargo.toml index 9dcd39ee03965..0dbcda618ecac 100644 --- a/src/librustc_lexer/Cargo.toml +++ b/src/librustc_lexer/Cargo.toml @@ -8,6 +8,8 @@ edition = "2018" [dependencies] unicode-xid = { version = "0.1.0", optional = true } +# Note: do not remove this blank `[lib]` section. +# This will be used when publishing this crate as `rustc-ap-rustc_lexer`. [lib] doctest = false name = "rustc_lexer"