add `suppress_restriction_lint_in_const` config #9920

naosense · 2022-11-21T07:07:05Z

According to #9808 , add a new lint suppress_lint_in_const to report even in const context. BTW, i am not good at naming either, if anyone have a better idea, i am happy to change it.

This PR is still in progress, so i keep it draft.

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

changelog: Enhancement: [indexing_slicing]: add new config suppress-restriction-lint-in-const to enable restriction lints, even if the suggestion might not be applicable

r? @xFrendet

rust-highfive · 2022-11-21T07:07:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon.

Please see the contribution instructions for more information.

naosense · 2022-11-21T07:09:34Z

r? @xFrednet

naosense · 2022-11-22T09:26:59Z

Hey @xFrednet , i think it's ready to go. Could you please review it?

xFrednet

Really solid start! Thank you for working on this issue :)

I hope that these suggestions are fairly simple. You're welcome to reach out if you'd like any help :)

clippy_lints/src/utils/conf.rs

tests/ui-toml/suppress_lint_in_const/clippy.toml

clippy_lints/src/indexing_slicing.rs

clippy_lints/src/utils/conf.rs

bors · 2022-11-22T20:25:26Z

☔ The latest upstream changes (presumably #9924) made this pull request unmergeable. Please resolve the merge conflicts.

naosense · 2022-11-23T08:46:53Z

@xFrednet thanks for your kindness and patience! Not quite sure everything is corrected as you requested, could you please review again?

xFrednet

@xFrednet thanks for your kindness and patience!

You're very welcome. Thank you for your kindness and contribution as well!

Not quite sure everything is corrected as you requested, could you please review again?

Yes, I have one suggestion, and then it should be good to go!

clippy_lints/src/indexing_slicing.rs

naosense · 2022-11-27T14:07:29Z

ping ping @xFrednet , let me know if anything wrong with my code.😄

xFrednet · 2022-11-27T14:38:50Z

Hey @naosense, everything is fine. It can sometimes take a few days, until I and other team members have time to review a PR. If you don't hear anything, it's nothing bad and a ping after some time can be helpful.

I generally aim to give a review within 5 days, but it can go up to a week, if life gets busy or there are several other reviews waiting. This one is the next proper review in my queue. Maybe you'll get a review today, but it's more likely that it'll be tomorrow. :)

naosense · 2022-11-27T14:51:41Z

@xFrednet got it, didn't know that before, I'm fine with any time, just follow your schedule

xFrednet

The implementation looks good! The tests still need some adjustment and then we can merge this :)

tests/ui/indexing_slicing_index.stderr

tests/ui-toml/suppress_lint_in_const/clippy.toml

xFrednet · 2022-11-28T08:51:51Z

Hey @naosense

got it, didn't know that before, I'm fine with any time, just follow your schedule

No problem. It's something I needed to get used to as well. You're welcome to reach out to me here or on Zulip if you're unsure about these things :)

bors · 2022-11-28T17:39:45Z

☔ The latest upstream changes (presumably #9865) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet

Almost perfect, I found one last nit, which should be simple to fix.

Could you then squash the fix test commits into one? Afterwards, we can merge this. Thank you for the effort you put into this, it's highly appreciated :)

xFrednet · 2022-12-01T12:24:56Z

clippy_lints/src/utils/conf.rs

+    /// suggested counterparts are unavailable in constant code. This
+    /// configuration will cause restriction lints to trigger even
+    /// if no suggestion can be made.
+    (suppress_restriction_lint_in_const: bool = false),


The description sounds perfect :)

clippy_lints/src/indexing_slicing.rs

xFrednet · 2022-12-05T15:08:25Z

This looks good to me. Thank you for all the work you put into this and the quick updates. I hope you had fun working on this. It's an excellent addition to Clippy IMO.

@bors r+

bors · 2022-12-05T15:08:28Z

📌 Commit eec5039 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2022-12-05T15:08:39Z

⌛ Testing commit eec5039 with merge 9c1ba16...

add `suppress_restriction_lint_in_const` config According to #9808 , add a new lint `suppress_lint_in_const` to report even in const context. BTW, i am not good at naming either, if anyone have a better idea, i am happy to change it. This PR is still in progress, so i keep it draft. - \[x] Followed [lint naming conventions][lint_naming] - \[x] Added passing UI tests (including committed `.stderr` file) - \[x] `cargo test` passes locally - \[x] Executed `cargo dev update_lints` - \[x] Added lint documentation - \[x] Run `cargo dev fmt` changelog: Enhancement: [`indexing_slicing`]: add new config `suppress-restriction-lint-in-const` to enable restriction lints, even if the suggestion might not be applicable r? `@xFrendet`

bors · 2022-12-05T15:14:15Z

💔 Test failed - checks-action_test

xFrednet · 2022-12-05T15:16:05Z

Looks like a fluke.

@bors retry

Be nice and let the CI be green this time :)

bors · 2022-12-05T15:20:05Z

⌛ Testing commit eec5039 with merge 3af38ec...

add `suppress_restriction_lint_in_const` config According to #9808 , add a new lint `suppress_lint_in_const` to report even in const context. BTW, i am not good at naming either, if anyone have a better idea, i am happy to change it. This PR is still in progress, so i keep it draft. - \[x] Followed [lint naming conventions][lint_naming] - \[x] Added passing UI tests (including committed `.stderr` file) - \[x] `cargo test` passes locally - \[x] Executed `cargo dev update_lints` - \[x] Added lint documentation - \[x] Run `cargo dev fmt` changelog: Enhancement: [`indexing_slicing`]: add new config `suppress-restriction-lint-in-const` to enable restriction lints, even if the suggestion might not be applicable r? `@xFrendet`

