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

Lint against escape sequences in Fluent files #109700

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

clubby789
Copy link
Contributor

Fixes #109686 by checking for \n, \" and \' in Fluent files. It might be useful to have a way to opt out of this check, but all messages with violations currently do seem to be incorrect.

@rustbot
Copy link
Collaborator

rustbot commented Mar 28, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2023
@clubby789
Copy link
Contributor Author

Actually, might this make more sense to do during compiling?

@compiler-errors
Copy link
Member

compiler-errors commented Mar 29, 2023

I think this could happen during compilation and doesn't need to be a tidy check, yeah.

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Mar 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2023

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Cool, glad that this was able to find regressions in the existing diagnostics.

@compiler-errors
Copy link
Member

r=me -- if you want to add a test, or not that's also probably ok

@bors delegate+

@bors
Copy link
Contributor

bors commented Mar 29, 2023

✌️ @clubby789 can now approve this pull request

@clubby789
Copy link
Contributor Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Mar 29, 2023

📌 Commit 979c265 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2023
LL | fluent_messages! { "./invalid-escape.ftl" }
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note: os-specific message
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit interesting, it's caused by // normalize-stderr-test "note.*" -> "note: os-specific message" in test.rs. It was added because the "could not open Fluent resource" error contains an OS specific error as the note. I think it would be nice if this replacement didn't affect this error.

To give two suggestions how to address this, either one could extend the note of the "could not open" error to something that a better pattern can be made for, or one could move that error into a different file.

Copy link
Member

Choose a reason for hiding this comment

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

Made a PR: #109798

@est31
Copy link
Member

est31 commented Mar 29, 2023

It might be useful to have a way to opt out of this check

One can use fluent quoted strings to avoid the check, if needed: For \n and \' one can do \{"n"} and \{"'"}. For \" one can do \{"\u0022"} (as the lint from my understanding would fire for \{"\""}) :).

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 29, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107387 (Use random `HashMap` keys on Hermit)
 - rust-lang#109511 (Make `EvalCtxt`'s `infcx` private)
 - rust-lang#109554 (Suggest ..= when someone tries to create an overflowing range)
 - rust-lang#109675 (Do not consider elaborated projection predicates for objects in new solver)
 - rust-lang#109693 (Remove ~const from alloc)
 - rust-lang#109700 (Lint against escape sequences in Fluent files)
 - rust-lang#109716 (Move `mir::Field` → `abi::FieldIdx`)
 - rust-lang#109726 (rustdoc: Don't strip crate module)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5937ec1 into rust-lang:master Mar 30, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 30, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 31, 2023
fluent_messages macro: don't emit the OS error in a note

This makes it possible to make the normalization of the error message precise, allowing us to not normalize all notes away. See rust-lang#109700 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove and tidy lint against rust str escape characters in ftl files
6 participants