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.

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

[WIP] BARE_TRAIT_OBJECTS -> Deny #59482

Closed

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 28, 2019

Based on https://github.com/rust-lang/rfcs/blob/master/text/2113-dyn-trait-syntax.md#migration.

Let's first crater this for both editions to see what the fallout is. (I expect it will be large)
I suspect the likely outcome will be Warn on at least Rust 2015 for a start.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2019
@Centril
Copy link
Contributor Author

Centril commented Mar 28, 2019

Before I try this; let's let travis have a go at it.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2019

I'm probably not the most reasonable choice for a reviewer here as I'm strongly of the opinion that lints are but sound and smoke. Irrelevant of the "fallout" (in quotes, because it doesn't break dependencies, is trivial to fix, even rustfixable), I think we should just merge this. Going through a warning phase is pretty useless, as (subjectively, I have no data) Rust developers don't really commit (or at least publish) code that has warnings.

@Centril
Copy link
Contributor Author

Centril commented Mar 28, 2019

@oli-obk Oh hmm... the lint is Allow right now tho so it generates no warnings. Let's discuss on Thursday's T-Lang meeting and see how we feel about jumping straight to Deny on both editions.

@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 28, 2019
@jethrogb
Copy link
Contributor

Regarding all of these rust_2018_idioms changes, I couldn't find much previous discussion on the proposed deprecation paths, but I feel like these changes should've happened as part of the edition release and it's too late to start failing on them now.

@Centril
Copy link
Contributor Author

Centril commented Mar 29, 2019

@jethrogb This is specified in https://github.com/rust-lang/rfcs/blob/master/text/2113-dyn-trait-syntax.md#migration. This is just executing the already agreed-to plan. (And we decided that we would wait with ramping up the idioms lints until later into the edition)

@jethrogb
Copy link
Contributor

RFC 2113 just covers this PR, what about the other 3?

Also it seems bad to change a lint from allow to deny without a warning period.

@Centril
Copy link
Contributor Author

Centril commented Mar 30, 2019

@jethrogb Left comments on those.

Also it seems bad to change a lint from allow to deny without a warning period.

Please see the first comment in the PR. The idea is to measure the impact and deciding how to proceed from there.

@Centril
Copy link
Contributor Author

Centril commented Apr 4, 2019

We discussed this on this weeks lang meeting; the general consensus was to crater and see how things fall out and move on from there.

@Dylan-DPC-zz
Copy link

ping from triage, closing this due to inactivity

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2019
@Centril Centril deleted the bare_trait_object-evolution branch May 30, 2019 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants