Skip to content

Commit

Permalink
Rollup merge of rust-lang#65398 - estebank:capitalization-only, r=varkor
Browse files Browse the repository at this point in the history
Bring attention to suggestions when the only difference is capitalization

CC rust-lang#65386.
  • Loading branch information
tmandry authored Oct 15, 2019
2 parents 8d1123d + 8bf6d35 commit a14e35f
Show file tree
Hide file tree
Showing 28 changed files with 115 additions and 52 deletions.
3 changes: 2 additions & 1 deletion src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use syntax::feature_gate::UnstableFeatures;
use syntax::source_map::SourceMap;

use errors::emitter::HumanReadableErrorType;
use errors::{ColorConfig, FatalError, Handler};
use errors::{ColorConfig, FatalError, Handler, SourceMapperDyn};

use getopts;

Expand Down Expand Up @@ -1857,6 +1857,7 @@ struct NullEmitter;

impl errors::emitter::Emitter for NullEmitter {
fn emit_diagnostic(&mut self, _: &errors::Diagnostic) {}
fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> { None }
}

// Converts strings provided as `--cfg [cfgspec]` into a `crate_cfg`.
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ use rustc::util::common::{time_depth, set_time_depth, print_time_passes_entry};
use rustc::util::profiling::SelfProfilerRef;
use rustc_fs_util::link_or_copy;
use rustc_data_structures::svh::Svh;
use rustc_errors::{Handler, Level, FatalError, DiagnosticId};
use rustc_data_structures::sync::Lrc;
use rustc_errors::{Handler, Level, FatalError, DiagnosticId, SourceMapperDyn};
use rustc_errors::emitter::{Emitter};
use rustc_target::spec::MergeFunctions;
use syntax::attr;
Expand Down Expand Up @@ -1681,6 +1682,9 @@ impl Emitter for SharedEmitter {
}
drop(self.sender.send(SharedEmitterMessage::AbortIfErrors));
}
fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> {
None
}
}

impl SharedEmitterMain {
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_errors/annotate_snippet_emitter_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ impl Emitter for AnnotateSnippetEmitterWriter {
&suggestions);
}

fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> {
self.source_map.as_ref()
}

fn should_show_explain(&self) -> bool {
!self.short_message
}
Expand Down
49 changes: 44 additions & 5 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use syntax_pos::{SourceFile, Span, MultiSpan};

use crate::{
Level, CodeSuggestion, Diagnostic, SubDiagnostic,
SuggestionStyle, SourceMapperDyn, DiagnosticId,
SuggestionStyle, SourceMapper, SourceMapperDyn, DiagnosticId,
};
use crate::Level::Error;
use crate::snippet::{Annotation, AnnotationType, Line, MultilineAnnotation, StyledString, Style};
Expand Down Expand Up @@ -192,6 +192,8 @@ pub trait Emitter {
true
}

fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>>;

