New lint: `disallowed_script_idents` #7400

popzxc · 2021-06-25T09:54:31Z

This PR implements a new lint to restrict locales that can be used in the code,
as proposed in #7376.

Current concerns / unresolved questions:

Mixed usage of script (as a Unicode term) and locale (as something that is easier to understand for the broad audience). I'm not sure whether these terms are fully interchangeable and whether in the current form it is more confusing than helpful. script is now used everywhere.
Having to mostly copy-paste AllowedScript. Probably it's not a big problem, as the list of scripts is standardized and is unlikely to change, and even if we'd stick to the unicode_script::Script, we'll still have to implement custom deserialization, and I don't think that it will be shorter in terms of the amount of LoC. unicode::Script is used together with a filtering deserialize function.
Should we stick to the list of "recommended scripts" from UAX #31 in the configuration?

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

changelog: [`disallowed_script_idents`]

r? @Manishearth

Manishearth

Might be good to get a second set of eyes on this, cc @camsteffen

clippy_lints/src/restricted_locales.rs

Manishearth · 2021-06-25T14:41:52Z

clippy_lints/src/restricted_locales.rs

+impl_lint_pass!(RestrictedLocales => [RESTRICTED_LOCALES]);
+
+impl<'tcx> LateLintPass<'tcx> for RestrictedLocales {
+    fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {


Given that lint passes are always run, can you check if there's a measurable perf difference when linting a decently sized crate?

It seems that I cannot just build the release version of clippy and run it. clippy-driver fails with the following error: dyld: Library not loaded: @rpath/librustc_driver-22f067a38d2152ee.dylib.

If I understood things correctly from looking through this repo issues, I need the whole rust toolchain for that, which is not desirable. Is there a simpler way to run clippy on a custom crate?

You need to have the rustc-dev component installed. You're probably using a different toolchain when you build clippy vs when you run it.

Check out https://github.com/rust-lang/rust-clippy/blob/master/rust-toolchain#L3

Dunno. The compiler is (and was) overridden to be the same as for clippy itself. Components are also seem to be installed.

$ rustup +nightly-2021-06-17 component add llvm-tools-preview info: component 'llvm-tools-preview' for target 'aarch64-apple-darwin' is up to date $ rustup +nightly-2021-06-17 component add rust-src info: component 'rust-src' is up to date $ rustup +nightly-2021-06-17 component add rustc-dev info: component 'rustc-dev' for target 'aarch64-apple-darwin' is up to date $ rustup show # ... nightly-2021-06-17-aarch64-apple-darwin (directory override for '.../actix-web') # ...

Yet it still fails:

$ ../rust-clippy/target/release/clippy-driver --deny clippy::needless_borrow dyld: Library not loaded: @rpath/librustc_driver-22f067a38d2152ee.dylib Referenced from: /<...>/actix-web/../rust-clippy/target/release/clippy-driver Reason: image not found [1] 32340 abort ../rust-clippy/target/release/clippy-driver --deny clippy::needless_borrow

If that's important, I build clippy via simple cargo build --release (maybe I need to do it some other way?).

clippy_lints/src/utils/conf.rs

clippy_lints/src/restricted_locales.rs

camsteffen · 2021-06-25T16:56:11Z

This lint is analogous to non_ascii_idents. I think we should add "See also: non_ascii_idents" to the docs. And maybe the lint name should have some similarity, like disallowed_script_idents.

Manishearth · 2021-06-25T19:14:49Z

Yeah, I like that!

camsteffen · 2021-06-25T17:43:09Z

clippy_lints/src/restricted_scripts.rs

+/// Parses the `unicode_script::Script` object, filtering
+/// bad values and undesired scripts (such as `Common` and `Unknown`).
+#[allow(clippy::too_many_lines)]
+fn parse_script(script_name: &str) -> Option<Script> {


I feel like there should be Script::from_alias provided by the unicode-script crate. @Manishearth?

In any case, this should be clearly documented as expecting an alias value.

Yes, if someone is willing to make the PR I'd merge it!

There is an existing function Script::full_name which seems to be to what the Unicode docs refer to as "alias". So would you name the new function Script::from_full_name?

Would it make sense to add it now (so the new version will be released and used here), or for now it's better to implement conversion logic in clippy and replace it in one of the future releases?

I think it would be good to add it to unicode-script now. Would you be interested in doing that?

Yeah, no problem, will take care of that tomorrow.

And that code is published now

camsteffen · 2021-06-25T17:48:36Z

clippy_lints/src/restricted_scripts.rs

+    ///
+    /// For example, `Syloti Nagri` must be represented as `"Syloti_Nagri`".
+    ///
+    /// [supported_scripts]: http://www.unicode.org/standard/supported.html


I think you should reference this page instead and say to use the value in the "Alias" column. Then you don't need to say "replace spaces with underscores".

Well, it looks like it'll be more complicated to implement. I can see, for example, the alias Katakana_Or_Hiragana, which represents two scripts rather than one.
In theory it could've implemented through ScriptExtensions, but I don't see a way to construct it manually, which means that we'll have to return Vec<Script>, which is less efficient.

Do you find it significant to stick to the aliases?

Well, it looks like it'll be more complicated to implement. I can see, for example, the alias Katakana_Or_Hiragana, which represents two scripts rather than one.
In theory it could've implemented through ScriptExtensions, but I don't see a way to construct it manually, which means that we'll have to return Vec<Script>, which is less efficient.

Okay, this had me puzzled for a bit but I finally figured it out - it is in fact one script but it is no longer used in the latest version of Unicode. I think we can just ignore special cases like this. I looked at the source of the unicode-script crate and it is definitely using the "Alias" values.

Do you find it significant to stick to the aliases?

Yes, it is better to use a pre-existing naming convention.

FYI I learned about Katakana_Or_Hiragana being a special case on this page.

Oh, if you're looking for handling that special case you should use the AugmentedScriptSet property from unicode-security instead

I added a note with a link to the page about it being a special case instead. I think that it's better than adding one more dependency.

There are lots of scripts that are not used in current Unicode. Katakana_Or_Hiragana is just one of them. I don't think it's anything we need to document or worry about.

OK then, removed 😅

clippy_lints/src/restricted_scripts.rs

popzxc · 2021-06-30T13:26:46Z

So, apart from the perfing this lint (which I have some troubles with, unfortunately, so I'll be grateful if someone else have tried it), is there anything else I should improve?

Manishearth

I think this is good, and with the global bailing-out there's no need to test for perf

(Going to wait for a second review from @camsteffen)

clippy_lints/src/disallowed_script_idents.rs

camsteffen

Looks good to me!

Manishearth · 2021-06-30T16:06:03Z

@bors r+

bors · 2021-06-30T16:06:04Z

📌 Commit e2eb453 has been approved by Manishearth

New lint: `disallowed_script_idents` This PR implements a new lint to restrict locales that can be used in the code, as proposed in #7376. Current concerns / unresolved questions: - ~~Mixed usage of `script` (as a Unicode term) and `locale` (as something that is easier to understand for the broad audience). I'm not sure whether these terms are fully interchangeable and whether in the current form it is more confusing than helpful.~~ `script` is now used everywhere. - ~~Having to mostly copy-paste `AllowedScript`. Probably it's not a big problem, as the list of scripts is standardized and is unlikely to change, and even if we'd stick to the `unicode_script::Script`, we'll still have to implement custom deserialization, and I don't think that it will be shorter in terms of the amount of LoC.~~ `unicode::Script` is used together with a filtering deserialize function. - Should we stick to the list of "recommended scripts" from [UAX #31](http://www.unicode.org/reports/tr31/#Table_Recommended_Scripts) in the configuration? *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: ``[`disallowed_script_idents`]`` r? `@Manishearth`

bors · 2021-06-30T16:06:12Z

⌛ Testing commit e2eb453 with merge e3dffc0...

popzxc · 2021-06-30T18:06:19Z

Uhm. It seems that at the same time I squashed commits and bors was r+ed, which caused it to be stuck.

I think bors retry is required now @Manishearth 😅

camsteffen · 2021-06-30T18:14:11Z

@bors r=Manishearth

bors · 2021-06-30T18:14:12Z

📌 Commit 018be41 has been approved by Manishearth

bors · 2021-06-30T18:14:20Z

⌛ Testing commit 018be41 with merge cadb93b...

bors · 2021-06-30T18:29:22Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing cadb93b to master...

rust-highfive assigned Manishearth Jun 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 25, 2021

Manishearth reviewed Jun 25, 2021

View reviewed changes

popzxc changed the title ~~New lint: restricted_locales~~ New lint: restricted_scripts Jun 25, 2021

camsteffen suggested changes Jun 25, 2021

View reviewed changes

popzxc force-pushed the restrict-locales branch from 3b46f95 to 58bb8f2 Compare June 26, 2021 08:54

popzxc changed the title ~~New lint: restricted_scripts~~ New lint: disallowed_script_idents Jun 26, 2021

popzxc mentioned this pull request Jun 27, 2021

Add function to parse script from string unicode-rs/unicode-script#7

Merged

popzxc force-pushed the restrict-locales branch from d317389 to 94dd5bb Compare June 28, 2021 16:18

Manishearth approved these changes Jun 30, 2021

View reviewed changes

camsteffen reviewed Jun 30, 2021

View reviewed changes

clippy_lints/src/disallowed_script_idents.rs Outdated Show resolved Hide resolved

camsteffen approved these changes Jun 30, 2021

View reviewed changes

Implement 'disallowed_script_idents' lint

018be41

popzxc force-pushed the restrict-locales branch from e2eb453 to 018be41 Compare June 30, 2021 16:06

bors merged commit cadb93b into rust-lang:master Jun 30, 2021

This was referenced Jun 30, 2021

Add new lint: strlen_on_c_strings #7243

Merged

New lint: [self_named_constructor] #7403

Merged

popzxc deleted the restrict-locales branch June 30, 2021 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `disallowed_script_idents` #7400

New lint: `disallowed_script_idents` #7400

popzxc commented Jun 25, 2021 •

edited

Loading

Manishearth left a comment

Manishearth Jun 25, 2021

popzxc Jun 25, 2021

Manishearth Jun 28, 2021

popzxc Jun 28, 2021

camsteffen commented Jun 25, 2021

Manishearth commented Jun 25, 2021

camsteffen Jun 25, 2021

Manishearth Jun 25, 2021

camsteffen Jun 25, 2021

popzxc Jun 26, 2021

camsteffen Jun 26, 2021

popzxc Jun 26, 2021

popzxc Jun 27, 2021

Manishearth Jun 28, 2021

camsteffen Jun 25, 2021

popzxc Jun 26, 2021

camsteffen Jun 26, 2021

camsteffen Jun 26, 2021

Manishearth Jun 28, 2021

popzxc Jun 28, 2021

camsteffen Jun 28, 2021

popzxc Jun 28, 2021

popzxc commented Jun 30, 2021

Manishearth left a comment •

edited

Loading

camsteffen left a comment

Manishearth commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

popzxc commented Jun 30, 2021

camsteffen commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

New lint: disallowed_script_idents #7400

New lint: disallowed_script_idents #7400

Conversation

popzxc commented Jun 25, 2021 • edited Loading

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsteffen commented Jun 25, 2021

Manishearth commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popzxc commented Jun 30, 2021

Manishearth left a comment • edited Loading

Choose a reason for hiding this comment

camsteffen left a comment

Choose a reason for hiding this comment

Manishearth commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

popzxc commented Jun 30, 2021

camsteffen commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

bors commented Jun 30, 2021

New lint: `disallowed_script_idents` #7400

New lint: `disallowed_script_idents` #7400

popzxc commented Jun 25, 2021 •

edited

Loading

Manishearth left a comment •

edited

Loading