Skip to content

Commit

Permalink
Use more precise span when reporting semicolon hint
Browse files Browse the repository at this point in the history
When reporting "consider removing this semicolon" hint message, the
offending semicolon may come from macro call site instead of macro
itself. Using the more appropriate span makes the hint more helpful.

Closes rust-lang#13428.
  • Loading branch information
edwardw committed Apr 18, 2014
1 parent ce2bab6 commit cc5be28
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 18 deletions.
9 changes: 5 additions & 4 deletions src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ use std::rc::Rc;
use std::str;
use std::uint;
use syntax::ast::*;
use syntax::codemap::Span;
use syntax::codemap::{BytePos, original_sp, Span};
use syntax::parse::token::special_idents;
use syntax::parse::token;
use syntax::print::pprust::{expr_to_str, block_to_str};
Expand Down Expand Up @@ -1473,10 +1473,11 @@ impl<'a> Liveness<'a> {
};
if ends_with_stmt {
let last_stmt = body.stmts.last().unwrap();
let original_span = original_sp(last_stmt.span, sp);
let span_semicolon = Span {
lo: last_stmt.span.hi,
hi: last_stmt.span.hi,
expn_info: last_stmt.span.expn_info
lo: original_span.hi - BytePos(1),
hi: original_span.hi,
expn_info: original_span.expn_info
};
self.ir.tcx.sess.span_note(
span_semicolon, "consider removing this semicolon:");
Expand Down
11 changes: 11 additions & 0 deletions src/libsyntax/codemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,17 @@ pub fn mk_sp(lo: BytePos, hi: BytePos) -> Span {
Span {lo: lo, hi: hi, expn_info: None}
}

/// Return the span itself if it doesn't come from a macro expansion,
/// otherwise return the call site span up to the `enclosing_sp` by
/// following the `expn_info` chain.
pub fn original_sp(sp: Span, enclosing_sp: Span) -> Span {
match (sp.expn_info, enclosing_sp.expn_info) {
(None, _) => sp,
(Some(expn1), Some(expn2)) if expn1.call_site == expn2.call_site => sp,
(Some(expn1), _) => original_sp(expn1.call_site, enclosing_sp),
}
}

/// A source code location used for error reporting
pub struct Loc {
/// Information about the original source
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,7 @@ mod test {
}),
span: sp(17,18)},
ast::DUMMY_NODE_ID),
span: sp(17,18)}),
span: sp(17,19)}),
expr: None,
id: ast::DUMMY_NODE_ID,
rules: ast::DefaultBlock, // no idea
Expand Down
24 changes: 11 additions & 13 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3260,9 +3260,14 @@ impl<'a> Parser<'a> {
match self.token {
token::SEMI => {
self.bump();
let span_with_semi = Span {
lo: stmt.span.lo,
hi: self.last_span.hi,
expn_info: stmt.span.expn_info,
};
stmts.push(@codemap::Spanned {
node: StmtSemi(e, stmt_id),
span: stmt.span,
span: span_with_semi,
});
}
token::RBRACE => {
Expand All @@ -3275,33 +3280,26 @@ impl<'a> Parser<'a> {
}
StmtMac(ref m, _) => {
// statement macro; might be an expr
let has_semi;
match self.token {
token::SEMI => {
has_semi = true;
self.bump();
stmts.push(@codemap::Spanned {
node: StmtMac((*m).clone(), true),
span: stmt.span,
});
}
token::RBRACE => {
// if a block ends in `m!(arg)` without
// a `;`, it must be an expr
has_semi = false;
expr = Some(
self.mk_mac_expr(stmt.span.lo,
stmt.span.hi,
m.node.clone()));
}
_ => {
has_semi = false;
stmts.push(stmt);
}
}

if has_semi {
self.bump();
stmts.push(@codemap::Spanned {
node: StmtMac((*m).clone(), true),
span: stmt.span,
});
}
}
_ => { // all other kinds of statements:
stmts.push(stmt);
Expand Down
26 changes: 26 additions & 0 deletions src/test/compile-fail/issue-13428.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// 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.

// Regression test for #13428

fn foo() -> ~str { //~ ERROR not all control paths return a value
format!("Hello {}",
"world")
// Put the trailing semicolon on its own line to test that the
// note message gets the offending semicolon exactly
; //~ NOTE consider removing this semicolon
}

fn bar() -> ~str { //~ ERROR not all control paths return a value
"foobar".to_owned()
; //~ NOTE consider removing this semicolon
}

pub fn main() {}

1 comment on commit cc5be28

@brson
Copy link

@brson brson commented on cc5be28 Apr 21, 2014

Choose a reason for hiding this comment

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

r+ thanks!

Please sign in to comment.