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

Add a feature gate for nested uses of impl Trait #46888

Merged
merged 1 commit into from
Dec 24, 2017
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
72 changes: 70 additions & 2 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,9 @@ 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)),
);

declare_features! (
Expand Down Expand Up @@ -1314,8 +1317,73 @@ 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) {
fn whole_crate_feature_gates(&mut self, krate: &ast::Crate) {
visit::walk_crate(
&mut NestedImplTraitVisitor {
context: self.context,
is_in_impl_trait: false,
}, krate);

for &(ident, span) in &*self.context.parse_sess.non_modrs_mods.borrow() {
if !span.allows_unstable() {
let cx = &self.context;
Expand Down Expand Up @@ -1892,7 +1960,7 @@ pub fn check_crate(krate: &ast::Crate,
plugin_attributes,
};
let visitor = &mut PostExpansionVisitor { context: &ctx };
visitor.whole_crate_feature_gates();
visitor.whole_crate_feature_gates(krate);
visit::walk_crate(visitor, krate);
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/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)]
#![feature(conservative_impl_trait, nested_impl_trait)]

trait Id<T> {}
trait Lt<'a> {}
Expand Down
39 changes: 39 additions & 0 deletions src/test/compile-fail/feature-gate-nested_impl_trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// 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 <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(conservative_impl_trait, universal_impl_trait)]

use std::fmt::Debug;

fn fine(x: impl Into<u32>) -> impl Into<u32> { x }
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test for fn bad(x: impl Into<impl Debug>) -- it occurs to me that nested syntax in this position is also "not great" for the same reason. Though perhaps a mite less bad in that it's more clear how we would allow those types to be specified in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I purposefully made that work, so that test would fail. I will make the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.


fn bad_in_ret_position(x: impl Into<u32>) -> impl Into<impl Debug> { x }
//~^ ERROR nested `impl Trait` is experimental

fn bad_in_fn_syntax(x: fn() -> impl Into<impl Debug>) {}
//~^ ERROR nested `impl Trait` is experimental

fn bad_in_arg_position(_: impl Into<impl Debug>) { }
//~^ ERROR nested `impl Trait` is experimental

struct X;
impl X {
fn bad(x: impl Into<u32>) -> impl Into<impl Debug> { x }
//~^ ERROR nested `impl Trait` is experimental
}

fn allowed_in_assoc_type() -> impl Iterator<Item=impl Fn()> {
vec![|| println!("woot")].into_iter()
}

fn allowed_in_ret_type() -> impl Fn() -> impl Into<u32> {
|| 5
}

fn main() {}
2 changes: 1 addition & 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, universal_impl_trait, dyn_trait)]
#![feature(conservative_impl_trait, nested_impl_trait, universal_impl_trait, dyn_trait)]
use std::fmt::Debug;

// Allowed
Expand Down
2 changes: 1 addition & 1 deletion 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)]
#![feature(conservative_impl_trait, underscore_lifetimes, universal_impl_trait, nested_impl_trait)]
#![allow(warnings)]

use std::fmt::Debug;
Expand Down