bors · 2022-12-05T15:25:49Z

💔 Test failed - checks-action_test

xFrednet · 2022-12-05T15:39:50Z

It looks like something in the CI is broken. This is not specific to this PR. I'll merge it, once the problem has been resolved :)

xFrednet · 2022-12-06T18:35:30Z

Let's give this another try

@bors r+

bors · 2022-12-06T18:35:32Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Use ubuntu-20.04 instead of ubuntu-latest #10039

bors · 2022-12-06T18:35:33Z

📌 Commit eec5039 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2022-12-06T18:43:31Z

⌛ Testing commit eec5039 with merge 5d70746...

bors · 2022-12-06T18:57:02Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 5d70746 to master...

rust-highfive assigned Alexendoo Nov 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 21, 2022

rust-highfive assigned xFrednet and unassigned Alexendoo Nov 21, 2022

naosense marked this pull request as ready for review November 22, 2022 09:25

xFrednet reviewed Nov 22, 2022

View reviewed changes

clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved

tests/ui-toml/suppress_lint_in_const/clippy.toml Outdated Show resolved Hide resolved

clippy_lints/src/indexing_slicing.rs Outdated Show resolved Hide resolved

clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved

naosense force-pushed the suppress_lint_in_const branch from 4da3bbd to 9c0d3f2 Compare November 23, 2022 08:44

xFrednet changed the title ~~add suppress_lint_in_const config~~ add suppress_restriction_lint_in_const config Nov 23, 2022

xFrednet reviewed Nov 23, 2022

View reviewed changes

clippy_lints/src/indexing_slicing.rs Outdated Show resolved Hide resolved

naosense requested a review from xFrednet November 24, 2022 01:47

xFrednet reviewed Nov 28, 2022

View reviewed changes

tests/ui/indexing_slicing_index.stderr Outdated Show resolved Hide resolved

tests/ui-toml/suppress_lint_in_const/clippy.toml Outdated Show resolved Hide resolved

naosense requested a review from xFrednet November 28, 2022 09:41

naosense added 6 commits November 29, 2022 15:00

add suppress_lint_in_const conf

96f1385

add test and stderr

aed9497

update config and suggest

4528aec

resolve conflicts

c9bf4b7

change default value

1fc98c5

change note style

67a9413

naosense force-pushed the suppress_lint_in_const branch from d01f15e to 2ac83f9 Compare November 29, 2022 07:03

xFrednet reviewed Dec 1, 2022

View reviewed changes

fix test

eec5039

naosense force-pushed the suppress_lint_in_const branch from 2ac83f9 to eec5039 Compare December 3, 2022 08:07

naosense requested a review from xFrednet December 3, 2022 08:07

xFrednet approved these changes Dec 5, 2022

View reviewed changes

xFrednet added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 6, 2022

bors merged commit 5d70746 into rust-lang:master Dec 6, 2022

naosense deleted the suppress_lint_in_const branch December 6, 2022 23:47

naosense mentioned this pull request Dec 27, 2022

indexing_slicing is not triggered in const fns #9808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `suppress_restriction_lint_in_const` config #9920

add `suppress_restriction_lint_in_const` config #9920

naosense commented Nov 21, 2022 •

edited by xFrednet

Loading

rust-highfive commented Nov 21, 2022

naosense commented Nov 21, 2022

naosense commented Nov 22, 2022

xFrednet left a comment •

edited

Loading

bors commented Nov 22, 2022

naosense commented Nov 23, 2022

xFrednet left a comment

naosense commented Nov 27, 2022

xFrednet commented Nov 27, 2022

naosense commented Nov 27, 2022

xFrednet left a comment

xFrednet commented Nov 28, 2022

bors commented Nov 28, 2022

xFrednet left a comment

xFrednet Dec 1, 2022

xFrednet commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

xFrednet commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

xFrednet commented Dec 5, 2022

xFrednet commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

add suppress_restriction_lint_in_const config #9920

add suppress_restriction_lint_in_const config #9920

Conversation

naosense commented Nov 21, 2022 • edited by xFrednet Loading

rust-highfive commented Nov 21, 2022

naosense commented Nov 21, 2022

naosense commented Nov 22, 2022

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

bors commented Nov 22, 2022

naosense commented Nov 23, 2022

xFrednet left a comment

Choose a reason for hiding this comment

naosense commented Nov 27, 2022

xFrednet commented Nov 27, 2022

naosense commented Nov 27, 2022

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet commented Nov 28, 2022

bors commented Nov 28, 2022

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Dec 1, 2022

Choose a reason for hiding this comment

xFrednet commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

xFrednet commented Dec 5, 2022

bors commented Dec 5, 2022

bors commented Dec 5, 2022

xFrednet commented Dec 5, 2022

xFrednet commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

bors commented Dec 6, 2022

add `suppress_restriction_lint_in_const` config #9920

add `suppress_restriction_lint_in_const` config #9920

naosense commented Nov 21, 2022 •

edited by xFrednet

Loading

xFrednet left a comment •

edited

Loading