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 ability to attach custom #[on_unimplemented] error messages for unimplemented traits #20889

Merged
merged 7 commits into from
Jan 12, 2015
2 changes: 2 additions & 0 deletions src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ pub trait Iterator {

/// Conversion from an `Iterator`
#[stable]
#[on_unimplemented="a collection of type `{Self}` cannot be \
built from an iterator over elements of type `{A}`"]
pub trait FromIterator<A> {
/// Build a container with elements from an external iterator.
fn from_iter<T: Iterator<Item=A>>(iterator: T) -> Self;
Expand Down
1 change: 1 addition & 0 deletions src/librustc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

extern crate arena;
extern crate flate;
extern crate fmt_macros;
extern crate getopts;
extern crate graphviz;
extern crate libc;
Expand Down
64 changes: 64 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
//! a `pub fn new()`.
use self::MethodContext::*;


use fmt_macros::{Parser, Piece, Position};

use metadata::csearch;
use middle::def::*;
use middle::subst::Substs;
Expand Down Expand Up @@ -666,6 +669,7 @@ impl LintPass for UnusedAttributes {
"must_use",
"stable",
"unstable",
"on_unimplemented",

// FIXME: #19470 this shouldn't be needed forever
"old_orphan_check",
Expand Down Expand Up @@ -1920,3 +1924,63 @@ impl LintPass for UnstableFeatures {
}
}
}

/// Checks usage of `#[on_unimplemented]`
#[derive(Copy)]
pub struct BadOnUnimplemented;

declare_lint!(BAD_ON_UNIMPLEMENTED, Deny,
"Checks usage of `#[on_unimplemented]`");

impl LintPass for BadOnUnimplemented {
fn get_lints(&self) -> LintArray {
lint_array!(BAD_ON_UNIMPLEMENTED)
}
fn check_item(&mut self, ctx: &Context, item: &ast::Item) {
match item.node {
ast::ItemTrait(_, ref generics, _, _) => {
if let Some(ref attr) = item.attrs.iter().find(|&: a| {
a.check_name("on_unimplemented")
}) {
if let Some(ref istring) = attr.value_str() {
let mut parser = Parser::new(istring.get());
let types = generics.ty_params.as_slice();
for token in parser {
match token {
Piece::String(_) => (), // Normal string, no need to check it
Piece::NextArgument(a) => match a.position {
// `{Self}` is allowed
Position::ArgumentNamed(s) if s == "Self" => (),
// So is `{A}` if A is a type parameter
Position::ArgumentNamed(s) => match types.iter().find(|t| {
t.ident.as_str() == s
}) {
Some(_) => (),
None => {
ctx.span_lint(BAD_ON_UNIMPLEMENTED, attr.span,
format!("there is no type parameter \
{} on trait {}",
s, item.ident.as_str())
.as_slice());
}
},
// `{:1}` and `{}` are not to be used
Position::ArgumentIs(_) | Position::ArgumentNext => {
ctx.span_lint(BAD_ON_UNIMPLEMENTED, attr.span,
"only named substitution \
parameters are allowed");
}
}
}
}
} else {
ctx.span_lint(BAD_ON_UNIMPLEMENTED, attr.span,
"this attribute must have a value, \
eg `#[on_unimplemented = \"foo\"]`")
}
}
},
_ => () // Not a trait def, move along
}
}
}
1 change: 1 addition & 0 deletions src/librustc/lint/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ impl LintStore {
UnusedAllocation,
MissingCopyImplementations,
UnstableFeatures,
BadOnUnimplemented,
);

add_builtin_with_new!(sess,
Expand Down
76 changes: 75 additions & 1 deletion src/librustc/middle/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ use super::{
SelectionError,
};

use fmt_macros::{Parser, Piece, Position};
use middle::infer::InferCtxt;
use middle::ty::{self, AsPredicate, ReferencesError, ToPolyTraitRef};
use middle::ty::{self, AsPredicate, ReferencesError, ToPolyTraitRef, TraitRef};
use std::collections::HashMap;
use syntax::codemap::Span;
use syntax::attr::{AttributeMethods, AttrMetaMethods};
use util::ppaux::{Repr, UserString};

pub fn report_fulfillment_errors<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
Expand Down Expand Up @@ -62,6 +65,69 @@ pub fn report_projection_error<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
}
}

