Skip to content

Bring attention to suggestions when the only difference is capitalization #65398

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

Merged
merged 3 commits into from
Oct 15, 2019
Merged
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/librustc/session/config.rs
Original file line number Diff line number Diff line change
@@ -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;

@@ -1854,6 +1854,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`.
6 changes: 5 additions & 1 deletion src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
@@ -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;
@@ -1681,6 +1682,9 @@ impl Emitter for SharedEmitter {
}
drop(self.sender.send(SharedEmitterMessage::AbortIfErrors));
}
fn source_map(&self) -> Option<&Lrc<SourceMapperDyn>> {
None
}
}

impl SharedEmitterMain {
4 changes: 4 additions & 0 deletions src/librustc_errors/annotate_snippet_emitter_writer.rs
Original file line number Diff line number Diff line change
@@ -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
}
49 changes: 44 additions & 5 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
@@ -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};
@@ -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:
@@ -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() {
@@ -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);

@@ -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);
@@ -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.
@@ -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(())
@@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can avoid an allocation by changing into found.chars().flat_map(char::to_lowercase).eq(suggested.chars().flat_map(char::to_lowercase)). Or it is not worth it?

// 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
@@ -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};
@@ -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.
///
@@ -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;
@@ -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,
@@ -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);
@@ -240,7 +248,7 @@ impl CodeSuggestion {
while buf.ends_with('\n') {
buf.pop();
}
(buf, substitution.parts)
(buf, substitution.parts, only_capitalization)
}).collect()
}
}
6 changes: 5 additions & 1 deletion src/libsyntax/json.rs
Original file line number Diff line number Diff line change
@@ -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};

@@ -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,
3 changes: 3 additions & 0 deletions src/libsyntax/source_map.rs
Original file line number Diff line number Diff line change
@@ -987,6 +987,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)
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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

Original file line number Diff line number Diff line change
@@ -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
@@ -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

2 changes: 1 addition & 1 deletion src/test/ui/error-codes/E0423.stderr
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/test/ui/expr_attr_paren_order.stderr
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/test/ui/hygiene/globs.stderr
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/test/ui/hygiene/rustc-macro-transparency.stderr
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-10200.stderr
Original file line number Diff line number Diff line change
@@ -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

4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-17546.stderr
Original file line number Diff line number Diff line change
@@ -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
@@ -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
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
@@ -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
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-46332.stderr
Original file line number Diff line number Diff line change
@@ -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

Loading