Skip to content

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

Merged
bors merged 2 commits intorust-lang:masterfrom
Urgau:invalid-utf8-walk-up-hir
Sep 21, 2023
Merged

Improve invalid UTF-8 lint by finding the expression initializer#115257
bors merged 2 commits intorust-lang:masterfrom
Urgau:invalid-utf8-walk-up-hir

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

WaffleLapkin commented Sep 18, 2023

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

Noratrieb commented Sep 21, 2023

@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

oriongonza commented Sep 21, 2023

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