/// Formats the substitutions of the primary_span
///
/// The are a lot of conditions to this method, but in short:
Expand All @@ -204,7 +206,7 @@ pub trait Emitter {
/// we return the original `primary_span` and the original suggestions.
fn primary_span_formatted<'a>(
&mut self,
db: &'a Diagnostic
db: &'a Diagnostic,
) -> (MultiSpan, &'a [CodeSuggestion]) {
let mut primary_span = db.span.clone();
if let Some((sugg, rest)) = db.suggestions.split_first() {
Expand Down Expand Up @@ -234,7 +236,20 @@ pub trait Emitter {
format!("help: {}", sugg.msg)
} else {
// Show the default suggestion text with the substitution
format!("help: {}: `{}`", sugg.msg, substitution)
format!(
"help: {}{}: `{}`",
sugg.msg,
if self.source_map().map(|sm| is_case_difference(
&**sm,
substitution,
sugg.substitutions[0].parts[0].span,
)).unwrap_or(false) {
" (notice the capitalization)"
} else {
""
},
substitution,
)
};
primary_span.push_span_label(sugg.substitutions[0].parts[0].span, msg);

Expand Down Expand Up @@ -382,6 +397,10 @@ pub trait Emitter {
}

impl Emitter for EmitterWriter {
fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> {
self.sm.as_ref()
}

fn emit_diagnostic(&mut self, db: &Diagnostic) {
let mut children = db.children.clone();
let (mut primary_span, suggestions) = self.primary_span_formatted(&db);
Expand Down Expand Up @@ -1461,7 +1480,9 @@ impl EmitterWriter {
let suggestions = suggestion.splice_lines(&**sm);

let mut row_num = 2;
for &(ref complete, ref parts) in suggestions.iter().take(MAX_SUGGESTIONS) {
let mut notice_capitalization = false;
for (complete, parts, only_capitalization) in suggestions.iter().take(MAX_SUGGESTIONS) {
notice_capitalization |= only_capitalization;
// Only show underline if the suggestion spans a single line and doesn't cover the
// entirety of the code output. If you have multiple replacements in the same line
// of code, show the underline.
Expand Down Expand Up @@ -1552,7 +1573,10 @@ impl EmitterWriter {
}
if suggestions.len() > MAX_SUGGESTIONS {
let msg = format!("and {} other candidates", suggestions.len() - MAX_SUGGESTIONS);
buffer.puts(row_num, 0, &msg, Style::NoStyle);
buffer.puts(row_num, max_line_num_len + 3, &msg, Style::NoStyle);
} else if notice_capitalization {
let msg = "notice the capitalization difference";
buffer.puts(row_num, max_line_num_len + 3, &msg, Style::NoStyle);
}
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
Ok(())
Expand Down Expand Up @@ -2034,3 +2058,18 @@ impl<'a> Drop for WritableDst<'a> {
}
}
}

/// Whether the original and suggested code are visually similar enough to warrant extra wording.
pub fn is_case_difference(sm: &dyn SourceMapper, suggested: &str, sp: Span) -> bool {
// FIXME: this should probably be extended to also account for `FO0` → `FOO` and unicode.
let found = sm.span_to_snippet(sp).unwrap();
let ascii_confusables = &['c', 'f', 'i', 'k', 'o', 's', 'u', 'v', 'w', 'x', 'y', 'z'];
// All the chars that differ in capitalization are confusable (above):
let confusable = found.chars().zip(suggested.chars()).filter(|(f, s)| f != s).all(|(f, s)| {
(ascii_confusables.contains(&f) || ascii_confusables.contains(&s))
});
confusable && found.to_lowercase() == suggested.to_lowercase()
// FIXME: We sometimes suggest the same thing we already have, which is a
// bug, but be defensive against that here.
&& found != suggested
}
32 changes: 20 additions & 12 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub use emitter::ColorConfig;

use Level::*;

use emitter::{Emitter, EmitterWriter};
use emitter::{Emitter, EmitterWriter, is_case_difference};
use registry::Registry;

use rustc_data_structures::sync::{self, Lrc, Lock};
Expand All @@ -37,13 +37,16 @@ pub mod registry;
mod styled_buffer;
mod lock;

use syntax_pos::{BytePos,
Loc,
FileLinesResult,
SourceFile,
FileName,
MultiSpan,
Span};
use syntax_pos::{
BytePos,
FileLinesResult,
FileName,
Loc,
MultiSpan,
SourceFile,
Span,
SpanSnippetError,
};

/// Indicates the confidence in the correctness of a suggestion.
///
Expand Down Expand Up @@ -147,6 +150,7 @@ pub trait SourceMapper {
fn lookup_char_pos(&self, pos: BytePos) -> Loc;
fn span_to_lines(&self, sp: Span) -> FileLinesResult;
fn span_to_string(&self, sp: Span) -> String;
fn span_to_snippet(&self, sp: Span) -> Result<String, SpanSnippetError>;
fn span_to_filename(&self, sp: Span) -> FileName;
fn merge_spans(&self, sp_lhs: Span, sp_rhs: Span) -> Option<Span>;
fn call_span_if_macro(&self, sp: Span) -> Span;
Expand All @@ -155,9 +159,12 @@ pub trait SourceMapper {
}

impl CodeSuggestion {
/// Returns the assembled code suggestions and whether they should be shown with an underline.
pub fn splice_lines(&self, cm: &SourceMapperDyn)
-> Vec<(String, Vec<SubstitutionPart>)> {
/// Returns the assembled code suggestions, whether they should be shown with an underline
/// and whether the substitution only differs in capitalization.
pub fn splice_lines(
&self,
cm: &SourceMapperDyn,
) -> Vec<(String, Vec<SubstitutionPart>, bool)> {
use syntax_pos::{CharPos, Pos};

fn push_trailing(buf: &mut String,
Expand Down Expand Up @@ -232,6 +239,7 @@ impl CodeSuggestion {
prev_hi = cm.lookup_char_pos(part.span.hi());
prev_line = fm.get_line(prev_hi.line - 1);
}
let only_capitalization = is_case_difference(cm, &buf, bounding_span);
// if the replacement already ends with a newline, don't print the next line
if !buf.ends_with('\n') {
push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None);
Expand All @@ -240,7 +248,7 @@ impl CodeSuggestion {
while buf.ends_with('\n') {
buf.pop();
}
(buf, substitution.parts)
(buf, substitution.parts, only_capitalization)
}).collect()
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/libsyntax/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
use crate::source_map::{SourceMap, FilePathMapping};

use errors::registry::Registry;
use errors::{SubDiagnostic, CodeSuggestion, SourceMapper};
use errors::{SubDiagnostic, CodeSuggestion, SourceMapper, SourceMapperDyn};
use errors::{DiagnosticId, Applicability};
use errors::emitter::{Emitter, HumanReadableErrorType};

Expand Down Expand Up @@ -113,6 +113,10 @@ impl Emitter for JsonEmitter {
}
}

fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> {
Some(&self.sm)
}

fn should_show_explain(&self) -> bool {
match self.json_rendered {
HumanReadableErrorType::Short(_) => false,
Expand Down
3 changes: 3 additions & 0 deletions src/libsyntax/source_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,9 @@ impl SourceMapper for SourceMap {
fn span_to_string(&self, sp: Span) -> String {
self.span_to_string(sp)
}
fn span_to_snippet(&self, sp: Span) -> Result<String, SpanSnippetError> {
self.span_to_snippet(sp)
}
fn span_to_filename(&self, sp: Span) -> FileName {
self.span_to_filename(sp)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ error: const parameter `x` should have an upper case name
--> $DIR/const-parameter-uppercase-lint.rs:6:15
|
LL | fn noop<const x: u32>() {
| ^ help: convert the identifier to upper case: `X`
| ^ help: convert the identifier to upper case (notice the capitalization): `X`
|
note: lint level defined here
--> $DIR/const-parameter-uppercase-lint.rs:4:9
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ LL | let z = ManyVariants::Three();
| ^^^^^^^^^^^^^^^^^^^
LL | let z = ManyVariants::Four();
| ^^^^^^^^^^^^^^^^^^
and 6 other candidates
and 6 other candidates

error: aborting due to 5 previous errors

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ LL | fn setup() -> Determine { Set }
| ^^^^^^^^^
LL | fn setup() -> PutDown { Set }
| ^^^^^^^
and 3 other candidates
and 3 other candidates

error[E0425]: cannot find value `Set` in this scope
--> $DIR/issue-56028-there-is-an-enum-variant.rs:9:21
Expand All @@ -30,7 +30,7 @@ LL | use Determine::Set;
|
LL | use PutDown::Set;
|
and 3 other candidates
and 3 other candidates

error: aborting due to 2 previous errors

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/error-codes/E0423.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ LL | let f = Foo();
| ^^^
| |
| did you mean `Foo { /* fields */ }`?
| help: a function with a similar name exists: `foo`
| help: a function with a similar name exists (notice the capitalization): `foo`

error[E0423]: expected value, found struct `T`
--> $DIR/E0423.rs:14:8
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/expr_attr_paren_order.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: variable `X` should have a snake case name
--> $DIR/expr_attr_paren_order.rs:19:17
|
LL | let X = 0;
| ^ help: convert the identifier to snake case: `x`
| ^ help: convert the identifier to snake case (notice the capitalization): `x`
|
note: lint level defined here
--> $DIR/expr_attr_paren_order.rs:17:17
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/hygiene/globs.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ LL | use foo::test2::test::g;
|
LL | use foo::test::g;
|
and 2 other candidates
and 2 other candidates

error[E0425]: cannot find function `f` in this scope
--> $DIR/globs.rs:61:12
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/hygiene/rustc-macro-transparency.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0425]: cannot find value `Opaque` in this scope
--> $DIR/rustc-macro-transparency.rs:26:5
|
LL | Opaque;
| ^^^^^^ help: a local variable with a similar name exists: `opaque`
| ^^^^^^ help: a local variable with a similar name exists (notice the capitalization): `opaque`

error[E0423]: expected value, found macro `semitransparent`
--> $DIR/rustc-macro-transparency.rs:29:5
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-10200.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0532]: expected tuple struct/variant, found function `foo`
--> $DIR/issue-10200.rs:6:9
|
LL | foo(x)
| ^^^ help: a tuple struct with a similar name exists: `Foo`
| ^^^ help: a tuple struct with a similar name exists (notice the capitalization): `Foo`

error: aborting due to previous error

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-17546.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ LL | use std::prelude::v1::Result;
|
LL | use std::result::Result;
|
and 1 other candidates
and 1 other candidates

error[E0573]: expected type, found variant `Result`
--> $DIR/issue-17546.rs:28:13
Expand All @@ -44,7 +44,7 @@ LL | use std::prelude::v1::Result;
|
LL | use std::result::Result;
|
and 1 other candidates
and 1 other candidates

error[E0573]: expected type, found variant `NoResult`
--> $DIR/issue-17546.rs:33:15
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-17718-const-naming.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ error: constant `foo` should have an upper case name
--> $DIR/issue-17718-const-naming.rs:4:7
|
LL | const foo: isize = 3;
| ^^^ help: convert the identifier to upper case: `FOO`
| ^^^ help: convert the identifier to upper case (notice the capitalization): `FOO`
|
note: lint level defined here
--> $DIR/issue-17718-const-naming.rs:2:9
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-46332.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0422]: cannot find struct, variant or union type `TyUInt` in this scope
--> $DIR/issue-46332.rs:9:5
|
LL | TyUInt {};
| ^^^^^^ help: a struct with a similar name exists: `TyUint`
| ^^^^^^ help: a struct with a similar name exists (notice the capitalization): `TyUint`

error: aborting due to previous error

Expand Down
Loading

0 comments on commit a14e35f

Please sign in to comment.