Skip to content

Improve invalid UTF-8 lint by finding the expression initializer #115257

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

Jump to bottom
Merged
merged 2 commits into from
Sep 21, 2023
Merged

Improve invalid UTF-8 lint by finding the expression initializer #115257

merged 2 commits into from
Sep 21, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Aug 26, 2023

This PR introduce a small mechanism to walk up the HIR through bindings, if/else, consts, ... when trying lint on invalid UTF-8.

Fixes #115208

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2023

r? @TaKO8Ki

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

@rustbot rustbot added 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 Aug 26, 2023
@Urgau Urgau force-pushed the invalid-utf8-walk-up-hir branch from 0788854 to 9b78b29 Compare August 27, 2023 10:10
@Urgau
Copy link
Member Author

Urgau commented Aug 28, 2023

Discussion about the current approach is being discussed on Zulip

r? @WaffleLapkin

@rustbot rustbot assigned WaffleLapkin and unassigned TaKO8Ki Aug 28, 2023
@Urgau Urgau force-pushed the invalid-utf8-walk-up-hir branch from 9b78b29 to b974232 Compare September 6, 2023 09:32
@Urgau
Copy link
Member Author

Urgau commented Sep 6, 2023

Per discussion on Zulip, I've ported clippy_utils::expr_or_init to LateContext and adjusted the invalid utf8 lints to use it.

@WaffleLapkin, should be ready for a review.
@rustbot ready

@Urgau Urgau changed the title Walk up the HIR when trying lint on invalid UTF-8 Improve invalid UTF-8 lint by finding the expression initializer Sep 6, 2023
@WaffleLapkin
Copy link
Member

I have a slight concern about walking HIR to get values, this doesn't sit right with me. HIR doesn't seem like the right IR to do such dataflow-ish analysis. Similarly the error kinds that this lint now looks through seems a bit arbitrary.

That said, this is still a useful lint change, so I'd like another opinion here:
r? @Nilstrieb

As a side note, maybe the lint docs should be changed, so they don't say "literal". Although I'm not sure what would be the best way to describe the lint's new behavior.

@rustbot rustbot assigned Noratrieb and unassigned WaffleLapkin Sep 18, 2023
Noratrieb
Noratrieb reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

I agree with waffle that the lint docs should be changed and don't have any great ideas either.

While I don't love this code, it looks good enough and is a clear improvement that can also be applied to other lints, so I'm fine with accepting it.

r=me after updating the docs.

@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@Urgau Urgau force-pushed the invalid-utf8-walk-up-hir branch from b974232 to f86ef44 Compare September 19, 2023 20:20
@Urgau
Copy link
Member Author

Urgau commented Sep 19, 2023 •
edited
Loading

I tried to keep it simple, so I've added the phrasing modulo bindings at the end:

- with an invalid UTF-8 literal.
+ with an invalid UTF-8 literal (modulo bindings).

While I don't love this code, it looks good enough and is a clear improvement that can also be applied to other lints, so I'm fine with accepting it.

I agree with you both, HIR doesn't seems to me either as the right place to do such analysis but I don't know a better place to do it.

r=me after updating the docs.

Done, but I don't have r+ rights.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2023
Noratrieb
Noratrieb reviewed Sep 21, 2023
View reviewed changes
@Urgau Urgau force-pushed the invalid-utf8-walk-up-hir branch from f86ef44 to f156d3b Compare September 21, 2023 08:16
@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 21, 2023

📌 Commit f156d3b has been approved by Nilstrieb

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 Sep 21, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2023
@bors
Auto merge of rust-lang#116027 - GuillaumeGomez:rollup-3zdi9lf, r=Gui…
66ab7e6 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#115257 (Improve invalid UTF-8 lint by finding the expression initializer)
 - rust-lang#115936 (Prevent promotion of const fn calls in inline consts)
 - rust-lang#115972 (rename mir::Constant -> mir::ConstOperand, mir::ConstKind -> mir::Const)
 - rust-lang#116007 (Call panic_display directly in const_panic_fmt.)
 - rust-lang#116019 (Delete obsolete `--disable-per-crate-search` rustdoc flag)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9ce64ba into rust-lang:master Sep 21, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 21, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2023
@rust-timer
Unrolled build for rust-lang#115257
04e5f50 
Rollup merge of rust-lang#115257 - Urgau:invalid-utf8-walk-up-hir, r=Nilstrieb

Improve invalid UTF-8 lint by finding the expression initializer

This PR introduce a small mechanism to walk up the HIR through bindings, if/else, consts, ... when trying lint on invalid UTF-8.

Fixes rust-lang#115208
@oriongonza
Copy link
Contributor

Great stuff

@Urgau Urgau deleted the invalid-utf8-walk-up-hir branch September 23, 2023 22:08
@Urgau Urgau mentioned this pull request Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb approved these changes

Assignees

@Noratrieb Noratrieb

Labels
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
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

invalid_from_utf8_unchecked does not fail for include_bytes!
7 participants
@Urgau @rustbot @WaffleLapkin @Noratrieb @bors @oriongonza @TaKO8Ki