diff options
| author | Matthias Krüger <matthias.krueger@famsik.de> | 2025-02-14 16:23:32 +0100 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-02-14 16:23:32 +0100 |
| commit | 49fb61c4961bb0244abe708d24a5e206a96f5304 (patch) | |
| tree | 03482cdae92844b955eb8b0b1ee2d9866129b2c7 /compiler/rustc_errors/src | |
| parent | 2980f75713c9ed59dbcdd2f7a272cc13755085ce (diff) | |
| parent | 6d71251cf9e40326461f90f8ff9a7024706aea87 (diff) | |
| download | rust-49fb61c4961bb0244abe708d24a5e206a96f5304.tar.gz rust-49fb61c4961bb0244abe708d24a5e206a96f5304.zip | |
Rollup merge of #136958 - compiler-errors:additive-replacmeent, r=estebank
Fix presentation of purely "additive" replacement suggestion parts #127541 changes replacement suggestions to use the "diff" view always, which I think is really verbose in cases where a replacement snippet is a "superset" of the snippet that is being replaced. Consider: ``` LL - Self::Baz: Clone, LL + Self::Baz: Clone, T: std::clone::Clone ``` In this code, we suggest replacing `", "` with `", T: std::clone::Clone"`. This is a consequence of how the snippet is constructed. I believe that since the string that is being replaced is a subset of the replacement string, it's not providing much value to present this as a diff. Users should be able to clearly understand what's being suggested here using the `~` underline view we've been suggesting for some time now. Given that this affects ~100 tests out of the ~1000 UI tests affected, I expect this to be a pretty meaningful improvement of the fallout of #127541. --- In the last commit, this PR also "trims" replacement parts so that they are turned into their purely additive subset, if possible. See the diff for what this means. --- r? estebank
Diffstat (limited to 'compiler/rustc_errors/src')
| -rw-r--r-- | compiler/rustc_errors/src/emitter.rs | 25 | ||||
| -rw-r--r-- | compiler/rustc_errors/src/lib.rs | 30 |
2 files changed, 48 insertions, 7 deletions
diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 4824dc098ad..634afacf539 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -1976,13 +1976,16 @@ impl HumanEmitter { Some(Style::HeaderMsg), ); + let other_suggestions = suggestions.len().saturating_sub(MAX_SUGGESTIONS); + let mut row_num = 2; for (i, (complete, parts, highlights, _)) in - suggestions.iter().enumerate().take(MAX_SUGGESTIONS) + suggestions.into_iter().enumerate().take(MAX_SUGGESTIONS) { debug!(?complete, ?parts, ?highlights); - let has_deletion = parts.iter().any(|p| p.is_deletion(sm) || p.is_replacement(sm)); + let has_deletion = + parts.iter().any(|p| p.is_deletion(sm) || p.is_destructive_replacement(sm)); let is_multiline = complete.lines().count() > 1; if i == 0 { @@ -2167,7 +2170,7 @@ impl HumanEmitter { self.draw_code_line( &mut buffer, &mut row_num, - highlight_parts, + &highlight_parts, line_pos + line_start, line, show_code_change, @@ -2213,7 +2216,12 @@ impl HumanEmitter { if let DisplaySuggestion::Diff | DisplaySuggestion::Underline | DisplaySuggestion::Add = show_code_change { - for part in parts { + for mut part in parts { + // If this is a replacement of, e.g. `"a"` into `"ab"`, adjust the + // suggestion and snippet to look as if we just suggested to add + // `"b"`, which is typically much easier for the user to understand. + part.trim_trivial_replacements(sm); + let snippet = if let Ok(snippet) = sm.span_to_snippet(part.span) { snippet } else { @@ -2376,9 +2384,12 @@ impl HumanEmitter { row_num = row + 1; } } - if suggestions.len() > MAX_SUGGESTIONS { - let others = suggestions.len() - MAX_SUGGESTIONS; - let msg = format!("and {} other candidate{}", others, pluralize!(others)); + if other_suggestions > 0 { + let msg = format!( + "and {} other candidate{}", + other_suggestions, + pluralize!(other_suggestions) + ); buffer.puts(row_num, max_line_num_len + 3, &msg, Style::NoStyle); } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 9af17db9a6e..8ff5dc12596 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -230,10 +230,40 @@ impl SubstitutionPart { !self.snippet.is_empty() && self.replaces_meaningful_content(sm) } + /// Whether this is a replacement that overwrites source with a snippet + /// in a way that isn't a superset of the original string. For example, + /// replacing "abc" with "abcde" is not destructive, but replacing it + /// it with "abx" is, since the "c" character is lost. + pub fn is_destructive_replacement(&self, sm: &SourceMap) -> bool { + self.is_replacement(sm) + && !sm.span_to_snippet(self.span).is_ok_and(|snippet| { + self.snippet.trim_start().starts_with(snippet.trim_start()) + || self.snippet.trim_end().ends_with(snippet.trim_end()) + }) + } + fn replaces_meaningful_content(&self, sm: &SourceMap) -> bool { sm.span_to_snippet(self.span) .map_or(!self.span.is_empty(), |snippet| !snippet.trim().is_empty()) } + + /// Try to turn a replacement into an addition when the span that is being + /// overwritten matches either the prefix or suffix of the replacement. + fn trim_trivial_replacements(&mut self, sm: &SourceMap) { + if self.snippet.is_empty() { + return; + } + let Ok(snippet) = sm.span_to_snippet(self.span) else { + return; + }; + if self.snippet.starts_with(&snippet) { + self.span = self.span.shrink_to_hi(); + self.snippet = self.snippet[snippet.len()..].to_string(); + } else if self.snippet.ends_with(&snippet) { + self.span = self.span.shrink_to_lo(); + self.snippet = self.snippet[..self.snippet.len() - snippet.len()].to_string(); + } + } } impl CodeSuggestion { |
