Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error on nested impl Trait and path projections from impl Trait #48084

Merged
merged 5 commits into from
Feb 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 135 additions & 0 deletions src/librustc_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,141 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
}

// Bans nested `impl Trait`, e.g. `impl Into<impl Debug>`.
// Nested `impl Trait` _is_ allowed in associated type position,
// e.g `impl Iterator<Item=impl Debug>`
struct NestedImplTraitVisitor<'a> {
session: &'a Session,
outer_impl_trait: Option<Span>,
}

impl<'a> NestedImplTraitVisitor<'a> {
fn with_impl_trait<F>(&mut self, outer_impl_trait: Option<Span>, f: F)
where F: FnOnce(&mut NestedImplTraitVisitor<'a>)
{
let old_outer_impl_trait = self.outer_impl_trait;
self.outer_impl_trait = outer_impl_trait;
f(self);
self.outer_impl_trait = old_outer_impl_trait;
}
}


impl<'a> Visitor<'a> for NestedImplTraitVisitor<'a> {
fn visit_ty(&mut self, t: &'a Ty) {
if let TyKind::ImplTrait(_) = t.node {
if let Some(outer_impl_trait) = self.outer_impl_trait {
struct_span_err!(self.session, t.span, E0666,
"nested `impl Trait` is not allowed")
.span_label(outer_impl_trait, "outer `impl Trait`")
.span_label(t.span, "nested `impl Trait` here")
.emit();

}
self.with_impl_trait(Some(t.span), |this| visit::walk_ty(this, t));
} else {
visit::walk_ty(self, t);
}
}
fn visit_path_parameters(&mut self, _: Span, path_parameters: &'a PathParameters) {
match *path_parameters {
PathParameters::AngleBracketed(ref params) => {
for type_ in &params.types {
self.visit_ty(type_);
}
for type_binding in &params.bindings {
// Type bindings such as `Item=impl Debug` in `Iterator<Item=Debug>`
// are allowed to contain nested `impl Trait`.
self.with_impl_trait(None, |this| visit::walk_ty(this, &type_binding.ty));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code would permit this

<dyn Iterator<Item = impl Debug> as Iterator>::Item

which ought not to be allowed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, probably not, because that is handled by a totally distinct visitor, I see

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for it, anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
PathParameters::Parenthesized(ref params) => {
for type_ in &params.inputs {
self.visit_ty(type_);
}
if let Some(ref type_) = params.output {
// `-> Foo` syntax is essentially an associated type binding,
// so it is also allowed to contain nested `impl Trait`.
self.with_impl_trait(None, |this| visit::walk_ty(this, type_));
}
}
}
}
}

// Bans `impl Trait` in path projections like `<impl Iterator>::Item` or `Foo::Bar<impl Trait>`.
struct ImplTraitProjectionVisitor<'a> {
session: &'a Session,
is_banned: bool,
}

impl<'a> ImplTraitProjectionVisitor<'a> {
fn with_ban<F>(&mut self, f: F)
where F: FnOnce(&mut ImplTraitProjectionVisitor<'a>)
{
let old_is_banned = self.is_banned;
self.is_banned = true;
f(self);
self.is_banned = old_is_banned;
}
}

impl<'a> Visitor<'a> for ImplTraitProjectionVisitor<'a> {
fn visit_ty(&mut self, t: &'a Ty) {
match t.node {
TyKind::ImplTrait(_) => {
if self.is_banned {
struct_span_err!(self.session, t.span, E0667,
"`impl Trait` is not allowed in path parameters")
.emit();
}
}
TyKind::Path(ref qself, ref path) => {
// We allow these:
// - `Option<impl Trait>`
// - `option::Option<impl Trait>`
// - `option::Option<T>::Foo<impl Trait>
//
// But not these:
// - `<impl Trait>::Foo`
// - `option::Option<impl Trait>::Foo`.
//
// To implement this, we disallow `impl Trait` from `qself`
// (for cases like `<impl Trait>::Foo>`)
// but we allow `impl Trait` in `PathParameters`
// iff there are no more PathSegments.
if let Some(ref qself) = *qself {
// `impl Trait` in `qself` is always illegal
self.with_ban(|this| this.visit_ty(&qself.ty));
}

for (i, segment) in path.segments.iter().enumerate() {
// Allow `impl Trait` iff we're on the final path segment
if i == (path.segments.len() - 1) {
visit::walk_path_segment(self, path.span, segment);
} else {
self.with_ban(|this|
visit::walk_path_segment(this, path.span, segment));
}
}
}
_ => visit::walk_ty(self, t),
}
}
}

pub fn check_crate(session: &Session, krate: &Crate) {
visit::walk_crate(
&mut NestedImplTraitVisitor {
session,
outer_impl_trait: None,
}, krate);

visit::walk_crate(
&mut ImplTraitProjectionVisitor {
session,
is_banned: false,
}, krate);

visit::walk_crate(&mut AstValidator { session: session }, krate)
}
2 changes: 2 additions & 0 deletions src/librustc_passes/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,4 +320,6 @@ register_diagnostics! {
E0567, // auto traits can not have generic parameters
E0568, // auto traits can not have super traits
E0642, // patterns aren't allowed in methods without bodies
E0666, // nested `impl Trait` is illegal
E0667, // `impl Trait` in projections
}
70 changes: 1 addition & 69 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,6 @@ declare_features! (
// `foo.rs` as an alternative to `foo/mod.rs`
(active, non_modrs_mods, "1.24.0", Some(44660)),

// Nested `impl Trait`
(active, nested_impl_trait, "1.24.0", Some(34511)),

// Termination trait in main (RFC 1937)
(active, termination_trait, "1.24.0", Some(43301)),

Expand Down Expand Up @@ -1352,73 +1349,8 @@ fn contains_novel_literal(item: &ast::MetaItem) -> bool {
}
}

// Bans nested `impl Trait`, e.g. `impl Into<impl Debug>`.
// Nested `impl Trait` _is_ allowed in associated type position,
// e.g `impl Iterator<Item=impl Debug>`
struct NestedImplTraitVisitor<'a> {
context: &'a Context<'a>,
is_in_impl_trait: bool,
}

