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

Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021 #83213

Merged
merged 14 commits into from
May 4, 2021
Merged

Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021 #83213

merged 14 commits into from
May 4, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Mar 16, 2021

This addresses #81244 by updating two lints to errors in the Rust 2021 edition.

r? @estebank

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2021
@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 16, 2021 •
edited
Loading

bare_trait_objects is a case where a hard error is better reported in entirely different way than the lint.

The lint reporting is leaky (#65371) and it's not trivial to catch all the relevant cases.

If it's an error, then it would just be a regular "expected a type, found a trait" or "cannot find associated item foo in trait Tr" error in resolve.

It should be ok to land this as is though and land a proper implementation later.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

Lets update #65371 after landing this with a comment to make the lint not trigger at all in 2021 and make resolve handle it (only in 2021). It'll complicate the codebase, sadly. I am slightly concerned though if the lint isn't triggering in all cases today, then 2021 will start denying currently working code. Ideally the lint and errors would remain fully accurate. @petrochenkov do you think we could move the lint to live in resolve and be triggered there instead, to make it more accurate?

@estebank
Copy link
Contributor

r=me after fixing failures

@ehuss
Copy link
Contributor

ehuss commented Mar 17, 2021 •
edited
Loading

Just a note of concern, I believe this will break any project that allows these lints when they transition to 2021. That seems a little aggressive to me. I count about 84 projects on crates.io allowing bare-trait-object, including quite a few prominent ones (serde, proptest, error-chain, env_logger, protobuf, cc, bincode, and many more).

Can there be a more gentle way to make this transition? Maybe some way that is rustfix-able? (Like maybe making it part of the compatibility lint group.)

@rylev
Copy link
Member Author

rylev commented Mar 17, 2021

@ehuss The point of moving this to an error is force users to make the transition. That being said, we definitely need to be able to transition using rustfix. That's the next step I'd like to take care of in a separate PR.

One thing we might want to consider is to transition this lint to deny-by-default. This won't necessarily encourage those projects that allow this lint currently, but it could bring more attention to the issue.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

One thing we might want to consider is to transition this lint to deny-by-default. This won't necessarily encourage those projects that allow this lint currently, but it could bring more attention to the issue.

I'm ok with that approach, although it makes me sad to do that.

GuillaumeGomez
GuillaumeGomez requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup! Just a few other little things to update and it should be good to go!

GuillaumeGomez
GuillaumeGomez approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Thanks!

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 27, 2021
@bors
Copy link
Contributor

bors commented Apr 28, 2021

⌛ Testing commit 4193b0b with merge 4b9c3fbfa512b53dde55e3176a50e55ef480fcf0...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 28, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2021
@Mark-Simulacrum
Copy link
Member

Looks like clippy tests failed.

@Mark-Simulacrum Mark-Simulacrum 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 Apr 28, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2021
@rylev rylev requested a review from nikomatsakis April 29, 2021 16:37
@rylev rylev 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 Apr 29, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 3, 2021

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit 7a50392 has been approved by nikomatsakis

@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 May 3, 2021
@bors
Copy link
Contributor

bors commented May 4, 2021

⌛ Testing commit 7a50392 with merge 7a0f178...

@bors
Copy link
Contributor

bors commented May 4, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 7a0f178 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2021
@bors bors merged commit 7a0f178 into rust-lang:master May 4, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 4, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 6, 2021
@bors
Auto merge of rust-lang#83213 - rylev:update-lints-to-errors, r=nikom…
9735470 
…atsakis

Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021

This addresses rust-lang#81244 by updating two lints to errors in the Rust 2021 edition.

r? `@estebank`
@ghost ghost mentioned this pull request May 12, 2021
Merged
@nikomatsakis nikomatsakis mentioned this pull request May 27, 2021
Closed
4 tasks
@m-ou-se m-ou-se mentioned this pull request May 29, 2021
Closed
2 tasks
@m-ou-se m-ou-se mentioned this pull request Sep 3, 2021
Closed
@petrochenkov petrochenkov mentioned this pull request Sep 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@Manishearth Manishearth Manishearth approved these changes

@nikomatsakis nikomatsakis Awaiting requested review from nikomatsakis

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-diagnostics Area: Messages for errors, warnings, and lints disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.

None yet