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.

Sign up for GitHub

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

Remove usages of span_suggestion without Applicability #54241

Merged
merged 13 commits into from
Sep 20, 2018
Merged

Remove usages of span_suggestion without Applicability #54241

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b6fea32
Remove usages of span_suggestion without Applicability
vi Sep 15, 2018
2f5cb6d
Add multipart_suggestion_with_applicability
vi Sep 15, 2018
4b05128
Attach Applicability to multipart_suggestion and span_suggestions
vi Sep 15, 2018
959fc61
Deprecate *_suggestion* that are without explicit applicability
vi Sep 15, 2018
3be24c6
trailing whitespace fix
vi Sep 15, 2018
f395072
Fixup inaccurate rebase
vi Sep 16, 2018
6ebb916
Fix style according to review comments.
vi Sep 16, 2018
0ad1c0e
Change diagnostic_builder's forward! macro to enforce trailing argume…
vi Sep 16, 2018
c61f4a7
Fix tidy's too long lines.
vi Sep 16, 2018
2b77760
Fill in suggestions Applicability according to @estebank
vi Sep 17, 2018
0269e66
Fixed some remaining whitespace issues.
vi Sep 17, 2018
15982fe
Better trick for allowing trailing comma at forward!
vi Sep 17, 2018
d0790c4
Whitespace fix again.
vi Sep 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use infer::error_reporting::nice_region_error::NiceRegionError;
use ty;
use util::common::ErrorReported;
use errors::Applicability;

impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
/// When given a `ConcreteFailure` for a function with arguments containing a named region and
Expand Down Expand Up @@ -111,10 +112,11 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
E0621,
"explicit lifetime required in {}",
error_var
).span_suggestion(
).span_suggestion_with_applicability(
new_ty_span,
&format!("add explicit lifetime `{}` to {}", named, span_label_var),
new_ty.to_string()
new_ty.to_string(),
Applicability::Unspecified,
)
.span_label(span, format!("lifetime `{}` required", named))
.emit();
Expand Down
4 changes: 3 additions & 1 deletion src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use infer::error_reporting::nice_region_error::NiceRegionError;
use infer::lexical_region_resolve::RegionResolutionError;
use ty::{BoundRegion, FreeRegion, RegionKind};
use util::common::ErrorReported;
use errors::Applicability;

impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
/// Print the error message for lifetime errors when the return type is a static impl Trait.
Expand Down Expand Up @@ -61,14 +62,15 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
_ => "'_".to_owned(),
};
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(return_sp) {
err.span_suggestion(
err.span_suggestion_with_applicability(
return_sp,
&format!(
"you can add a constraint to the return type to make it last \
less than `'static` and match {}",
lifetime,
),
format!("{} + {}", snippet, lifetime_name),
Applicability::Unspecified,
);
}
err.emit();
Expand Down
11 changes: 8 additions & 3 deletions src/librustc/session/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use rustc_data_structures::base_n;
use rustc_data_structures::sync::{self, Lrc, Lock, LockCell, OneThread, Once, RwLock};

use syntax::ast::NodeId;
use errors::{self, DiagnosticBuilder, DiagnosticId};
use errors::{self, DiagnosticBuilder, DiagnosticId, Applicability};
use errors::emitter::{Emitter, EmitterWriter};
use syntax::edition::Edition;
use syntax::json::JsonEmitter;
Expand Down Expand Up @@ -431,8 +431,13 @@ impl Session {
diag_builder.span_note(span, message);
}
DiagnosticBuilderMethod::SpanSuggestion(suggestion) => {
let span = span_maybe.expect("span_suggestion needs a span");
diag_builder.span_suggestion(span, message, suggestion);
let span = span_maybe.expect("span_suggestion_* needs a span");
diag_builder.span_suggestion_with_applicability(
span,
message,
suggestion,
Applicability::Unspecified,
);
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see what the formatting commits were about now: Please, for all the people who like split diffs, indent the arguments just one level and not visually :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@vi the only thing left is the formatting here (and elsewhere) should have the arguments indented one level (four spaces):

                    diag_builder.span_suggestion_with_applicability(
                        span,
                        message,
                        suggestion,
                        Applicability::Unspecified,
                    );

}
}
}
Expand Down
37 changes: 26 additions & 11 deletions src/librustc_borrowck/borrowck/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,10 +867,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}) = cmt.cat {
db.note(fn_closure_msg);
} else {
db.span_suggestion(sp, msg, suggestion);
db.span_suggestion_with_applicability(
sp,
msg,
suggestion,
Applicability::Unspecified,
);
}
} else {
db.span_suggestion(sp, msg, suggestion);
db.span_suggestion_with_applicability(
sp,
msg,
suggestion,
Applicability::Unspecified,
);
}
}
_ => {
Expand Down Expand Up @@ -1236,10 +1246,11 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
let let_span = self.tcx.hir.span(node_id);
let suggestion = suggest_ref_mut(self.tcx, let_span);
if let Some(replace_str) = suggestion {
db.span_suggestion(
db.span_suggestion_with_applicability(
let_span,
"use a mutable reference instead",
replace_str,
Applicability::Unspecified,
vi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

cargo fix will actually try to iteratively apply suggestions when necessary: rust-lang/cargo#5842. IIUC, this will generate the same suggestion over and over again? This should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@killercup Won't it cause the final code to be something along the lines of let mut mut mut mut x?

Copy link
Member

Choose a reason for hiding this comment

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

rustfix works by replacing specific byte ranges. Once a part of the file has been replaced, rustfix will yell at you when you try to replace it against. cargo-fix will then try to resolve this in some ways (see link above)

);
}
}
Expand Down Expand Up @@ -1292,11 +1303,12 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
)) = ty.map(|t| &t.node)
{
let borrow_expr_id = self.tcx.hir.get_parent_node(borrowed_node_id);
db.span_suggestion(
db.span_suggestion_with_applicability(
self.tcx.hir.span(borrow_expr_id),
"consider removing the `&mut`, as it is an \
immutable binding to a mutable reference",
snippet
snippet,
Applicability::Unspecified,
vi marked this conversation as resolved.
Show resolved Hide resolved
);
} else {
db.span_suggestion_with_applicability(
Expand Down Expand Up @@ -1326,12 +1338,15 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
&cmt_path_or_string,
capture_span,
Origin::Ast)
.span_suggestion(err.span,
&format!("to force the closure to take ownership of {} \
(and any other referenced variables), \
use the `move` keyword",
cmt_path_or_string),
suggestion)
.span_suggestion_with_applicability(
err.span,
&format!("to force the closure to take ownership of {} \
(and any other referenced variables), \
use the `move` keyword",
cmt_path_or_string),
suggestion,
Applicability::Unspecified,
)
vi marked this conversation as resolved.
Show resolved Hide resolved
.emit();
self.signal_error();
}
Expand Down
21 changes: 19 additions & 2 deletions src/librustc_errors/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ impl Diagnostic {
/// inline it will only show the text message and not the text.
///
/// See `CodeSuggestion` for more information.
#[deprecated(note = "Use `span_suggestion_short_with_applicability`")]
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this affects other tools that might be using it. They will have to add #[allow(deprecated)] for a bit. CC @rust-lang/dev-tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've prepared a similar pull request for Clippy (with similar style/formatting mistakes), but it may intersect existing Applicability-assigning pull request there.

pub fn span_suggestion_short(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitutions: vec![Substitution {
Expand Down Expand Up @@ -263,6 +264,7 @@ impl Diagnostic {
/// * may contain a name of a function, variable or type, but not whole expressions
///
/// See `CodeSuggestion` for more information.
#[deprecated(note = "Use `span_suggestion_with_applicability`")]
pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitutions: vec![Substitution {
Expand All @@ -278,10 +280,11 @@ impl Diagnostic {
self
}

pub fn multipart_suggestion(
pub fn multipart_suggestion_with_applicability(
&mut self,
msg: &str,
suggestion: Vec<(Span, String)>,
applicability: Applicability,
) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitutions: vec![Substitution {
Expand All @@ -292,12 +295,26 @@ impl Diagnostic {
}],
msg: msg.to_owned(),
show_code_when_inline: true,
applicability: Applicability::Unspecified,
applicability,
});
self
}

#[deprecated(note = "Use `multipart_suggestion_with_applicability`")]
pub fn multipart_suggestion(
&mut self,
msg: &str,
suggestion: Vec<(Span, String)>,
) -> &mut Self {
self.multipart_suggestion_with_applicability(
msg,
suggestion,
Applicability::Unspecified,
)
}

/// Prints out a message with multiple suggested edits of the code.
#[deprecated(note = "Use `span_suggestions_with_applicability`")]
pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec<String>) -> &mut Self {
self.suggestions.push(CodeSuggestion {
substitutions: suggestions.into_iter().map(|snippet| Substitution {
Expand Down
85 changes: 57 additions & 28 deletions src/librustc_errors/diagnostic_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,28 @@ pub struct DiagnosticBuilder<'a> {
/// it easy to declare such methods on the builder.
macro_rules! forward {
// Forward pattern for &self -> &Self
(pub fn $n:ident(&self, $($name:ident: $ty:ty),*) -> &Self) => {
(pub fn $n:ident(&self, $($name:ident: $ty:ty,)*) -> &Self) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change? It seems to me that including the , inside the repetition makes it slightly more annoying to use elsewhere in the file, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inside the macro call, it was requested to update it to new style (each argument on separate line). This looks better with trailing commas, but original macro didn't support trailing commas. Seeing no easy way to make it accept optional trailing comma, I just made it accept non-optional trailing comma. This cascaded into changing all invocations (and one-line invocations just got in the way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slightly more annoying

For me diff/merge-friendliness brought by trailing commas typically trumps niceness for eye. So if there can only be one, it should be with the commas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remembered that there's $(,)* trick, now it can be both with and without.

pub fn $n(&self, $($name: $ty),*) -> &Self {
#[allow(deprecated)]
self.diagnostic.$n($($name),*);
self
}
};

// Forward pattern for &mut self -> &mut Self
(pub fn $n:ident(&mut self, $($name:ident: $ty:ty),*) -> &mut Self) => {
(pub fn $n:ident(&mut self, $($name:ident: $ty:ty,)*) -> &mut Self) => {
pub fn $n(&mut self, $($name: $ty),*) -> &mut Self {
#[allow(deprecated)]
self.diagnostic.$n($($name),*);
self
}
};

// Forward pattern for &mut self -> &mut Self, with S: Into<MultiSpan>
// type parameter. No obvious way to make this more generic.
(pub fn $n:ident<S: Into<MultiSpan>>(&mut self, $($name:ident: $ty:ty),*) -> &mut Self) => {
pub fn $n<S: Into<MultiSpan>>(&mut self, $($name: $ty),*) -> &mut Self {
(pub fn $n:ident<S: Into<MultiSpan>>(&mut self, $($name:ident: $ty:ty,)*) -> &mut Self) => {
pub fn $n<S: Into<MultiSpan>>(&mut self, $($name: $ty,)*) -> &mut Self {
#[allow(deprecated)]
self.diagnostic.$n($($name),*);
self
}
Expand Down Expand Up @@ -144,49 +147,75 @@ impl<'a> DiagnosticBuilder<'a> {
forward!(pub fn note_expected_found(&mut self,
label: &dyn fmt::Display,
expected: DiagnosticStyledString,
found: DiagnosticStyledString)
-> &mut Self);
found: DiagnosticStyledString,
) -> &mut Self);

forward!(pub fn note_expected_found_extra(&mut self,
label: &dyn fmt::Display,
expected: DiagnosticStyledString,
found: DiagnosticStyledString,
expected_extra: &dyn fmt::Display,
found_extra: &dyn fmt::Display)
-> &mut Self);
found_extra: &dyn fmt::Display,
) -> &mut Self);

forward!(pub fn note(&mut self, msg: &str) -> &mut Self);
forward!(pub fn note(&mut self, msg: &str,) -> &mut Self);
vi marked this conversation as resolved.
Show resolved Hide resolved
forward!(pub fn span_note<S: Into<MultiSpan>>(&mut self,
sp: S,
msg: &str)
-> &mut Self);
forward!(pub fn warn(&mut self, msg: &str) -> &mut Self);
forward!(pub fn span_warn<S: Into<MultiSpan>>(&mut self, sp: S, msg: &str) -> &mut Self);
forward!(pub fn help(&mut self , msg: &str) -> &mut Self);
msg: &str,
) -> &mut Self);
forward!(pub fn warn(&mut self, msg: &str,) -> &mut Self);
forward!(pub fn span_warn<S: Into<MultiSpan>>(&mut self, sp: S, msg: &str,) -> &mut Self);
forward!(pub fn help(&mut self , msg: &str,) -> &mut Self);
forward!(pub fn span_help<S: Into<MultiSpan>>(&mut self,
sp: S,
msg: &str)
-> &mut Self);
forward!(pub fn span_suggestion_short(&mut self,
sp: Span,
msg: &str,
suggestion: String)
-> &mut Self);
msg: &str,
) -> &mut Self);

#[deprecated(note = "Use `span_suggestion_short_with_applicability`")]
forward!(pub fn span_suggestion_short(
&mut self,
sp: Span,
msg: &str,
suggestion: String,
) -> &mut Self);

#[deprecated(note = "Use `multipart_suggestion_with_applicability`")]
forward!(pub fn multipart_suggestion(
&mut self,
msg: &str,
suggestion: Vec<(Span, String)>
suggestion: Vec<(Span, String)>,
) -> &mut Self);

#[deprecated(note = "Use `span_suggestion_with_applicability`")]
forward!(pub fn span_suggestion(&mut self,
sp: Span,
msg: &str,
suggestion: String)
-> &mut Self);
suggestion: String,
) -> &mut Self);

#[deprecated(note = "Use `span_suggestions_with_applicability`")]
forward!(pub fn span_suggestions(&mut self,
sp: Span,
msg: &str,
suggestions: Vec<String>)
-> &mut Self);
suggestions: Vec<String>,
) -> &mut Self);

pub fn multipart_suggestion_with_applicability(&mut self,
msg: &str,
suggestion: Vec<(Span, String)>,
applicability: Applicability,
) -> &mut Self {
if !self.allow_suggestions {
return self
}
self.diagnostic.multipart_suggestion_with_applicability(
msg,
suggestion,
applicability,
);
self
}

pub fn span_suggestion_with_applicability(&mut self,
sp: Span,
msg: &str,
Expand Down Expand Up @@ -240,8 +269,8 @@ impl<'a> DiagnosticBuilder<'a> {
);
self
}
forward!(pub fn set_span<S: Into<MultiSpan>>(&mut self, sp: S) -> &mut Self);
forward!(pub fn code(&mut self, s: DiagnosticId) -> &mut Self);
forward!(pub fn set_span<S: Into<MultiSpan>>(&mut self, sp: S,) -> &mut Self);
forward!(pub fn code(&mut self, s: DiagnosticId,) -> &mut Self);

pub fn allow_suggestions(&mut self, allow: bool) -> &mut Self {
self.allow_suggestions = allow;
Expand Down
13 changes: 8 additions & 5 deletions src/librustc_mir/borrow_check/move_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use core::unicode::property::Pattern_White_Space;
use rustc::mir::*;
use rustc::ty;
use rustc_errors::DiagnosticBuilder;
use rustc_errors::{DiagnosticBuilder,Applicability};
use syntax_pos::Span;

use borrow_check::MirBorrowckCtxt;
Expand Down Expand Up @@ -350,16 +350,18 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
// expressions `a[b]`, which roughly desugar to
// `*Index::index(&a, b)` or
// `*IndexMut::index_mut(&mut a, b)`.
err.span_suggestion(
err.span_suggestion_with_applicability(
span,
"consider removing the `*`",
snippet[1..].to_owned(),
Applicability::Unspecified,
);
} else {
err.span_suggestion(
err.span_suggestion_with_applicability(
span,
"consider borrowing here",
format!("&{}", snippet),
Applicability::Unspecified,
);
}

Expand Down Expand Up @@ -420,10 +422,11 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
suggestions.sort_unstable_by_key(|&(span, _, _)| span);
suggestions.dedup_by_key(|&mut (span, _, _)| span);
for (span, to_remove, suggestion) in suggestions {
err.span_suggestion(
err.span_suggestion_with_applicability(
span,
&format!("consider removing the `{}`", to_remove),
suggestion
suggestion,
Applicability::Unspecified,
vi marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
Expand Down
Loading