fn report_on_unimplemented<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
trait_ref: &TraitRef<'tcx>) -> Option<String> {
let def_id = trait_ref.def_id;
let mut report = None;
ty::each_attr(infcx.tcx, def_id, |item| {
if item.check_name("on_unimplemented") {
if let Some(ref istring) = item.value_str() {
let def = ty::lookup_trait_def(infcx.tcx, def_id);
let mut generic_map = def.generics.types.iter_enumerated()
.map(|(param, i, gen)| {
(gen.name.as_str().to_string(),
trait_ref.substs.types.get(param, i)
.user_string(infcx.tcx))
}).collect::<HashMap<String, String>>();
generic_map.insert("Self".to_string(),
trait_ref.self_ty().user_string(infcx.tcx));
let parser = Parser::new(istring.get());
let mut errored = false;
let err: String = parser.filter_map(|p| {
match p {
Piece::String(s) => Some(s),
Piece::NextArgument(a) => match a.position {
Position::ArgumentNamed(s) => match generic_map.get(s) {
Some(val) => Some(val.as_slice()),
None => {
infcx.tcx.sess
.span_err(item.meta().span,
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I suspect these spans are useless for the most part (I imagine they are usually cross-crate, and so unable to point to the actual attribute in question). It may be better to either use obligation.cause.span always, or detect if item.meta().span is syntax::codemap::DUMMY_SP or not and default to obligation.cause.span when it is DUMMY_SP.

Copy link
Member

Choose a reason for hiding this comment

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

(In fact, thinking about it now, it may also make sense to make these error messages clearer that they are talking about the on_unimplemented message, not the code you're calling, something like invalid #[on_unimplemented] format: there is no....)

format!("there is no type parameter \
{} on trait {}",
s, def.trait_ref
.user_string(infcx.tcx))
.as_slice());
errored = true;
None
}
},
_ => {
infcx.tcx.sess.span_err(item.meta().span,
"only named substitution \
parameters are allowed");
errored = true;
None
}
}
}
}).collect();
// Report only if the format string checks out
if !errored {
report = Some(err);
}
} else {
infcx.tcx.sess.span_err(item.meta().span,
"this attribute must have a value, \
eg `#[on_unimplemented = \"foo\"]`")
}
false
} else {
true
}
});
report
}

pub fn report_selection_error<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
obligation: &PredicateObligation<'tcx>,
error: &SelectionError<'tcx>)
Expand All @@ -88,12 +154,20 @@ pub fn report_selection_error<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
infcx.resolve_type_vars_if_possible(trait_predicate);
if !trait_predicate.references_error() {
let trait_ref = trait_predicate.to_poly_trait_ref();
// Check if it has a custom "#[on_unimplemented]" error message,
// report with that message if it does
let custom_note = report_on_unimplemented(infcx, &*trait_ref.0);
Copy link
Member

Choose a reason for hiding this comment

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

This is probably best run after the main error message, just in case the span_errs inside it are triggered, so that the output has a slightly more sensible order.

infcx.tcx.sess.span_err(
obligation.cause.span,
format!(
"the trait `{}` is not implemented for the type `{}`",
trait_ref.user_string(infcx.tcx),
trait_ref.self_ty().user_string(infcx.tcx)).as_slice());
if let Some(s) = custom_note {
infcx.tcx.sess.span_note(
obligation.cause.span,
s.as_slice());
}
}
}

Expand Down
32 changes: 32 additions & 0 deletions src/test/compile-fail/on-unimplemented-bad-anno.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2014 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.
// ignore-tidy-linelength

#[allow(unused)]

#[on_unimplemented = "test error `{Self}` with `{Bar}` `{Baz}` `{Quux}`"]
trait Foo<Bar, Baz, Quux>{}

#[on_unimplemented="a collection of type `{Self}` cannot be built from an iterator over elements of type `{A}`"]
trait MyFromIterator<A> {
/// Build a container with elements from an external iterator.
fn my_from_iter<T: Iterator<Item=A>>(iterator: T) -> Self;
}

#[on_unimplemented] //~ ERROR this attribute must have a value
trait BadAnnotation1 {}

#[on_unimplemented = "Unimplemented trait error on `{Self}` with params `<{A},{B},{C}>`"]
//~^ ERROR there is no type parameter C on trait BadAnnotation2
trait BadAnnotation2<A,B> {}


pub fn main() {
}
51 changes: 51 additions & 0 deletions src/test/compile-fail/on-unimplemented.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2014 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.
// ignore-tidy-linelength

#[on_unimplemented = "test error `{Self}` with `{Bar}` `{Baz}` `{Quux}`"]
trait Foo<Bar, Baz, Quux>{}

fn foobar<U: Clone, T: Foo<u8, U, u32>>() -> T {

}

#[on_unimplemented="a collection of type `{Self}` cannot be built from an iterator over elements of type `{A}`"]
trait MyFromIterator<A> {
/// Build a container with elements from an external iterator.
fn my_from_iter<T: Iterator<Item=A>>(iterator: T) -> Self;
}

fn collect<A, I: Iterator<Item=A>, B: MyFromIterator<A>>(it: I) -> B {
MyFromIterator::my_from_iter(it)
}

#[on_unimplemented] //~ ERROR this attribute must have a value
trait BadAnnotation1 {}

#[on_unimplemented = "Unimplemented trait error on `{Self}` with params `<{A},{B},{C}>`"]
//~^ ERROR there is no type parameter C on trait BadAnnotation2<A, B>
trait BadAnnotation2<A,B> {}

fn trigger1<T: BadAnnotation1>(t: T) {}
fn trigger2<A, B, T: BadAnnotation2<A,B>>(t: T) {}

pub fn main() {
let x = vec!(1u8, 2, 3, 4);
let y: Option<Vec<u8>> = collect(x.iter()); // this should give approximately the same error for x.iter().collect()
//~^ ERROR
//~^^ NOTE a collection of type `core::option::Option<collections::vec::Vec<u8>>` cannot be built from an iterator over elements of type `&u8`
let x: String = foobar(); //~ ERROR
//~^ NOTE test error `collections::string::String` with `u8` `_` `u32`

// The following two have errors in their annotations, so the regular error should be thrown
trigger1(1u8); //~ ERROR the trait `BadAnnotation1` is not implemented for the type `u8`
trigger2::<u8, u8, u8>(1u8); //~ ERROR the trait `BadAnnotation2<u8, u8>` is not implemented

}