impl<'a> NestedImplTraitVisitor<'a> {
fn with_impl_trait<F>(&mut self, is_in_impl_trait: bool, f: F)
where F: FnOnce(&mut NestedImplTraitVisitor<'a>)
{
let old_is_in_impl_trait = self.is_in_impl_trait;
self.is_in_impl_trait = is_in_impl_trait;
f(self);
self.is_in_impl_trait = old_is_in_impl_trait;
}
}


impl<'a> Visitor<'a> for NestedImplTraitVisitor<'a> {
fn visit_ty(&mut self, t: &'a ast::Ty) {
if let ast::TyKind::ImplTrait(_) = t.node {
if self.is_in_impl_trait {
gate_feature_post!(&self, nested_impl_trait, t.span,
"nested `impl Trait` is experimental"
);
}
self.with_impl_trait(true, |this| visit::walk_ty(this, t));
} else {
visit::walk_ty(self, t);
}
}
fn visit_path_parameters(&mut self, _: Span, path_parameters: &'a ast::PathParameters) {
match *path_parameters {
ast::PathParameters::AngleBracketed(ref params) => {
for type_ in &params.types {
self.visit_ty(type_);
}
for type_binding in &params.bindings {
// Type bindings such as `Item=impl Debug` in `Iterator<Item=Debug>`
// are allowed to contain nested `impl Trait`.
self.with_impl_trait(false, |this| visit::walk_ty(this, &type_binding.ty));
}
}
ast::PathParameters::Parenthesized(ref params) => {
for type_ in &params.inputs {
self.visit_ty(type_);
}
if let Some(ref type_) = params.output {
// `-> Foo` syntax is essentially an associated type binding,
// so it is also allowed to contain nested `impl Trait`.
self.with_impl_trait(false, |this| visit::walk_ty(this, type_));
}
}
}
}
}

