Skip to content

Commit

Permalink
Fix symbol ordering for confusable idents detection.
Browse files Browse the repository at this point in the history
Confusable idents detection uses a type `BTreeMap<Symbol, Span>`. This is
highly dubious given that `Symbol` doesn't guarantee a meaningful order. (In
practice, it currently gives an order that mostly matches source code order.)

As a result, changes in `Symbol` representation make the
`lint-confusable-idents.rs` test fail, because this error message:

> identifier pair considered confusable between `s` and `s`

is changed to this:

> identifier pair considered confusable between `s` and `s`

and the corresponding span pointers get swapped erroneously, leading to
an incorrect "previous identifier" label.

This commit sorts the relevant symbols by span before doing the checking,
which ensures that the ident that appears first in the code will be mentioned
first in the message. The commit also extends the test slightly to be more
thorough.
  • Loading branch information
nnethercote committed Aug 9, 2020
1 parent 39e593a commit 75b67c2
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 4 deletions.
6 changes: 6 additions & 0 deletions src/librustc_lint/non_ascii_idents.rs
Expand Up @@ -58,6 +58,12 @@ impl EarlyLintPass for NonAsciiIdents {

let mut has_non_ascii_idents = false;
let symbols = cx.sess.parse_sess.symbol_gallery.symbols.lock();

// Sort by `Span` so that error messages make sense with respect to the
// order of identifier locations in the code.
let mut symbols: Vec<_> = symbols.iter().collect();
symbols.sort_by_key(|k| k.1);

for (symbol, &sp) in symbols.iter() {
let symbol_str = symbol.as_str();
if symbol_str.is_ascii() {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_session/parse.rs
Expand Up @@ -13,7 +13,6 @@ use rustc_span::hygiene::ExpnId;
use rustc_span::source_map::{FilePathMapping, SourceMap};
use rustc_span::{MultiSpan, Span, Symbol};

use std::collections::BTreeMap;
use std::path::PathBuf;
use std::str;

Expand Down Expand Up @@ -64,7 +63,7 @@ impl GatedSpans {
#[derive(Default)]
pub struct SymbolGallery {
/// All symbols occurred and their first occurrence span.
pub symbols: Lock<BTreeMap<Symbol, Span>>,
pub symbols: Lock<FxHashMap<Symbol, Span>>,
}

impl SymbolGallery {
Expand Down
Expand Up @@ -3,9 +3,11 @@
#![allow(uncommon_codepoints, non_upper_case_globals)]

const: usize = 42;
const s_s: usize = 42;

fn main() {
let s = "rust"; //~ ERROR identifier pair considered confusable
let s_s = "rust2"; //~ ERROR identifier pair considered confusable
not_affected();
}

Expand Down
@@ -1,5 +1,5 @@
error: identifier pair considered confusable between `s` and `s`
--> $DIR/lint-confusable-idents.rs:8:9
--> $DIR/lint-confusable-idents.rs:9:9
|
LL | const s: usize = 42;
| -- this is where the previous identifier occurred
Expand All @@ -13,5 +13,14 @@ note: the lint level is defined here
LL | #![deny(confusable_idents)]
| ^^^^^^^^^^^^^^^^^

error: aborting due to previous error
error: identifier pair considered confusable between `s_s` and `s_s`
--> $DIR/lint-confusable-idents.rs:10:9
|
LL | const s_s: usize = 42;
| --- this is where the previous identifier occurred
...
LL | let s_s = "rust2";
| ^^^^^

error: aborting due to 2 previous errors

0 comments on commit 75b67c2

Please sign in to comment.