lint: Do not provide suggestions for non standard characters #77805
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @estebank |
| // FIXME: How we should handle this? | ||
| struct 𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝; | ||
| //~^ WARN: type `𝕟𝕠𝕥_𝕒_𝕔𝕒𝕞𝕖𝕝` should have an upper camel case name |
There was a problem hiding this comment.
@estebank Do you have any ideas to improve this case?
There was a problem hiding this comment.
we can probably improve this in a separate pr if needed
There was a problem hiding this comment.
@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 😅)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
camelid
added
the
A-lint
camelid
added
S-waiting-on-review
|
looks good to me @bors r+ rollup |
|
📌 Commit 410fc0e has been approved by |
bors
added
S-waiting-on-bors
| 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 { |
There was a problem hiding this comment.
Suggestion:
| 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
…=Dylan-DPC lint: Do not provide suggestions for non standard characters Fixes rust-lang#77273 Only provide suggestions if the case-fixed result is different than the original.
|
☀️ Test successful - checks-actions |
Fix invalid camel case suggestion involving unicode idents Follow up to rust-lang#77805.
Fix invalid camel case suggestion involving unicode idents Follow up to rust-lang#77805.
Fixes #77273
Only provide suggestions if the case-fixed result is different than the original.