improve span of erroneous empty macro invocation

[TimNN]

The ideas is to use the span of the complete macro invocation if the span of a macro error is DUMMY_SP.

fixes #7970

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[TimNN]

Don't know how this got closed, but it shouldn't be.

[huonw]

Maybe this function could be changed to take in a substitute_span: Span and have the DUMMY_SP comparison internally?

[TimNN]

Sure, I choose to remove it because

1. It was only called in one place
2. It only forwarded it's arguments to parse and panicked on failure

Taking this into account I don't think the existence of this function is really justified, especially since I would have had to add another parameter to it.

[huonw]

Your reasoning sounds good to me.

[huonw]

I wonder if Span could get a method:

fn substitute_dummy(self, other: Span) -> Span {
    if *self == DUMMY_SP { other } else { *self }
}

Which can be used like:

Error(err_sp, ref msg) => panic!(cx.span_fatal(err_sp.substitute_dummy(sp), msg))

[huonw]

Thanks for tackling this, it's definitely annoying.

[TimNN]

Regarding substitute_dummy, I've gone over all the other DUMMY_SP usages, and such a method would only be useful in two other places:

• https://github.com/rust-lang/rust/blob/master/src/librustc/middle/traits/error_reporting.rs#L84
• https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/mod.rs#L210

which again does not seem to justify adding such a method to me.

[huonw]

Your choice, but I do think abstracting out this pattern is a good way to encourage avoiding printing DUMMY_SP even if it isn't used that much right now (even 3 uses seems like enough to justify it for me).

[TimNN]

Alright, should I add the method and change the other usages in this pull request or does that require another?

[huonw]

I think doing it in this PR is fine.

[TimNN]

I added the suggested method and have to admit that the code looks cleaner that way :)

[huonw]

@bors r+

Thanks!

[bors]

📌 Commit d46e840 has been approved by huonw

[bors]

⌛ Testing commit d46e840 with merge 58b0aa5...

[bors]

💔 Test failed - auto-linux-32-opt

[alexcrichton]

@bors: retry

On Tue, Aug 11, 2015 at 5:39 PM, bors [email protected] wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/6069

—
Reply to this email directly or view it on GitHub
#27584 (comment).

[bors]

⚡ Previous build results for auto-linux-64-nopt-t, auto-linux-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-x-android-t, auto-win-gnu-32-nopt-t, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt