RFC: Deprecate the per-build-target edition field in Cargo.toml
#3772
Conversation
epage
added
the
T-cargo
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
| - doctests | ||
| - creating packages just for the sake of defining tests | ||
|
|
||
| # Rationale and alternatives |
There was a problem hiding this comment.
@clarfonthey from #3772 (comment)
I didn't even know this was an option, and I agree it's very confusing, especially considering how cargo itself is affected by editions.
I honestly would go further and propose working with the existing crates that use this feature to remove it, then disabling it for all editions in a future version. Making the presence of an edition field depend on edition feels weird.
There was a problem hiding this comment.
This seems to run counter to our compatibility guarantees. We take this approach to help people as we fix bugs that people accidentally relied on but this was intentional behavior supported in the manifest and documented as part of working with Editions. In practice, this would mean that new versions of cargo would not be able to build older versions of crates like linkme, nameof, and num-derive.
There was a problem hiding this comment.
I added d8c992a which discusses one edition field affecting another
There was a problem hiding this comment.
That's very fair, and I guess my justification comes from the fact that the feature is extremely counter-intuitive and not used very often. However, from what you've mentioned, it is used a lot in the ecosystem, just not in very many crates, which means that at least half of my argument is invalid.
So, it's fair to prefer this over an edition boundary rather than removing it outright.
text/3772-build-target-edition.md
Outdated
| A non-`None` edition will be considered deprecated | ||
| - A deprecation message will eventually be shown by Cargo | ||
| - Timing depends on if this will be blocked on having `[lints]` control over this or not | ||
| - In Edition 20XX+, an error will be reported when this field is present |
There was a problem hiding this comment.
@Lokathor from #3772 (comment)
So the error case is if you set an edition >= X in your
packageand then also set any edition value at all in particular build targets?
There was a problem hiding this comment.
Yes. I will make that more clear.
|
@rfcbot merge |
|
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
I'm checking my box, though I do note that cross-edition testing, particularly of macros, can be challenging. However, it seems evident that it is extremely rare that anyone is doing that, and there are workarounds (such as using separate test packages), and the benefits seem to outweigh that drawback to me. |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
Deprecate
lib.edition, etc in favor of only settingpackage.edition, removing the fields in the next Edition.Rendered