Use Symbol, Span in libfmt_macros #61568
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
src/libfmt_macros/lib.rs
Outdated
| let curr_last_brace = self.last_opening_brace; | ||
| let byte_pos = self.to_span_index(pos); | ||
| self.last_opening_brace = | ||
| Some(Span::new(byte_pos, byte_pos, SyntaxContext::empty())); |
There was a problem hiding this comment.
I think this needs to be Span::new(byte_pos, byte_pos + 1, SyntaxContext::empty()) if we want the Span to point at the } and not the space between whatever comes right before the brace and the brace itself.
There was a problem hiding this comment.
I'll wait to do anything here pending resolution of whether we want to be using Spans at all, though I think this is strictly "the same" as what we had before in any case so it's at least consistent if, well, wrong. :)
There was a problem hiding this comment.
Hurray for uncovering subtle bugs that would otherwise had gone undetected.
| if let Some((label, start, end)) = err.secondary_label { | ||
| let sp = fmt.span.from_inner_byte_pos(start.unwrap(), end.unwrap()); | ||
| if let Some((label, span)) = err.secondary_label { | ||
| let sp = fmt.span.from_inner(span); |
There was a problem hiding this comment.
I'm not sure whether this makes sense. span should already be based of the codemap, right? If not, then I'm not sure we should be using Spans there to avoid mixing Spans that are valid in some contexts and not others.
There was a problem hiding this comment.
It looks like the machinery in libfmt_macros is used from two places in the compiler: parsing format_args! literals in println, format, etc. and in parsing the attribute to rustc_on_unimplemented. The first has the relevant span information AFAICT fully, but the latter we don't appear to have good spans for it looks like we just have the span for all of rustc_on_unimplemented = "..." not just the "..." bit.
I'm happy to revert the bits which move libfmt_macros to using "fake" spans and back to (SpanIndex, SpanIndex). I don't feel great about that since it seems worse than the inner spans.
There was a problem hiding this comment.
Could we new type wrap them? That way we won't be able to accidentally pass an InnerSpan somewhere a Span is expected. At the very least we should add generous comments about what's happening here. You're right that it seems better than the status quo.
There was a problem hiding this comment.
I'll do that. I agree that just Span is probably not great. I'll also try to eliminate the other uses of from_inner_byte_pos.
Mark-Simulacrum
force-pushed
the
symbol-fmt-macros
branch
from
a66c29e
to
966754a
Compare
|
Okay, I've updated with new code and an |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
symbol-fmt-macros
branch
from
c024dde
to
f99f465
Compare
|
Rebased and hopefully fixed the test failure. |
This comment has been minimized.
This comment has been minimized.
This should be used when trying to get at subsets of a larger span, especially when the larger span is not available in the code attempting to work with those subsets (especially common in the fmt_macros crate). This is usually a good replacement for (BytePos, BytePos) and (usize, usize) tuples. This commit also removes from_inner_byte_pos, since it took usize arguments, which is error prone.
Mark-Simulacrum
force-pushed
the
symbol-fmt-macros
branch
from
f99f465
to
45df52f
Compare
|
@bors r+ |
|
📌 Commit 45df52f has been approved by |
bors
added
S-waiting-on-bors
…r=estebank Use Symbol, Span in libfmt_macros I'm not super happy with this, personally, but I think it might be a decent start -- happy to take suggestions as to how to expand this or change things further. r? @estebank Fixes rust-lang#60795
Rollup of 9 pull requests Successful merges: - #60187 (Generator optimization: Overlap locals that never have storage live at the same time) - #61348 (Implement Clone::clone_from for Option and Result) - #61568 (Use Symbol, Span in libfmt_macros) - #61632 (ci: Collect CPU usage statistics on Azure) - #61654 (use pattern matching for slices destructuring) - #61671 (implement nth_back for Range(Inclusive)) - #61688 (is_fp and is_floating_point do the same thing, remove the former) - #61705 (Pass cflags rather than cxxflags to LLVM as CMAKE_C_FLAGS) - #61734 (Migrate rust-by-example to MdBook2) Failed merges: r? @ghost
I'm not super happy with this, personally, but I think it might be a decent start -- happy to take suggestions as to how to expand this or change things further.
r? @estebank
Fixes #60795