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

syntax: recovery for incorrect associated item paths like [T; N]::clone #46788

Merged
merged 1 commit into from
Dec 17, 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
94 changes: 94 additions & 0 deletions src/libsyntax/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use syntax_pos::{Span, DUMMY_SP};
use codemap::{respan, Spanned};
use abi::Abi;
use ext::hygiene::{Mark, SyntaxContext};
use parse::parser::{RecoverQPath, PathStyle};
use print::pprust;
use ptr::P;
use rustc_data_structures::indexed_vec;
Expand Down Expand Up @@ -519,6 +520,38 @@ impl Pat {
}
}

impl RecoverQPath for Pat {
fn to_ty(&self) -> Option<P<Ty>> {
let node = match &self.node {
PatKind::Wild => TyKind::Infer,
PatKind::Ident(BindingMode::ByValue(Mutability::Immutable), ident, None) =>
TyKind::Path(None, Path::from_ident(ident.span, ident.node)),
PatKind::Path(qself, path) => TyKind::Path(qself.clone(), path.clone()),
PatKind::Mac(mac) => TyKind::Mac(mac.clone()),
PatKind::Ref(pat, mutbl) =>
pat.to_ty().map(|ty| TyKind::Rptr(None, MutTy { ty, mutbl: *mutbl }))?,
PatKind::Slice(pats, None, _) if pats.len() == 1 =>
pats[0].to_ty().map(TyKind::Slice)?,
PatKind::Tuple(pats, None) => {
let mut tys = Vec::new();
for pat in pats {
tys.push(pat.to_ty()?);
}
TyKind::Tup(tys)
}
_ => return None,
};

Some(P(Ty { node, id: self.id, span: self.span }))
}
fn to_recovered(&self, qself: Option<QSelf>, path: Path) -> Self {
Self { span: path.span, node: PatKind::Path(qself, path), id: self.id }
}
fn to_string(&self) -> String {
pprust::pat_to_string(self)
}
}

/// A single field in a struct pattern
///
/// Patterns like the fields of Foo `{ x, ref y, ref mut z }`
Expand Down Expand Up @@ -877,6 +910,54 @@ impl Expr {
true
}
}

fn to_bound(&self) -> Option<TyParamBound> {
match &self.node {
ExprKind::Path(None, path) =>
Some(TraitTyParamBound(PolyTraitRef::new(Vec::new(), path.clone(), self.span),
TraitBoundModifier::None)),
_ => None,
}
}
}

impl RecoverQPath for Expr {
fn to_ty(&self) -> Option<P<Ty>> {
let node = match &self.node {
ExprKind::Path(qself, path) => TyKind::Path(qself.clone(), path.clone()),
ExprKind::Mac(mac) => TyKind::Mac(mac.clone()),
ExprKind::Paren(expr) => expr.to_ty().map(TyKind::Paren)?,
ExprKind::AddrOf(mutbl, expr) =>
expr.to_ty().map(|ty| TyKind::Rptr(None, MutTy { ty, mutbl: *mutbl }))?,
ExprKind::Repeat(expr, expr_len) =>
expr.to_ty().map(|ty| TyKind::Array(ty, expr_len.clone()))?,
ExprKind::Array(exprs) if exprs.len() == 1 =>
exprs[0].to_ty().map(TyKind::Slice)?,
ExprKind::Tup(exprs) => {
let mut tys = Vec::new();
for expr in exprs {
tys.push(expr.to_ty()?);
}
TyKind::Tup(tys)
}
ExprKind::Binary(binop, lhs, rhs) if binop.node == BinOpKind::Add =>
if let (Some(lhs), Some(rhs)) = (lhs.to_bound(), rhs.to_bound()) {
TyKind::TraitObject(vec![lhs, rhs], TraitObjectSyntax::None)
} else {
return None;
}
_ => return None,
};

Some(P(Ty { node, id: self.id, span: self.span }))
}
fn to_recovered(&self, qself: Option<QSelf>, path: Path) -> Self {
Self { span: path.span, node: ExprKind::Path(qself, path),
id: self.id, attrs: self.attrs.clone() }
}
fn to_string(&self) -> String {
pprust::expr_to_string(self)
}
}

