Remove duplicate impl of string unescape from parse_format

[hkBst]

r? @nnethercote

[rustbot]

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

[nnethercote]

This is a large (+447/−547) change with zero explanation. I need some context and motivation, please! Also, from skimming it I think there might be multiple distinct changes in the single commit? If so, it would be easier to review if they were separate.

[hkBst]

This is a large (+447/−547) change with zero explanation. I need some context and motivation, please!

Ah, sorry about that. Let me try and fix that:

The idea for this comes from this code at the bottom of rustc_parse_format/lib.rs:

fn unescape_string(string: &str) -> Option<String> {
    let mut buf = String::new();
    let mut ok = true;
    unescape::unescape_unicode(string, unescape::Mode::Str, &mut |_, unescaped_char| {
        match unescaped_char {
            Ok(c) => buf.push(c),
            Err(_) => ok = false,
        }
    });

    ok.then_some(buf)
}

which does unescaping but throws away all span information from the original string (via the _ in &mut |_, unescaped_char|. This function is called in fn find_width_map_from_snippet:

    let Some(unescaped) = unescape_string(snippet) else {
        return InputStringKind::NotALiteral;
    };

which then does its own light version of string unescape to build a width map (if the unescaped string matches the input string), which is basically a list of expansions from the unescaped string back to the original string, which has to be traversed to determine the old position (this happens in fn remap_pos, fn to_span_index, fn to_span_width, and fn span). (Doing a Vec traversal for each original position that is needed is quadratic behavior as it is linear in both the length of the width map Vec and the number of such translations from new to old position.)

The new code does the unescaping in Parser::new, while collecting the position information into a Vec, and checking that the unescaped string matches the input string like so:

                // snippet is not a raw string
                if snippet.starts_with('"') {
                    // snippet looks like an ordinary string literal
                    // check whether it is the escaped version of input
                    let without_quotes = &snippet[1..snippet.len() - 1];
                    let (mut ok, mut vec) = (true, vec![]);
                    let mut chars = input.chars();
                    unescape::unescape_unicode(
                        without_quotes,
                        unescape::Mode::Str,
                        &mut |range, res| match res {
                            Ok(ch) if ok && chars.next().is_some_and(|c| ch == c) => {
                                vec.push((range, ch));
                            }
                            _ => {
                                ok = false;
                                vec = vec![];
                            }
                        },
                    );

Here we're feeling some pain from the callback-based nature of unescaping, which forces the collection of span info into a Vec (at least I could not see a good alternative). Basically, the Parser needs to know the position of each character in input (Peekable<Char<>> in the current code) and in the original string as typed (snippet) (width_map plus translation functions in the current code). This new code ultimately collects a Vec<(original span, char byte pos in input, char in input)> for the same purpose. It is thus probably using more memory.

Most of the other changes are because of this change of span info representation.

Also, from skimming it I think there might be multiple distinct changes in the single commit?

It is possible, but this code went through a lot of iterations, as I came to understand the exact constraints imposed by the ui tests, and this is basically the first version that passes all those ui tests.

There are a few minor things that come to mind:

• I ended up inlining fn self.suggest_format_parameter into its single use, since most of the code was just duplicate work. I'm not sure if that is also possible with the old way of handling the span info.
• I also ended up inlining err and err_with_note, one of which had a single use, and the other two or three uses, since they did not seem to be carrying their weight.

I'm hoping this is enough to get the broad idea, such that you can start asking about specific bits of this change, but let me know if there is more you need or that I can do to clarify.

[hkBst]

Given the changes in work done and the probable increase in memory use:
@bors try @rust-timer queue

[bors]

@hkBst: 🔑 Insufficient privileges: not in try users

[nnethercote]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 94fb87a with merge fe03ab0...

[bors]

☀️ Try build successful - checks-actions
Build commit: fe03ab0 (fe03ab008f4474bd7092d967e9eb28cfe09d0664)

[rust-timer]

Finished benchmarking commit (fe03ab0): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 777.245s -> 777.886s (0.08%)
Artifact size: 362.11 MiB -> 362.15 MiB (0.01%)

[nnethercote]

Sorry for the slow review. This looks good. A few nits to address. I didn't follow every single detail, but it looks like a clear simplification. The removal of InnerSpan, InputStringKind and InnerWidthMapping in particular are good.

You inlined a few functions, it would have been good to do them in separate commits. Also I wonder if the InnerSpan-to-Range change could have been done in its own commit, before the other changes. (Just thinking out loud; it's always a good idea to split up changes into multiple commits where possible, to make life easier for the reviewer.)

[nnethercote]

This change is unnecessary, AFAICT.

[hkBst]

Indeed, change removed.

[nnethercote]

If you use #![feature(new_range_api)] you can use the new experimental core::range::Range type, which implements Copy. That would avoid some clone calls you've had to add.

[lnicola]

We are using this crate in rust-analyzer, so we'd appreciate if it kept building on stable.

[hkBst]

Given, @lnicola's objection I'll leave this for now.

[nnethercote]

start_idx or start_index would be more idiomatic for this code base.

[hkBst]

Changed

[nnethercote]

This could have a better name, one that indicates what it indexes into. Is it input_vec? If so, input_vec_index would be appropriate.

[nnethercote]

Comments on these new fields (and maybe input) would also be helpful.

[hkBst]

Name changed and comments added.

[nnethercote]

@rustbot author

[hkBst]

Sorry for the slow review. This looks good. A few nits to address. I didn't follow every single detail, but it looks like a clear simplification. The removal of InnerSpan, InputStringKind and InnerWidthMapping in particular are good.

No problem. Glad we're in agreement here.

You inlined a few functions, it would have been good to do them in separate commits. Also I wonder if the InnerSpan-to-Range change could have been done in its own commit, before the other changes. (Just thinking out loud; it's always a good idea to split up changes into multiple commits where possible, to make life easier for the reviewer.)

Thanks for the advice. It is good to get your perspective. I'll try to be more mindful of the reviewer's job. Thanks for reviewing!

[hkBst]

@rustbot ready

[nnethercote]

@bors r+

[bors]

📌 Commit 4711153 has been approved by nnethercote

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4711153 with merge 4652ee6...

[jieyouxu]

Sorry, perf and CI LLVM is a bit broken atm. Please re-approve once bootstrap & perf is fixed.
@bors retry r-

[hkBst]

@rustbot ready

[bors]

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