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.

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

Remove MAX_SUGGESTION_HIGHLIGHT_LINES #98261

Merged
merged 3 commits into from
Jun 28, 2022
Merged

Remove MAX_SUGGESTION_HIGHLIGHT_LINES #98261

merged 3 commits into from
Jun 28, 2022

Conversation

WaffleLapkin
Copy link
Member

After #97798 the MAX_SUGGESTION_HIGHLIGHT_LINES constant doesn't really make sense since we always show full suggestions. This PR removes last usages of the constant and the constant itself.

r? @flip1995 (this mostly does changes in clippy)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 19, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2022
flip1995
flip1995 reviewed Jun 22, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Great! So removing those extra checks from Clippy seemed to just have worked. Only a small comment.

src/tools/clippy/tests/ui/map_flatten_fixable.fixed
Comment on lines -62 to -63
// we need some newlines
// so that the span is big enough
Copy link
Member

Choose a reason for hiding this comment

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

Those lines are probably there in order to test that also with more than 6 lines the message is displayed correctly. Can you add those back an re-bless the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's weird, because I didn't actually change anything here, the original file doesn't have these too:

rust/src/tools/clippy/tests/ui/map_flatten_fixable.rs

Lines 56 to 67 in 10f4ce3

fn issue8878() {
std::collections::HashMap::<u32, u32>::new()
.get(&0)
.map(|_| {
// we need some newlines
// so that the span is big enough
// for a splitted output of the diagnostic
Some("")
// whitespace beforehand is important as well
})
.flatten();
}

These were removed by x test clippy --bless

Copy link
Member Author

@WaffleLapkin WaffleLapkin Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The commit that added this test, rust-lang/rust-clippy@22673bc for some reason has more comments in the .fixed file, than in the .rs file.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so this was a bug before, but this PR now fixed that bug. Awesome! Thanks for pointing this out.

src/tools/clippy/tests/ui/or_fun_call.stderr Show resolved Hide resolved
@flip1995 flip1995 mentioned this pull request Jun 22, 2022
Closed
@flip1995
Copy link
Member

To remove the constant was already agreed on in #97798 AFAICT

Everything else are Clippy changes, so it shouldn't be a problem if I approve this PR myself.

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jun 27, 2022

📌 Commit ffb593b has been approved by flip1995

@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 Jun 27, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 28, 2022
@Dylan-DPC
Rollup merge of rust-lang#98261 - WaffleLapkin:attempt_to_remove_max_…
da898bf 
…suggestion_highlight_lines, r=flip1995

Remove `MAX_SUGGESTION_HIGHLIGHT_LINES`

After rust-lang#97798 the `MAX_SUGGESTION_HIGHLIGHT_LINES` constant doesn't really make sense since we always show full suggestions. This PR removes last usages of the constant and the constant itself.

r? `@flip1995` (this mostly does changes in clippy)
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 28, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 28, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2022
@bors
Auto merge of rust-lang#98612 - Dylan-DPC:rollup-7tasikc, r=Dylan-DPC
00ebeb8 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#97346 (Remove a back-compat hack on lazy TAIT)
 - rust-lang#98261 (Remove `MAX_SUGGESTION_HIGHLIGHT_LINES`)
 - rust-lang#98337 ([RFC 2011] Optimize non-consuming operators)
 - rust-lang#98384 (Fix RSS reporting on macOS)
 - rust-lang#98420 (translation: lint fix + more migration)
 - rust-lang#98430 (Refactor iter adapters with less macros)
 - rust-lang#98555 (Hermit: Fix initializing lazy locks)
 - rust-lang#98595 (Implement `Send` and `Sync` for `ThinBox<T>`)
 - rust-lang#98597 (Remove unstable CStr/CString change from 1.62 release note)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9b3dbb8 into rust-lang:master Jun 28, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jun 28, 2022
@Alexendoo Alexendoo mentioned this pull request Jun 30, 2022
Merged
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 30, 2022
@bors
Auto merge of #9071 - Alexendoo:8734, r=dswij
4198013 
Uncomment test for #8734

I believe the issue was an interaction between rustfix and `span_lint_and_sugg_for_edges`, so this would've been fixed by rust-lang/rust#98261 (Thanks, `@WaffleLapkin!)`

Closes #8734

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@flip1995 flip1995

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.64.0
Development

Successfully merging this pull request may close these issues.
5 participants
@WaffleLapkin @rust-highfive @flip1995 @bors @rustbot