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

Don't store a Span for ExprKind::Lit in the AST. #58284

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 1 addition & 1 deletion src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3852,7 +3852,7 @@ impl<'a> LoweringContext<'a> {
let ohs = P(self.lower_expr(ohs));
hir::ExprKind::Unary(op, ohs)
}
ExprKind::Lit(ref l) => hir::ExprKind::Lit((*l).clone()),
ExprKind::Lit(ref l) => hir::ExprKind::Lit(respan(e.span, (*l).clone())),
ExprKind::Cast(ref expr, ref ty) => {
let expr = P(self.lower_expr(expr));
hir::ExprKind::Cast(expr, self.lower_ty(ty, ImplTraitContext::disallowed()))
Expand Down
7 changes: 6 additions & 1 deletion src/librustc/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1450,7 +1450,12 @@ pub enum ExprKind {
Binary(BinOp, P<Expr>, P<Expr>),
/// A unary operation (For example: `!x`, `*x`)
Unary(UnOp, P<Expr>),
/// A literal (For example: `1`, `"foo"`)
/// A literal (For example: `1`, `"foo"`). Unlike the AST, HIR stores a
/// Span for the literal. This is because that Span may be different to the
/// Span of the enclosing expression, due to HIR not explicitly
/// representing parentheses around expressions. E.g. in the expression
/// "(1.0)", the expression's Span covers the whole thing, while the
/// literal's Span covers only "1.0".
Lit(Lit),
/// A cast (`foo as f64`)
Cast(P<Expr>, P<Ty>),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
<https://github.com/rust-lang/rust/issues/27779#issuecomment-378416911>"
);
match val.node {
ExprKind::Lit(ref v) if v.node.is_numeric() => {
ExprKind::Lit(ref v) if v.is_numeric() => {
err.span_suggestion(
place.span.between(val.span),
"if you meant to write a comparison against a negative value, add a \
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1076,7 +1076,7 @@ pub enum ExprKind {
/// A unary operation (e.g., `!x`, `*x`).
Unary(UnOp, P<Expr>),
/// A literal (e.g., `1`, `"foo"`).
Lit(Lit),
Lit(LitKind),
/// A cast (e.g., `foo as f64`).
Cast(P<Expr>, P<Ty>),
Type(P<Expr>, P<Ty>),
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/attr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ impl LitKind {
Token::Ident(ident, false) if ident.name == "false" => Some(LitKind::Bool(false)),
Token::Interpolated(ref nt) => match nt.0 {
token::NtExpr(ref v) | token::NtLiteral(ref v) => match v.node {
ExprKind::Lit(ref lit) => Some(lit.node.clone()),
ExprKind::Lit(ref lit) => Some(lit.clone()),
_ => None,
},
_ => None,
Expand Down
5 changes: 3 additions & 2 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -984,14 +984,15 @@ pub fn expr_to_spanned_string<'a>(
err_msg: &str,
) -> Result<Spanned<(Symbol, ast::StrStyle)>, Option<DiagnosticBuilder<'a>>> {
// Update `expr.span`'s ctxt now in case expr is an `include!` macro invocation.
let lit_span = expr.span;
expr.span = expr.span.apply_mark(cx.current_expansion.mark);

// we want to be able to handle e.g., `concat!("foo", "bar")`
cx.expander().visit_expr(&mut expr);
Err(match expr.node {
ast::ExprKind::Lit(ref l) => match l.node {
ast::ExprKind::Lit(ref l) => match *l {
ast::LitKind::Str(s, style) => return Ok(respan(expr.span, (s, style))),
_ => Some(cx.struct_span_err(l.span, err_msg))
_ => Some(cx.struct_span_err(lit_span, err_msg))
},
ast::ExprKind::Err => None,
_ => Some(cx.struct_span_err(expr.span, err_msg))
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/ext/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
}

fn expr_lit(&self, sp: Span, lit: ast::LitKind) -> P<ast::Expr> {
self.expr(sp, ast::ExprKind::Lit(respan(sp, lit)))
self.expr(sp, ast::ExprKind::Lit(lit))
}
fn expr_usize(&self, span: Span, i: usize) -> P<ast::Expr> {
self.expr_lit(span, ast::LitKind::Int(i as u128,
Expand Down
12 changes: 9 additions & 3 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1996,7 +1996,7 @@ impl<'a> Parser<'a> {
let out = match self.token {
token::Interpolated(ref nt) => match nt.0 {
token::NtExpr(ref v) | token::NtLiteral(ref v) => match v.node {
ExprKind::Lit(ref lit) => { lit.node.clone() }
ExprKind::Lit(ref lit) => { lit.clone() }
_ => { return self.unexpected_last(&self.token); }
},
_ => { return self.unexpected_last(&self.token); }
Expand Down Expand Up @@ -2079,8 +2079,14 @@ impl<'a> Parser<'a> {
let minus_present = self.eat(&token::BinOp(token::Minus));
let lo = self.span;
let literal = self.parse_lit()?;
let hi = self.prev_span;
let expr = self.mk_expr(lo.to(hi), ExprKind::Lit(literal), ThinVec::new());
// We don't store a span for the literal because it would be the same
// as the span for the expression. (Check this.)
debug_assert_eq!(literal.span,
{ let hi = self.prev_span;
let expr_span = lo.to(hi);
expr_span
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit suspicious.
Have you checked whether this assertion passes on literals passed to macros via literal and expr matchers?

Copy link
Contributor

Choose a reason for hiding this comment

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

For

macro m { ... let x = $literal + y; ... }

m!(10);

I'd expect the expression span to point to $literal, but the literal span to point to 10.

Copy link
Contributor

@petrochenkov petrochenkov Feb 8, 2019

Choose a reason for hiding this comment

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

For expressions (and other fragments) passed through multiple matchers it would be nice to keep all the "backtrace" of spans by wrapping fragments into "invisible parentheses" on each substitution, but this isn't generally done now.

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 ran the tests and they passed. (In contrast, I tried doing the same no-span-for-literals transformation for HIR and the tests failed; that's how I found out about HIR's lack of parentheses being a problem.)

More compellingly, if you look at the code you can see that I haven't changed behaviour at all: parse_lit uses self.span.to(self.prev_span) for the span, where self.span is obtained at the start of parse_lit and self.prev_span is obtained at the end. And the new code does the same.

Copy link
Contributor

@petrochenkov petrochenkov Feb 9, 2019

Choose a reason for hiding this comment

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

More compellingly, if you look at the code you can see that I haven't changed behaviour at all

OK.
This still goes into the wrong direction IMO, because the spans should ideally be different even if they are not right now.
I'll leave this to eddyb to decide, if AST is going to be radically rewritten for incremental parsing/expansion, then details like this won't matter anyway.

let expr = self.mk_expr(literal.span, ExprKind::Lit(literal.node), ThinVec::new());

if minus_present {
let minus_hi = self.prev_span;
Expand Down
12 changes: 8 additions & 4 deletions src/libsyntax/print/pprust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,11 +598,15 @@ pub trait PrintState<'a> {
}

fn print_literal(&mut self, lit: &ast::Lit) -> io::Result<()> {
self.maybe_print_comment(lit.span.lo())?;
if let Some(ltrl) = self.next_lit(lit.span.lo()) {
self.print_literal_kind(&lit.node, lit.span)
}

fn print_literal_kind(&mut self, lit: &ast::LitKind, span: syntax_pos::Span) -> io::Result<()> {
self.maybe_print_comment(span.lo())?;
if let Some(ltrl) = self.next_lit(span.lo()) {
return self.writer().word(ltrl.lit.clone());
}
match lit.node {
match *lit {
ast::LitKind::Str(st, style) => self.print_string(&st.as_str(), style),
ast::LitKind::Err(st) => {
let st = st.as_str().escape_debug();
Expand Down Expand Up @@ -2121,7 +2125,7 @@ impl<'a> State<'a> {
self.print_expr_addr_of(m, expr)?;
}
ast::ExprKind::Lit(ref lit) => {
self.print_literal(lit)?;
self.print_literal_kind(lit, expr.span)?;
}
ast::ExprKind::Cast(ref expr, ref ty) => {
let prec = AssocOp::As.precedence() as i8;
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax_ext/concat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub fn expand_syntax_ext(
let mut has_errors = false;
for e in es {
match e.node {
ast::ExprKind::Lit(ref lit) => match lit.node {
ast::ExprKind::Lit(ref lit) => match *lit {
ast::LitKind::Str(ref s, _)
| ast::LitKind::Err(ref s)
| ast::LitKind::Float(ref s, _)
Expand Down