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.

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

Consider NFC normalization of idents? #6058

Open
workingjubilee opened this issue Feb 6, 2024 · 10 comments
Open

Consider NFC normalization of idents? #6058

workingjubilee opened this issue Feb 6, 2024 · 10 comments

Comments

@workingjubilee
Copy link
Member

Raised by @Jules-Bertholet in rust-lang/rust#120697

It could be desirable to NFC-normalize identifiers so as to remove these kinds of inconsistencies.

However, it seems plausible that this may cause issues if the identifiers are not local, e.g. they come from another crate that does not run rustfmt.

It may make sense, however, if rustc itself linted against non-NFC-normalized idents?

@Jules-Bertholet
Copy link
Contributor

this may cause issues if the identifiers are not local, e.g. they come from another crate that does not run rustfmt

I don't see how?

@workingjubilee
Copy link
Member Author

@Jules-Bertholet Wait, maybe my brain is just slow (I'm overdue for lunch...), does rustc fully NFC-normalize idents internally when resolving them?

@Jules-Bertholet
Copy link
Contributor

@Jules-Bertholet Wait, maybe my brain is just slow (I'm overdue for lunch...), does rustc fully NFC-normalize idents internally when resolving them?

Yes.

@workingjubilee
Copy link
Member Author

workingjubilee commented Feb 6, 2024

Oh, no issue then! In fact it would help motivate rustc linting since then we could just expect people to run rustfmt.

@ytmimi
Copy link
Contributor

ytmimi commented Feb 6, 2024

@Jules-Bertholet @workingjubilee Do either of you know if there are crates that already implement the normalization?

@ytmimi
Copy link
Contributor

ytmimi commented Feb 7, 2024

@Jules-Bertholet Ahhh nice! Very good to know that something like this already exists. Thanks for the link.

However, it seems plausible that this may cause issues if the identifiers are not local, e.g. they come from another crate that does not run rustfmt.

Just want to make sure I understand correctly. Neither of you think this would be an issue?

@Jules-Bertholet
Copy link
Contributor

Just want to make sure I understand correctly. Neither of you think this would be an issue?

Correct, AIUI. Even if a dependency uses non-NFC idents internally, they can be referred to by dependents with their NFC forms.

@workingjubilee
Copy link
Member Author

Just want to make sure I understand correctly. Neither of you think this would be an issue?

Yes. I was a bit slow on the uptake and hadn't remembered yet that we NFC idents (because of course we should). Applying NFC on format means that the visual appearance of the normalized identifiers will not be changed as a result.

@ytmimi
Copy link
Contributor

ytmimi commented Feb 7, 2024

Thank you both for the confirmation. In that case, I think we could add an opt-in configuration option to normalize idents.

I'm pretty sure all idents get rewritten in rewrite_ident:

rustfmt/src/utils.rs

Lines 27 to 29 in ead0fc9

pub(crate) fn rewrite_ident<'a>(context: &'a RewriteContext<'_>, ident: symbol::Ident) -> &'a str {
context.snippet(ident.span)
}

Probably makes sense to return a Cow<'a str> in case we don't need to normalize anything. If either of you (or anyone else) wants to work on this you can find docs for adding new configuration options here, and I'd be happy to review a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants