Add new lint: Mixed locale ident

[popzxc]

This PR adds a new lint to check that the identifier name has multiple locales.

I think that it's not the thing that must happen normally, as it both makes hand-writing the code much harder, and can lead to confusing errors (rustc's built-in lint mixed_script_confusables can be implicitly shadowed, which makes it not really reliable).

stderr example:

error: multiple locales used in identifier `Blоck`: Cyrillic, Latin
  --> $DIR/mixed_locale_idents.rs:5:12
   |
LL | pub struct Blоck;
   |            ^^^^^
   |
   = note: `-D clippy::mixed-locale-idents` implied by `-D warnings`

error: multiple locales used in identifier `black_чёрный_黒い_काला`: Devanagari, Cyrillic, Hiragana, Han, Latin
  --> $DIR/mixed_locale_idents.rs:8:9
   |
LL |     let black_чёрный_黒い_काला = "good luck hand-writing it";
   |         ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [`mixed_locale_idents`]

[rust-highfive]

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

[xFrednet]

What would the lint say about a value name like nutzer_zähler (German for user_counter)? Does the included library figure out that both words can originate from the same alphabet?

[popzxc]

What would the lint say about a value name like nutzer_zähler (German for user_counter)?

Good question and great test case! Added it, and yes, it does not spawn the lint (I'm not a specialist in unicode by any means, but AFAIK diaeresis doesn't move ä into any different locale).

[flip1995]

Ping @Manishearth since you maintain the unicode crates in question here. Do you think adding this dep to Clippy (with the same configuration as in rustc) is fine?

[Manishearth]

Strongly against this: I designed the lints that are in rustc for this and they were pretty carefully designed, over months of discussion. This particular thing was not considered to be a case worth addressing because idents like try_看 actually do make sense to have; and a core principle behind the design of those lints was to avoid malicious and confusing situations without actually harming legitimate use.

I don't understand why "handwriting the code is hard" is a concern at all; nor do I understand what you mean by the builtin getting implicitly shadowed.

[Manishearth]

Oh, I see what you mean by "shadowing". Yes, this was also considered when designing the rustc lint, and also determined to not be worth it: if someone has chosen to use Cyrillic in their code it's not worth it to try and nitpick "good usage" from "bad usage" because those can be pretty linked.

There are some other potential designs here: e.g. warning about mixed locales when it's only mixed-script confusables and only when not separated by underscores. But the current proposed design will catch too many legitimate use cases. Probably more than illegitimate ones, even, this kind of situation is super rare to trigger by accident.

It does sound like a reasonable style guideline to not mix scripts across underscores in a way that one of the scripts is purely using confusables.

[popzxc]

That's why it's in clippy and not in the compiler, isn't it? It's completely OK to 'allow' lints you find not suiting your style.

I guess not every project will be happy with the approach in rustc (at least I am personally have plans to actively dogfood this lint in projects I participate in).

Maybe just put this lint into 'pedantic' category?

[Manishearth]

That's why it's in clippy and not in the compiler, isn't it? It's completely OK to 'allow' lints you find not suiting your style.

Yes, but adding it to clippy still makes it a value judgement; and I do not consider this a good value judgement in this case. I'm not making this comment as a personal comment of style, I'm making this comment as a clippy maintainer who thinks that we need to be certain of the value judgements we are making when we add lints and as the person who did the research and design of the non ascii idents RFC. The lint as currently posed would likely warn on a lot of good code, more than the bad code.

Also 99% of the people who would enable this by default probably would also be okay with #[warn(non_ascii_idents)] IMO. Otherwise I would think this makes some sense as a restriction lint.

There's a way to do this better that I already proposed, which would work as a pedantic lint, though it's trickier to implement.

[Manishearth]

Another thing that I would be fine with adding would be a restriction lint that lets you allowlist the set of scripts allowed in your codebase based on a clippy config.

[popzxc]

Well, actually it makes sense.

Gonna implement.

[popzxc]

I think it's done.
You can see updated test examples here.

Does it look better now?

[flip1995]

r? @Manishearth (reassigning since @phansch is taking a break)

[Manishearth]

this should probably be a pedantic lint

still kinda feel like a restriction lint with a list of scripts is the more extensible way to go.

[Manishearth]

Okay so a problem with this is that not all writing systems have uppercase. More thought needs to be put into how that will work.

[Manishearth]

But perhaps using only mixed script confusables without underscores in a character is enough for us for now. Idk.

[popzxc]

still kinda feel like a restriction lint with a list of scripts is the more extensible way to go.

I was going to implement it after this one gets merged, as script-detection dependency is introduced here.

I don't think that having two lints in one PR is going to make things easier, assuming that it's already kinda sensitive 😅

[popzxc]

However, this going farther and farther from my original intent, and become more and more complicated.
I guess it makes no sense to build a hardly-usable Frankenstein with blurry use-cases (initially I wanted to provide simple lint that is pretty restrictive, but now it going towards an alternative version of mixed_script_confusables, which is definitely not something I was aiming for).

I'll close this PR and will implement a restriction lint for locales instead.

[Manishearth]

Thank you!