impl fmt::Debug for Expr {
Expand Down Expand Up @@ -1388,6 +1469,19 @@ pub struct Ty {
pub span: Span,
}

impl RecoverQPath for Ty {
fn to_ty(&self) -> Option<P<Ty>> {
Some(P(self.clone()))
}
fn to_recovered(&self, qself: Option<QSelf>, path: Path) -> Self {
Self { span: path.span, node: TyKind::Path(qself, path), id: self.id }
}
fn to_string(&self) -> String {
pprust::ty_to_string(self)
}
const PATH_STYLE: PathStyle = PathStyle::Type;
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 it is customary to have the consts at the beginning of an impl, but don't bother to change it here, this is short enough to not make a difference :)

}

impl fmt::Debug for Ty {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "type({})", pprust::ty_to_string(self))
Expand Down
1 change: 1 addition & 0 deletions src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#![feature(unicode)]
#![feature(rustc_diagnostic_macros)]
#![feature(match_default_bindings)]
#![feature(i128_type)]

// See librustc_cratesio_shim/Cargo.toml for a comment explaining this.
Expand Down
70 changes: 50 additions & 20 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,13 @@ enum PrevTokenKind {
Other,
}

pub(crate) trait RecoverQPath: Sized {
fn to_ty(&self) -> Option<P<Ty>>;
fn to_recovered(&self, qself: Option<QSelf>, path: ast::Path) -> Self;
fn to_string(&self) -> String;
const PATH_STYLE: PathStyle = PathStyle::Expr;
}

/* ident is handled by common.rs */

#[derive(Clone)]
Expand Down Expand Up @@ -1567,6 +1574,7 @@ impl<'a> Parser<'a> {

// Try to recover from use of `+` with incorrect priority.
self.maybe_recover_from_bad_type_plus(allow_plus, &ty)?;
let ty = self.maybe_recover_from_bad_qpath(ty)?;

Ok(P(ty))
}
Expand Down Expand Up @@ -1621,6 +1629,32 @@ impl<'a> Parser<'a> {
Ok(())
}

// Try to recover from associated item paths like `[T]::AssocItem`/`(T, U)::AssocItem`.
fn maybe_recover_from_bad_qpath<T: RecoverQPath>(&mut self, base: T) -> PResult<'a, T> {
// Do not add `::` to expected tokens.
if self.token != token::ModSep {
return Ok(base);
}
let ty = match base.to_ty() {
Some(ty) => ty,
None => return Ok(base),
};

self.bump(); // `::`
let mut segments = Vec::new();
self.parse_path_segments(&mut segments, T::PATH_STYLE, true)?;

let span = ty.span.to(self.prev_span);
let recovered =
base.to_recovered(Some(QSelf { ty, position: 0 }), ast::Path { segments, span });

self.diagnostic()
.struct_span_err(span, "missing angle brackets in associated item path")
.span_suggestion(span, "try", recovered.to_string()).emit();

Ok(recovered)
}

fn parse_borrowed_pointee(&mut self) -> PResult<'a, TyKind> {
let opt_lifetime = if self.check_lifetime() { Some(self.expect_lifetime()) } else { None };
let mutbl = self.parse_mutability();
Expand Down Expand Up @@ -2012,12 +2046,7 @@ impl<'a> Parser<'a> {
}

pub fn mk_expr(&mut self, span: Span, node: ExprKind, attrs: ThinVec<Attribute>) -> P<Expr> {
P(Expr {
id: ast::DUMMY_NODE_ID,
node,
span,
attrs: attrs.into(),
})
P(Expr { node, span, attrs, id: ast::DUMMY_NODE_ID })
}

pub fn mk_unary(&mut self, unop: ast::UnOp, expr: P<Expr>) -> ast::ExprKind {
Expand Down Expand Up @@ -2139,12 +2168,11 @@ impl<'a> Parser<'a> {
self.bump();

hi = self.prev_span;
let span = lo.to(hi);
return if es.len() == 1 && !trailing_comma {
Ok(self.mk_expr(span, ExprKind::Paren(es.into_iter().nth(0).unwrap()), attrs))
ex = if es.len() == 1 && !trailing_comma {
ExprKind::Paren(es.into_iter().nth(0).unwrap())
} else {
Ok(self.mk_expr(span, ExprKind::Tup(es), attrs))
}
ExprKind::Tup(es)
};
}
token::OpenDelim(token::Brace) => {
return self.parse_block_expr(lo, BlockCheckMode::Default, attrs);
Expand Down Expand Up @@ -2344,7 +2372,10 @@ impl<'a> Parser<'a> {
}
}

return Ok(self.mk_expr(lo.to(hi), ex, attrs));
let expr = Expr { node: ex, span: lo.to(hi), id: ast::DUMMY_NODE_ID, attrs };
let expr = self.maybe_recover_from_bad_qpath(expr)?;

return Ok(P(expr));
}

fn parse_struct_expr(&mut self, lo: Span, pth: ast::Path, mut attrs: ThinVec<Attribute>)
Expand Down Expand Up @@ -3405,7 +3436,7 @@ impl<'a> Parser<'a> {

if self.check(&token::Comma) ||
self.check(&token::CloseDelim(token::Bracket)) {
slice = Some(P(ast::Pat {
slice = Some(P(Pat {
id: ast::DUMMY_NODE_ID,
node: PatKind::Wild,
span: self.span,
Expand Down Expand Up @@ -3492,14 +3523,14 @@ impl<'a> Parser<'a> {
(false, false) => BindingMode::ByValue(Mutability::Immutable),
};
let fieldpath = codemap::Spanned{span:self.prev_span, node:fieldname};
let fieldpat = P(ast::Pat{
let fieldpat = P(Pat {
id: ast::DUMMY_NODE_ID,
node: PatKind::Ident(bind_type, fieldpath, None),
span: boxed_span.to(hi),
});

let subpat = if is_box {
P(ast::Pat{
P(Pat {
id: ast::DUMMY_NODE_ID,
node: PatKind::Box(fieldpat),
span: lo.to(hi),
Expand Down Expand Up @@ -3708,11 +3739,10 @@ impl<'a> Parser<'a> {
}
}

Ok(P(ast::Pat {
id: ast::DUMMY_NODE_ID,
node: pat,
span: lo.to(self.prev_span),
}))
let pat = Pat { node: pat, span: lo.to(self.prev_span), id: ast::DUMMY_NODE_ID };
let pat = self.maybe_recover_from_bad_qpath(pat)?;

Ok(P(pat))
}

/// Parse ident or ident @ pat
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/did_you_mean/bad-assoc-expr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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.

fn main() {
let a = [1, 2, 3, 4];
[i32; 4]::clone(&a);
//~^ ERROR missing angle brackets in associated item path

[i32]::as_ref(&a);
//~^ ERROR missing angle brackets in associated item path

(u8)::clone(&0);
//~^ ERROR missing angle brackets in associated item path

(u8, u8)::clone(&(0, 0));
//~^ ERROR missing angle brackets in associated item path
}
26 changes: 26 additions & 0 deletions src/test/ui/did_you_mean/bad-assoc-expr.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
error: missing angle brackets in associated item path
--> $DIR/bad-assoc-expr.rs:13:5
|
13 | [i32; 4]::clone(&a);
| ^^^^^^^^^^^^^^^ help: try: `<[i32; 4]>::clone`

error: missing angle brackets in associated item path
--> $DIR/bad-assoc-expr.rs:16:5
|
16 | [i32]::as_ref(&a);
| ^^^^^^^^^^^^^ help: try: `<[i32]>::as_ref`

error: missing angle brackets in associated item path
--> $DIR/bad-assoc-expr.rs:19:5
|
19 | (u8)::clone(&0);
| ^^^^^^^^^^^ help: try: `<(u8)>::clone`

error: missing angle brackets in associated item path
--> $DIR/bad-assoc-expr.rs:22:5
|
22 | (u8, u8)::clone(&(0, 0));
| ^^^^^^^^^^^^^^^ help: try: `<(u8, u8)>::clone`

error: aborting due to 4 previous errors

23 changes: 23 additions & 0 deletions src/test/ui/did_you_mean/bad-assoc-pat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// 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.

fn main() {
match 0u8 {
[u8]::AssocItem => {}
//~^ ERROR missing angle brackets in associated item path
//~| ERROR no associated item named `AssocItem` found for type `[u8]` in the current scope
(u8, u8)::AssocItem => {}
//~^ ERROR missing angle brackets in associated item path
//~| ERROR no associated item named `AssocItem` found for type `(u8, u8)` in the current sco
_::AssocItem => {}
//~^ ERROR missing angle brackets in associated item path
//~| ERROR no associated item named `AssocItem` found for type `_` in the current scope
}
}
38 changes: 38 additions & 0 deletions src/test/ui/did_you_mean/bad-assoc-pat.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error: missing angle brackets in associated item path
--> $DIR/bad-assoc-pat.rs:13:9
|
13 | [u8]::AssocItem => {}
| ^^^^^^^^^^^^^^^ help: try: `<[u8]>::AssocItem`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions are reserved for cases where our level of certainty that they're correct is very high (sometimes going to the length of continuing parsing to verify our theory). Because this error happens at the parser level, we don't know whether AssocItem is an associated item or not, so normally we would suggest using a span_label instead. Despite this, in this case I'm ok with the use of a span_suggestion, but I would reword it to something more descriptive, like "if you meant to use an associated item, surround the type with brackets:", or something shorter (long suggestion messages make the suggestion be presented on it's own).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, @oli-obk in the recent months moved everything containing code snippets to span_suggestion, regardless of suggestion reliability.
(FWIW, I'd also estimate this recovery as pretty reliable.)

"if you meant to use an associated item, surround the type with brackets:"

The primary message already says roughly this, so I avoided repeating it in the label (I also like really short labels in general).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on suggestion levels. Just make everything a suggestion and let me worry about the probablitity of it being correct

Copy link
Contributor

Choose a reason for hiding this comment

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

@oli-obk I've been thinking about making the error more structured, basically having a struct for each possible error the compiler generates, that way we can start playing with having the --explain content inline with the actual code being compiled (similar output to what Elm produces). This would require suggestions to be reworked slightly as well. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making the errors more explicit is definitely a good idea. Currently analysis code and error reporting is quite mangled. Maybe we can get it separated with this method.

The most important thing is that the diagnostics API stays as usable as it is now. Making it more complex will only unnecessarily hold up regular development

Copy link
Contributor

Choose a reason for hiding this comment

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

@oli-obk I definitely agree. The only thing that worries me is decoupling the place where the error is emitted and where it is "defined" in different files might lead to harder maintenance, but the way I'm thinking about it it should still allow the current ad-hoc DiagnosticBuilder usage to keep the current flexibility (and allowing a gradual migration to these structured errors).


error: missing angle brackets in associated item path
--> $DIR/bad-assoc-pat.rs:16:9
|
16 | (u8, u8)::AssocItem => {}
| ^^^^^^^^^^^^^^^^^^^ help: try: `<(u8, u8)>::AssocItem`

error: missing angle brackets in associated item path
--> $DIR/bad-assoc-pat.rs:19:9
|
19 | _::AssocItem => {}
| ^^^^^^^^^^^^ help: try: `<_>::AssocItem`

error[E0599]: no associated item named `AssocItem` found for type `[u8]` in the current scope
--> $DIR/bad-assoc-pat.rs:13:9
|
13 | [u8]::AssocItem => {}
| ^^^^^^^^^^^^^^^ associated item not found in `[u8]`

error[E0599]: no associated item named `AssocItem` found for type `(u8, u8)` in the current scope
--> $DIR/bad-assoc-pat.rs:16:9
|
16 | (u8, u8)::AssocItem => {}
| ^^^^^^^^^^^^^^^^^^^ associated item not found in `(u8, u8)`

error[E0599]: no associated item named `AssocItem` found for type `_` in the current scope
--> $DIR/bad-assoc-pat.rs:19:9
|
19 | _::AssocItem => {}
| ^^^^^^^^^^^^ associated item not found in `_`

error: aborting due to 6 previous errors

Loading