impl<'a> PostExpansionVisitor<'a> {
fn whole_crate_feature_gates(&mut self, krate: &ast::Crate) {
visit::walk_crate(
&mut NestedImplTraitVisitor {
context: self.context,
is_in_impl_trait: false,
}, krate);

fn whole_crate_feature_gates(&mut self, _krate: &ast::Crate) {
for &(ident, span) in &*self.context.parse_sess.non_modrs_mods.borrow() {
if !span.allows_unstable() {
let cx = &self.context;
Expand Down
4 changes: 3 additions & 1 deletion src/test/compile-fail/impl-trait/where-allowed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

//! A simple test for testing many permutations of allowedness of
//! impl Trait
#![feature(conservative_impl_trait, nested_impl_trait, universal_impl_trait, dyn_trait)]
#![feature(conservative_impl_trait, universal_impl_trait, dyn_trait)]
use std::fmt::Debug;

// Allowed
Expand Down Expand Up @@ -60,6 +60,7 @@ fn in_dyn_Fn_return_in_return() -> &'static dyn Fn() -> impl Debug { panic!() }
// Disallowed
fn in_impl_Fn_parameter_in_parameters(_: &impl Fn(impl Debug)) { panic!() }
//~^ ERROR `impl Trait` not allowed outside of function and inherent method return types
//~^^ ERROR nested `impl Trait` is not allowed

// Disallowed
fn in_impl_Fn_return_in_parameters(_: &impl Fn() -> impl Debug) { panic!() }
Expand All @@ -68,6 +69,7 @@ fn in_impl_Fn_return_in_parameters(_: &impl Fn() -> impl Debug) { panic!() }
// Disallowed
fn in_impl_Fn_parameter_in_return() -> &'static impl Fn(impl Debug) { panic!() }
//~^ ERROR `impl Trait` not allowed outside of function and inherent method return types
//~^^ ERROR nested `impl Trait` is not allowed

// Disallowed
fn in_impl_Fn_return_in_return() -> &'static impl Fn() -> impl Debug { panic!() }
Expand Down
11 changes: 5 additions & 6 deletions src/test/run-pass/impl-trait/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(conservative_impl_trait, underscore_lifetimes, universal_impl_trait, nested_impl_trait)]
#![feature(conservative_impl_trait, underscore_lifetimes, universal_impl_trait)]
#![allow(warnings)]

use std::fmt::Debug;
Expand Down Expand Up @@ -55,12 +55,11 @@ fn pass_through_elision_with_fn_ptr(x: &fn(&u32) -> &u32) -> impl Into<&fn(&u32)

fn pass_through_elision_with_fn_path<T: Fn(&u32) -> &u32>(
x: &T
) -> impl Into<&impl Fn(&u32) -> &u32> { x }
) -> &impl Fn(&u32) -> &u32 { x }

fn foo(x: &impl Debug) -> impl Into<&impl Debug> { x }
fn foo_explicit_lifetime<'a>(x: &'a impl Debug) -> impl Into<&'a impl Debug> { x }
fn foo_no_outer_impl(x: &impl Debug) -> &impl Debug { x }
fn foo_explicit_arg<T: Debug>(x: &T) -> impl Into<&impl Debug> { x }
fn foo(x: &impl Debug) -> &impl Debug { x }
fn foo_explicit_lifetime<'a>(x: &'a impl Debug) -> &'a impl Debug { x }
fn foo_explicit_arg<T: Debug>(x: &T) -> &impl Debug { x }

fn mixed_lifetimes<'a>() -> impl for<'b: 'a> Fn(&'b u32) { |_| () }
fn mixed_as_static() -> impl Fn(&'static u32) { mixed_lifetimes() }
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/error-codes/E0657.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
#![allow(warnings)]
#![feature(conservative_impl_trait, nested_impl_trait)]
#![feature(conservative_impl_trait)]

trait Id<T> {}
trait Lt<'a> {}
Expand All @@ -17,7 +17,7 @@ impl<'a> Lt<'a> for () {}
impl<T> Id<T> for T {}

fn free_fn_capture_hrtb_in_impl_trait()
-> impl for<'a> Id<impl Lt<'a>>
-> Box<for<'a> Id<impl Lt<'a>>>
//~^ ERROR `impl Trait` can only capture lifetimes bound at the fn or impl level [E0657]
{
()
Expand All @@ -26,7 +26,7 @@ fn free_fn_capture_hrtb_in_impl_trait()
struct Foo;
impl Foo {
fn impl_fn_capture_hrtb_in_impl_trait()
-> impl for<'a> Id<impl Lt<'a>>
-> Box<for<'a> Id<impl Lt<'a>>>
//~^ ERROR `impl Trait` can only capture lifetimes bound at the fn or impl level
{
()
Expand Down
12 changes: 6 additions & 6 deletions src/test/ui/error-codes/E0657.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
error[E0657]: `impl Trait` can only capture lifetimes bound at the fn or impl level
--> $DIR/E0657.rs:20:32
--> $DIR/E0657.rs:20:31
|
20 | -> impl for<'a> Id<impl Lt<'a>>
| ^^
20 | -> Box<for<'a> Id<impl Lt<'a>>>
| ^^

error[E0657]: `impl Trait` can only capture lifetimes bound at the fn or impl level
--> $DIR/E0657.rs:29:36
--> $DIR/E0657.rs:29:35
|
29 | -> impl for<'a> Id<impl Lt<'a>>
| ^^
29 | -> Box<for<'a> Id<impl Lt<'a>>>
| ^^

error: aborting due to 2 previous errors

50 changes: 50 additions & 0 deletions src/test/ui/impl_trait_projections.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright 2018 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
#![feature(dyn_trait, conservative_impl_trait, universal_impl_trait)]

use std::fmt::Debug;
use std::option;

fn parametrized_type_is_allowed() -> Option<impl Debug> {
Some(5i32)
}

fn path_parametrized_type_is_allowed() -> option::Option<impl Debug> {
Some(5i32)
}

fn projection_is_disallowed(x: impl Iterator) -> <impl Iterator>::Item {
//~^ ERROR `impl Trait` is not allowed in path parameters
//~^^ ERROR ambiguous associated type
x.next().unwrap()
}

fn projection_with_named_trait_is_disallowed(x: impl Iterator)
-> <impl Iterator as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
{
x.next().unwrap()
}

fn projection_with_named_trait_inside_path_is_disallowed()
-> <::std::ops::Range<impl Debug> as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
{
(1i32..100).next().unwrap()
}

fn projection_from_impl_trait_inside_dyn_trait_is_disallowed()
-> <dyn Iterator<Item = impl Debug> as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
{
panic!()
}

fn main() {}
Loading