Skip to content
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

Tweak some structured suggestions to be more verbose and accurate #127301

Merged
merged 9 commits into from
Jul 4, 2024
25 changes: 11 additions & 14 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3757,13 +3757,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
assigned_span: Span,
err_place: Place<'tcx>,
) {
let (from_arg, local_decl, local_name) = match err_place.as_local() {
Some(local) => (
self.body.local_kind(local) == LocalKind::Arg,
Some(&self.body.local_decls[local]),
self.local_names[local],
),
None => (false, None, None),
let (from_arg, local_decl) = match err_place.as_local() {
Some(local) => {
(self.body.local_kind(local) == LocalKind::Arg, Some(&self.body.local_decls[local]))
}
None => (false, None),
};

// If root local is initialized immediately (everything apart from let
Expand Down Expand Up @@ -3795,13 +3793,12 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
err.span_label(assigned_span, format!("first assignment to {place_description}"));
}
if let Some(decl) = local_decl
&& let Some(name) = local_name
&& decl.can_be_made_mutable()
{
err.span_suggestion(
decl.source_info.span,
err.span_suggestion_verbose(
decl.source_info.span.shrink_to_lo(),
"consider making this binding mutable",
format!("mut {name}"),
"mut ".to_string(),
Applicability::MachineApplicable,
);
if !from_arg
Expand All @@ -3813,10 +3810,10 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
}))
)
{
err.span_suggestion(
decl.source_info.span,
err.span_suggestion_verbose(
decl.source_info.span.shrink_to_lo(),
"to modify the original value, take a borrow instead",
format!("ref mut {name}"),
"ref mut ".to_string(),
Applicability::MaybeIncorrect,
);
}
Expand Down
29 changes: 16 additions & 13 deletions compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,21 +408,21 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
fn_decl.implicit_self,
hir::ImplicitSelfKind::RefImm | hir::ImplicitSelfKind::RefMut
) {
err.span_suggestion(
upvar_ident.span,
err.span_suggestion_verbose(
upvar_ident.span.shrink_to_lo(),
"consider changing this to be mutable",
format!("mut {}", upvar_ident.name),
"mut ",
Applicability::MachineApplicable,
);
break;
}
}
}
} else {
err.span_suggestion(
upvar_ident.span,
err.span_suggestion_verbose(
upvar_ident.span.shrink_to_lo(),
"consider changing this to be mutable",
format!("mut {}", upvar_ident.name),
"mut ",
Applicability::MachineApplicable,
);
}
Expand All @@ -449,8 +449,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
.is_ok_and(|snippet| snippet.starts_with("&mut ")) =>
{
err.span_label(span, format!("cannot {act}"));
err.span_suggestion(
span,
err.span_suggestion_verbose(
span.with_hi(span.lo() + BytePos(5)),
"try removing `&mut` here",
"",
Applicability::MaybeIncorrect,
Expand Down Expand Up @@ -755,13 +755,16 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
pat: hir::Pat { kind: hir::PatKind::Ref(_, _), .. },
..
}) = node
&& let Ok(name) =
self.infcx.tcx.sess.source_map().span_to_snippet(local_decl.source_info.span)
{
err.span_suggestion(
pat_span,
err.multipart_suggestion(
"consider changing this to be mutable",
format!("&(mut {name})"),
vec![
(pat_span.until(local_decl.source_info.span), "&(mut ".to_string()),
(
local_decl.source_info.span.shrink_to_hi().with_hi(pat_span.hi()),
")".to_string(),
),
],
Applicability::MachineApplicable,
);
return;
Expand Down
23 changes: 20 additions & 3 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2273,9 +2273,26 @@ impl HumanEmitter {
&normalize_whitespace(last_line),
Style::NoStyle,
);
buffer.puts(*row_num, 0, &self.maybe_anonymized(line_num), Style::LineNumber);
buffer.puts(*row_num, max_line_num_len + 1, "+ ", Style::Addition);
buffer.append(*row_num, &normalize_whitespace(line_to_add), Style::NoStyle);
if !line_to_add.trim().is_empty() {
// Check if after the removal, the line is left with only whitespace. If so, we
// will not show an "addition" line, as removing the whole line is what the user
// would really want.
// For example, for the following:
// |
// 2 - .await
// 2 + (note the left over whitepsace)
// |
// We really want
// |
// 2 - .await
// |
// *row_num -= 1;
buffer.puts(*row_num, 0, &self.maybe_anonymized(line_num), Style::LineNumber);
buffer.puts(*row_num, max_line_num_len + 1, "+ ", Style::Addition);
buffer.append(*row_num, &normalize_whitespace(line_to_add), Style::NoStyle);
} else {
*row_num -= 1;
}
} else {
*row_num -= 2;
}
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,11 @@ impl<'a, G: EmissionGuarantee> Diagnostic<'a, G> for MissingTypeParams {
} else {
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
// least we can clue them to the correct syntax `Iterator<Type>`.
err.span_suggestion(
self.span,
err.span_suggestion_verbose(
self.span.shrink_to_hi(),
fluent::hir_analysis_suggestion,
format!(
"{}<{}>",
snippet,
"<{}>",
self.missing_type_params
.iter()
.map(|n| n.to_string())
Expand Down
44 changes: 26 additions & 18 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2551,10 +2551,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

match *base_ty.peel_refs().kind() {
ty::Array(_, len) => {
self.maybe_suggest_array_indexing(&mut err, expr, base, ident, len);
self.maybe_suggest_array_indexing(&mut err, base, ident, len);
}
ty::RawPtr(..) => {
self.suggest_first_deref_field(&mut err, expr, base, ident);
self.suggest_first_deref_field(&mut err, base, ident);
}
ty::Param(param_ty) => {
err.span_label(ident.span, "unknown field");
Expand Down Expand Up @@ -2721,40 +2721,48 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn maybe_suggest_array_indexing(
&self,
err: &mut Diag<'_>,
expr: &hir::Expr<'_>,
base: &hir::Expr<'_>,
field: Ident,
len: ty::Const<'tcx>,
) {
err.span_label(field.span, "unknown field");
if let (Some(len), Ok(user_index)) =
(len.try_eval_target_usize(self.tcx, self.param_env), field.as_str().parse::<u64>())
&& let Ok(base) = self.tcx.sess.source_map().span_to_snippet(base.span)
{
let help = "instead of using tuple indexing, use array indexing";
let suggestion = format!("{base}[{field}]");
let applicability = if len < user_index {
Applicability::MachineApplicable
} else {
Applicability::MaybeIncorrect
};
err.span_suggestion(expr.span, help, suggestion, applicability);
err.multipart_suggestion(
help,
vec![
(base.span.between(field.span), "[".to_string()),
(field.span.shrink_to_hi(), "]".to_string()),
],
applicability,
);
}
}

fn suggest_first_deref_field(
&self,
err: &mut Diag<'_>,
expr: &hir::Expr<'_>,
base: &hir::Expr<'_>,
field: Ident,
) {
fn suggest_first_deref_field(&self, err: &mut Diag<'_>, base: &hir::Expr<'_>, field: Ident) {
err.span_label(field.span, "unknown field");
if let Ok(base) = self.tcx.sess.source_map().span_to_snippet(base.span) {
let msg = format!("`{base}` is a raw pointer; try dereferencing it");
let suggestion = format!("(*{base}).{field}");
err.span_suggestion(expr.span, msg, suggestion, Applicability::MaybeIncorrect);
}
let val = if let Ok(base) = self.tcx.sess.source_map().span_to_snippet(base.span)
&& base.len() < 20
{
format!("`{base}`")
} else {
"the value".to_string()
};
err.multipart_suggestion(
format!("{val} is a raw pointer; try dereferencing it"),
vec![
(base.span.shrink_to_lo(), "(*".to_string()),
(base.span.shrink_to_hi(), ")".to_string()),
],
Applicability::MaybeIncorrect,
);
}

fn no_such_field_err(&self, field: Ident, expr_t: Ty<'tcx>, id: HirId) -> Diag<'_> {
Expand Down
13 changes: 6 additions & 7 deletions compiler/rustc_hir_typeck/src/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2499,7 +2499,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.any(|(ty, _)| matches!(ty.kind(), ty::Slice(..) | ty::Array(..)))
&& let Some(span) = ti.span
&& let Some(_) = ti.origin_expr
&& let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span)
{
let resolved_ty = self.resolve_vars_if_possible(ti.expected);
let (is_slice_or_array_or_vector, resolved_ty) =
Expand All @@ -2510,10 +2509,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
|| self.tcx.is_diagnostic_item(sym::Result, adt_def.did()) =>
{
// Slicing won't work here, but `.as_deref()` might (issue #91328).
err.span_suggestion(
span,
err.span_suggestion_verbose(
span.shrink_to_hi(),
"consider using `as_deref` here",
format!("{snippet}.as_deref()"),
".as_deref()",
Applicability::MaybeIncorrect,
);
}
Expand All @@ -2522,10 +2521,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let is_top_level = current_depth <= 1;
if is_slice_or_array_or_vector && is_top_level {
err.span_suggestion(
span,
err.span_suggestion_verbose(
span.shrink_to_hi(),
"consider slicing here",
format!("{snippet}[..]"),
"[..]",
Applicability::MachineApplicable,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
) = tcx.sess.source_map().span_to_snippet(sp) =>
{
if snippet.chars().all(|c| c.is_digit(10) || c == '-' || c == '_') {
diag.span_suggestion(
sp,
diag.span_suggestion_verbose(
sp.shrink_to_hi(),
"use a float literal",
format!("{snippet}.0"),
".0",
MachineApplicable,
);
}
Expand Down
36 changes: 17 additions & 19 deletions src/librustdoc/passes/lint/bare_urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@ pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item) {
};
let dox = item.doc_value();
if !dox.is_empty() {
let report_diag =
|cx: &DocContext<'_>, msg: &'static str, url: &str, range: Range<usize>| {
let sp =
source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs.doc_strings)
.unwrap_or_else(|| item.attr_span(cx.tcx));
cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| {
lint.primary_message(msg)
.note("bare URLs are not automatically turned into clickable links")
.span_suggestion(
sp,
"use an automatic link instead",
format!("<{url}>"),
Applicability::MachineApplicable,
);
});
};
let report_diag = |cx: &DocContext<'_>, msg: &'static str, range: Range<usize>| {
let sp = source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs.doc_strings)
.unwrap_or_else(|| item.attr_span(cx.tcx));
cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| {
lint.primary_message(msg)
.note("bare URLs are not automatically turned into clickable links")
.multipart_suggestion(
"use an automatic link instead",
vec![
(sp.shrink_to_lo(), "<".to_string()),
(sp.shrink_to_hi(), ">".to_string()),
],
Applicability::MachineApplicable,
);
});
};

let mut p = Parser::new_ext(&dox, main_body_opts()).into_offset_iter();

Expand Down Expand Up @@ -74,17 +74,15 @@ fn find_raw_urls(
cx: &DocContext<'_>,
text: &str,
range: Range<usize>,
f: &impl Fn(&DocContext<'_>, &'static str, &str, Range<usize>),
f: &impl Fn(&DocContext<'_>, &'static str, Range<usize>),
) {
trace!("looking for raw urls in {text}");
// For now, we only check "full" URLs (meaning, starting with "http://" or "https://").
for match_ in URL_REGEX.find_iter(text) {
let url = match_.as_str();
let url_range = match_.range();
f(
cx,
"this URL is not a hyperlink",
url,
Range { start: range.start + url_range.start, end: range.start + url_range.end },
);
}
Expand Down
2 changes: 0 additions & 2 deletions src/tools/clippy/tests/ui/dbg_macro/dbg_macro.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ LL | dbg!();
help: remove the invocation before committing it to a version control system
|
LL - dbg!();
LL +
Comment on lines 88 to -89
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see those empty new line suggestion being removed. ❤️

|

error: the `dbg!` macro is intended as a debugging tool
Expand Down Expand Up @@ -146,7 +145,6 @@ LL | expand_to_dbg!();
help: remove the invocation before committing it to a version control system
|
LL - dbg!();
LL +
|

error: the `dbg!` macro is intended as a debugging tool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ LL | dbg!();
help: remove the invocation before committing it to a version control system
|
LL - dbg!();
LL +
|

error: the `dbg!` macro is intended as a debugging tool
Expand Down
Loading
Loading