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

feat(toml): Warn on unset Edition #13505

Merged
merged 6 commits into from Mar 1, 2024
Merged

feat(toml): Warn on unset Edition #13505

merged 6 commits into from Mar 1, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Feb 29, 2024

What does this PR try to resolve?

On Internals, the idea came up for warning on unset Edition.

Besides helping people who forget to set the Edition, this creates symmetry between Cargo.toml and cargo scripts (#12207). While the default is different in each case, we are making the default obvious and guiding people away from it.

How should we test and review this PR?

There are separate commits for adding tests (and refactors) so the changes in behavior will be more obvious

Additional information

This builds on

@rustbot
Copy link
Collaborator

rustbot commented Feb 29, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 29, 2024
@epage epage force-pushed the edition-warn branch 3 times, most recently from d56ad4a to 768c70a Compare March 1, 2024 15:40
p.cargo("check -v")
.with_stderr(
"\
[WARNING] no edition set: defaulting to the 2015 edition while 2018 is compatible with `rust-version`
Copy link
Member

Choose a reason for hiding this comment

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

Should we call out which package it is? There might be a workspace containing tons of members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like its done for us:

                        // In a workspace, it can be confusing where a warning
                        // originated, so include the path.
                        format!("{}: {}", path.display(), warning.message)

The other warnings I looked at do not mention the package which is why I dug into it to find that code. The test case must be only for a single package so it gets elided.

@weihanglo
Copy link
Member

Given edition 2015 is quite old, I assume most people won't be flooded with this error messages. If it turns out to be a real problem, we can consider revert.

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 1, 2024

📌 Commit 768c70a has been approved by weihanglo

It is now in the queue for this repository.

@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 Mar 1, 2024
@bors
Copy link
Collaborator

bors commented Mar 1, 2024

⌛ Testing commit 768c70a with merge f772ec0...

@bors
Copy link
Collaborator

bors commented Mar 1, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing f772ec0 to master...

@bors bors merged commit f772ec0 into rust-lang:master Mar 1, 2024
21 checks passed
@epage epage deleted the edition-warn branch March 2, 2024 00:43
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2024
Update cargo

12 commits in 8964c8ccff6e420e2a38b8696d178d69fab84d9d..f772ec0224d3755ce52ac5128a80319fb2eb45d0
2024-02-27 19:22:46 +0000 to 2024-03-01 22:57:35 +0000
- feat(toml): Warn on unset Edition (rust-lang/cargo#13505)
- fix(msrv): Report all incompatible packages, not just a random one (rust-lang/cargo#13514)
- refactor: abstract `std::fs` away from on-disk index cache (rust-lang/cargo#13515)
- chore(deps): update compatible (rust-lang/cargo#13507)
- chore(deps): update rust crate rusqlite to 0.31.0 (rust-lang/cargo#13510)
- [docs]: Clarify vendored sources as read-only and way to modify (rust-lang/cargo#13512)
- chore(deps): update rust crate supports-hyperlinks to v3 (rust-lang/cargo#13511)
- refactor: Clarify more Config -> Context (rust-lang/cargo#13506)
- test: Make `edition` explicit in packages (rust-lang/cargo#13504)
- Add all unit's children recursively for `doc.extern-map` option (rust-lang/cargo#13481)
- fix(rustc): Always pass --edition to rustc (rust-lang/cargo#13499)
- Silently ignore `cargo::rustc-check-cfg` to avoid MSRV annoyance when stabilizing `-Zcheck-cfg` (rust-lang/cargo#13438)

r? ghost
@rustbot rustbot added this to the 1.78.0 milestone Mar 2, 2024
@taiki-e
Copy link
Member

taiki-e commented Mar 3, 2024

Is it possible to silence this warning to the workspace contains a crate that have rust-version < 1.31?

I don't think this warning is useful at all for crates with MSRV lower than 1.31, as adding the edition will break the MSRV.

@weihanglo
Copy link
Member

I don't think this warning is useful at all for crates with MSRV lower than 1.31, as adding the edition will break the MSRV.

That sounds reasonable. However the edition field predates rust-version. How can Cargo determine MSRV when there is no rust-version set?

@taiki-e
Copy link
Member

taiki-e commented Mar 3, 2024

However the edition field predates rust-version. How can Cargo determine MSRV when there is no rust-version set?

rust-version field is just ignored on old cargo. This means a crate with old MSRV can have rust-version field. For example: https://github.com/serde-rs/serde/blob/v1.0.179/serde/Cargo.toml#L14

@weihanglo
Copy link
Member

rust-version field is just ignored on old cargo.

Isn't this the same as setting edition = "2015" for those package on old caroges? Also, does it make sense that a package supports old MSRV than its edition settings? For example the author only uses Cargo-specific edition features but not Rust compiler's.

Yet I understand that Cargo should be smarter to infer edition for those on old MSRV. Need to think more on it.

@taiki-e
Copy link
Member

taiki-e commented Mar 3, 2024

Isn't this the same as setting edition = "2015" for those package on old caroges?

edition = "2015" causes an error on some old versions (e.g., 1.27). See also an example in "Caveats" section in rust-lang/rust-clippy#6324.

$ cat Cargo.toml 
[package]
name = "repro"
version = "0.1.0"
edition = "2015"

$ cargo +1.27 build
error: failed to parse manifest at `/Users/taiki/projects/tmp/repro/Cargo.toml`

Caused by:
  editions are unstable

Caused by:
  feature `edition` is required

this Cargo does not support nightly features, but if you
switch to nightly channel you can add
`cargo-features = ["edition"]` to enable this feature

Most previously unstable fields have similar properties (e.g., #10954), but the rust-version field is special and is implemented in a way that it does not cause such errors in any version.

https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

The first version of Cargo that supports this field was released with Rust 1.56.0. In older releases, the field will be ignored, and Cargo will display a warning.


does it make sense that a package supports old MSRV than its edition settings?

(Assuming you are talking about a crate like: MSRV is 1.20 but the edition is 2018.)
I don't think it makes sense because the only time it doesn't cause an error is when building with a very old cargo (pre-1.27), and it is just ignored.

Also, specifying MSRV older than the edition in the rust-version field causes an error.

https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

The rust-version must be equal to or newer than the version that first introduced the configured edition.

bors added a commit that referenced this pull request Mar 4, 2024
fix(toml): Don't warn on unset Edition if only 2015 is compatible

### What does this PR try to resolve?

The warning doesn't help towards the stated goal if the only MSRV-compatible Edition is 2015 (plus, if you're MSRV is that old, you likely know better).

This also fixes a problem in #13505 where we were suggesting to set a field that might require an MSRV bump which may be unactionable to silence and we avoid warnings without an actionable way of silencing until #12235 gives us a general means.

### How should we test and review this PR?

### Additional information

This was discussed in
- #13505
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/warn.20on.20missing.20.60edition.60.20field.20in.20Cargo.2Etoml
@epage epage mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants