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

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@nnethercote
Copy link
Contributor

nnethercote commented Feb 8, 2019

Because it's always the same as the Span for the surrounding Expr.

This reduces instruction counts on various benchmarks by up to 0.5%.

Don't store a `Span` for `ExprKind::Lit` in the AST.
Because it's always the same as the `Span` for the surrounding `Expr`.

This reduces instruction counts on various benchmarks by up to 0.5%.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 8, 2019

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

{ let hi = self.prev_span;
let expr_span = lo.to(hi);
expr_span
});

This comment has been minimized.

@petrochenkov

petrochenkov Feb 8, 2019

Contributor

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

This comment has been minimized.

@petrochenkov

petrochenkov Feb 8, 2019

Contributor

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.

This comment has been minimized.

@petrochenkov

petrochenkov Feb 8, 2019

Contributor

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.

This comment has been minimized.

@nnethercote

nnethercote Feb 8, 2019

Author Contributor

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.

This comment has been minimized.

@petrochenkov

petrochenkov Feb 9, 2019

Contributor

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.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2019

☔️ The latest upstream changes (presumably #58341) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment