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.

Sign up for GitHub

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

Jump to bottom

Merge visitors in AST validation #57730

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Merge visitors in AST validation #57730

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13dc584
Merge visitors in AST validation
Zoxc Jan 18, 2019
a5d4aed
Address some comments
Zoxc Jan 19, 2019
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
255 changes: 111 additions & 144 deletions src/librustc_passes/ast_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,79 @@ use errors::Applicability;

struct AstValidator<'a> {
session: &'a Session,

// Used to ban 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>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also add a note about why we do this (cause we are not sure whether we want it to be higher rank: impl for<T: Debug> Into<T>, or rank-1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why we do this. I just copied this comment. Feel free to suggest the exact change you'd like.

outer_impl_trait: Option<Span>,

// Used to ban `impl Trait` in path projections like `<impl Iterator>::Item`
// or `Foo::Bar<impl Trait>`
is_impl_trait_banned: bool,
}

impl<'a> AstValidator<'a> {
fn with_banned_impl_trait<F>(&mut self, f: F)
Copy link
Contributor

Choose a reason for hiding this comment

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

use f: impl FnOnce(&mut Self) for increased readability?

where F: FnOnce(&mut Self)
{
let old_is_impl_trait_banned = self.is_impl_trait_banned;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use mem::replace?

self.is_impl_trait_banned = true;
f(self);
self.is_impl_trait_banned = old_is_impl_trait_banned;
}

fn with_impl_trait<F>(&mut self, outer_impl_trait: Option<Span>, f: F)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.. f: impl FnOnce(&mut Self).

where F: FnOnce(&mut Self)
{
let old_outer_impl_trait = self.outer_impl_trait;
Copy link
Contributor

Choose a reason for hiding this comment

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

use mem::replace here also?

self.outer_impl_trait = outer_impl_trait;
f(self);
self.outer_impl_trait = old_outer_impl_trait;
}

// Mirrors visit::walk_ty, but tracks relevant state
fn walk_ty(&mut self, t: &'a Ty) {
match t.node {
TyKind::ImplTrait(..) => {
self.with_impl_trait(Some(t.span), |this| visit::walk_ty(this, t))
}
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 `GenericArgs`
// iff there are no more PathSegments.
if let Some(ref qself) = *qself {
// `impl Trait` in `qself` is always illegal
self.with_banned_impl_trait(|this| this.visit_ty(&qself.ty));
}

// Note that there should be a call to visit_path here,
// so if any logic is added to process `Path`s a call to it should be
// added both in visit_path and here. This code mirrors visit::walk_path.
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 {
self.visit_path_segment(path.span, segment);
} else {
self.with_banned_impl_trait(|this| {
this.visit_path_segment(path.span, segment)
});
}
}
}
_ => visit::walk_ty(self, t),
}
}

fn err_handler(&self) -> &errors::Handler {
&self.session.diagnostic()
}
Expand Down Expand Up @@ -267,6 +337,19 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self.no_questions_in_bounds(bounds, "trait object types", false);
}
TyKind::ImplTrait(_, ref bounds) => {
if self.is_impl_trait_banned {
struct_span_err!(self.session, ty.span, E0667,
"`impl Trait` is not allowed in path parameters").emit();
}

if let Some(outer_impl_trait) = self.outer_impl_trait {
struct_span_err!(self.session, ty.span, E0666,
"nested `impl Trait` is not allowed")
.span_label(outer_impl_trait, "outer `impl Trait`")
.span_label(ty.span, "nested `impl Trait` here")
.emit();

}
if !bounds.iter()
.any(|b| if let GenericBound::Trait(..) = *b { true } else { false }) {
self.err_handler().span_err(ty.span, "at least one trait must be specified");
Expand All @@ -275,7 +358,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
_ => {}
}

visit::walk_ty(self, ty)
self.walk_ty(ty)
}

fn visit_label(&mut self, label: &'a Label) {
Expand Down Expand Up @@ -414,6 +497,28 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
visit::walk_foreign_item(self, fi)
}

// Mirrors visit::walk_generic_args, but tracks relevant state
fn visit_generic_args(&mut self, _: Span, generic_args: &'a GenericArgs) {
match *generic_args {
GenericArgs::AngleBracketed(ref data) => {
walk_list!(self, visit_generic_arg, &data.args);
// Type bindings such as `Item=impl Debug` in `Iterator<Item=Debug>`
// are allowed to contain nested `impl Trait`.
self.with_impl_trait(None, |this| {
walk_list!(this, visit_assoc_type_binding, &data.bindings);
});
}
GenericArgs::Parenthesized(ref data) => {
walk_list!(self, visit_ty, &data.inputs);
if let Some(ref type_) = data.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_));
}
}
}
}

fn visit_generics(&mut self, generics: &'a Generics) {
let mut seen_non_lifetime_param = false;
let mut seen_default = None;
Expand Down Expand Up @@ -490,148 +595,10 @@ 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_generic_args(&mut self, _: Span, generic_args: &'a GenericArgs) {
match *generic_args {
GenericArgs::AngleBracketed(ref data) => {
for arg in &data.args {
self.visit_generic_arg(arg)
}
for type_binding in &data.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));
}
}
GenericArgs::Parenthesized(ref data) => {
for type_ in &data.inputs {
self.visit_ty(type_);
}
if let Some(ref type_) = data.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_));
}
}
}
}

fn visit_mac(&mut self, _mac: &Spanned<Mac_>) {
// covered in AstValidator
}
}

// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR will give errors about nested impl traits being banned in path parameters, unlike the old code, which doesn't visit inside impl Trait. So we'd get 3 errors then, one for nested impl Trait and one for each instance of impl Trait in a path parameter.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM.

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 `GenericArgs`
// 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),
}
}

fn visit_mac(&mut self, _mac: &Spanned<Mac_>) {
// covered in AstValidator
}
}

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 }, krate)
visit::walk_crate(&mut AstValidator {
session,
outer_impl_trait: None,
is_impl_trait_banned: false,
}, krate)
}