Skip to content
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.

Sign up for GitHub

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

lint: Do not provide suggestions for non standard characters #77805

Merged
merged 1 commit into from
Nov 21, 2020
Merged

lint: Do not provide suggestions for non standard characters #77805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 35 additions & 19 deletions compiler/rustc_lint/src/nonstandard_style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,20 @@ impl NonCamelCaseTypes {
if !is_camel_case(name) {
cx.struct_span_lint(NON_CAMEL_CASE_TYPES, ident.span, |lint| {
let msg = format!("{} `{}` should have an upper camel case name", sort, name);
lint.build(&msg)
.span_suggestion(
let mut err = lint.build(&msg);
let cc = to_camel_case(name);
// We cannot provide meaningful suggestions
// if the characters are in the category of "Lowercase Letter".
if name.to_string() != cc {
Copy link
Contributor

@ogoffart ogoffart Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
if name.to_string() != cc {
if is_camel_case(cc) {

And similar could be done for the snake_case branch.

This way we would not give a suggestion if the resulting string is not conform to the style it tries to enforce.

(I'd even say that in this case, we should probably not even warn. If a mathematical symbol that does not have a representation in the given style, then it should be fine to use it)

Edit: fixed the condition

err.span_suggestion(
ident.span,
"convert the identifier to upper camel case",
to_camel_case(name),
Applicability::MaybeIncorrect,
)
.emit()
);
}

err.emit();
})
}
}
Expand Down Expand Up @@ -263,17 +269,21 @@ impl NonSnakeCase {
let sc = NonSnakeCase::to_snake_case(name);
let msg = format!("{} `{}` should have a snake case name", sort, name);
let mut err = lint.build(&msg);
// We have a valid span in almost all cases, but we don't have one when linting a crate
// name provided via the command line.
if !ident.span.is_dummy() {
err.span_suggestion(
ident.span,
"convert the identifier to snake case",
sc,
Applicability::MaybeIncorrect,
);
} else {
err.help(&format!("convert the identifier to snake case: `{}`", sc));
// We cannot provide meaningful suggestions
// if the characters are in the category of "Uppercase Letter".
if name.to_string() != sc {
// We have a valid span in almost all cases, but we don't have one when linting a crate
// name provided via the command line.
if !ident.span.is_dummy() {
err.span_suggestion(
ident.span,
"convert the identifier to snake case",
sc,
Applicability::MaybeIncorrect,
);
} else {
err.help(&format!("convert the identifier to snake case: `{}`", sc));
}
}

err.emit();
Expand Down Expand Up @@ -441,14 +451,20 @@ impl NonUpperCaseGlobals {
if name.chars().any(|c| c.is_lowercase()) {
cx.struct_span_lint(NON_UPPER_CASE_GLOBALS, ident.span, |lint| {
let uc = NonSnakeCase::to_snake_case(&name).to_uppercase();
lint.build(&format!("{} `{}` should have an upper case name", sort, name))
.span_suggestion(
let mut err =
lint.build(&format!("{} `{}` should have an upper case name", sort, name));
// We cannot provide meaningful suggestions
// if the characters are in the category of "Lowercase Letter".
if name.to_string() != uc {
err.span_suggestion(
ident.span,
"convert the identifier to upper case",
uc,
Applicability::MaybeIncorrect,
)
.emit();
);
}

err.emit();
})
}
}
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/lint/special-upper-lower-cases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// (#77273) These characters are in the general categories of
// "Uppercase/Lowercase Letter".
// The diagnostics don't provide meaningful suggestions for them
// as we cannot convert them properly.

// check-pass

#![feature(non_ascii_idents)]
#![allow(uncommon_codepoints, unused)]

struct 𝕟𝕠𝕥𝕒𝕔𝕒𝕞𝕖𝕝;
//~^ WARN: type `𝕟𝕠𝕥𝕒𝕔𝕒𝕞𝕖𝕝` should have an upper camel case name

// FIXME: How we should handle this?
struct 𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝;
//~^ WARN: type `𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝` should have an upper camel case name
Comment on lines +14 to +16
Copy link
Member Author

Choose a reason for hiding this comment

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

@estebank Do you have any ideas to improve this case?

Copy link

Choose a reason for hiding this comment

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

we can probably improve this in a separate pr if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

@JohnTitor I just took a look at this. The problem is that is_lowercase(𝕟) is true but to_uppercase(𝕟) == 𝕟. Changing char_has_case to instead of asking whether the char is lowercase ^ uppercase, we can do c.to_lowercase().to_string() != c.to_uppercase().to_string(). That check would be more reliable to us to decide how to reconstitute the suggestion. But I would go one step further and say that maybe we shouldn't be warning for structs that have a first char that is lowercase but can't be uppercased 🤔

(Yes, I know this is 3 months later. I've had this tab open since then to look into this once I had enough time 😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of the suggestion given in #77805 (comment) : check if the tranformed string would also produce a warning, and don't warn if it does.

Copy link
Contributor

@estebank estebank Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That could make sense. After all the PascalCase convention for types is good to differentiate them from bindings and functions, but when writing a struct or trait called 𝛅 you probably are encoding "business logic" with prior conventions. It doesn't make that much sense to complain about it. The problem is that then we wouldn't lint any of these cases then. Maybe we need two lints, for different levels of checks: one for the "simple" rule check of what PascalCase is "supposed to be" and another for the more lenient but also more useful in practice check of "can this string be mechanically changed into a valid form". But that would be... likely controversial. I guess if you're doing "mathy" things, you would just have to disable the lint...

I am somewhat surprised at the behavior here because although 𝕟 doesn't have a corresponding upper-case character, 𝕞 does: 𝕄, but to_uppercase doesn't touch it, it ignores all math symbols and gives you the same symbol, which is why the to_lowercase(x) != to_uppercase(x) check would be more resilient.


static 𝗻𝗼𝗻𝘂𝗽𝗽𝗲𝗿𝗰𝗮𝘀𝗲: i32 = 1;
//~^ WARN: static variable `𝗻𝗼𝗻𝘂𝗽𝗽𝗲𝗿𝗰𝗮𝘀𝗲` should have an upper case name

fn main() {
let 𝓢𝓝𝓐𝓐𝓐𝓐𝓚𝓔𝓢 = 1;
//~^ WARN: variable `𝓢𝓝𝓐𝓐𝓐𝓐𝓚𝓔𝓢` should have a snake case name
}
32 changes: 32 additions & 0 deletions src/test/ui/lint/special-upper-lower-cases.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
warning: type `𝕟𝕠𝕥𝕒𝕔𝕒𝕞𝕖𝕝` should have an upper camel case name
--> $DIR/special-upper-lower-cases.rs:11:8
|
LL | struct 𝕟𝕠𝕥𝕒𝕔𝕒𝕞𝕖𝕝;
| ^^^^^^^^^
|
= note: `#[warn(non_camel_case_types)]` on by default

warning: type `𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝` should have an upper camel case name
--> $DIR/special-upper-lower-cases.rs:15:8
|
LL | struct 𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝;
| ^^^^^^^^^^^ help: convert the identifier to upper camel case: `𝕟𝕠𝕥𝕒𝕔𝕒𝕞𝕖𝕝`

warning: static variable `𝗻𝗼𝗻𝘂𝗽𝗽𝗲𝗿𝗰𝗮𝘀𝗲` should have an upper case name
--> $DIR/special-upper-lower-cases.rs:18:8
|
LL | static 𝗻𝗼𝗻𝘂𝗽𝗽𝗲𝗿𝗰𝗮𝘀𝗲: i32 = 1;
| ^^^^^^^^^^^^
|
= note: `#[warn(non_upper_case_globals)]` on by default

warning: variable `𝓢𝓝𝓐𝓐𝓐𝓐𝓚𝓔𝓢` should have a snake case name
--> $DIR/special-upper-lower-cases.rs:22:9
|
LL | let 𝓢𝓝𝓐𝓐𝓐𝓐𝓚𝓔𝓢 = 1;
| ^^^^^^^^^
|
= note: `#[warn(non_snake_case)]` on by default

warning: 4 warnings emitted