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

Warn if an item coming from more recent version than MSRV is used #12160

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 17, 2024

Part of #6324.

Currently, the lint is not working for the simple reason that the stable attribute is not kept in dependencies. I'll send a PR to rustc to see if they'd be okay with keeping it.

EDIT: There was actually a lookup_stability function providing this information, so all good now!

cc @epage

changelog: create new [incompatible_msrv] lint

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2024

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 17, 2024
@GuillaumeGomez GuillaumeGomez marked this pull request as ready for review January 17, 2024 16:52
@GuillaumeGomez GuillaumeGomez force-pushed the incompatible-msrv branch 4 times, most recently from 63cd7d0 to 2954332 Compare January 17, 2024 21:52
@GuillaumeGomez
Copy link
Member Author

r? @blyxyas

@rustbot rustbot assigned blyxyas and unassigned dswij Jan 17, 2024
@blyxyas
Copy link
Member

blyxyas commented Jan 21, 2024

I'm not sure if this should be on suspicious or correctness. A MSRV setting is very important for people that take them into account.

Although I'm not sure why Clippy has a MSRV concept though, as if a project has a MSRV CI would already build it with that Rust version... But those are philosophical questions we don't do (・∀・)

@epage
Copy link

epage commented Jan 21, 2024

Although I'm not sure why Clippy has a MSRV concept though, as if a project has a MSRV CI would already build it with that Rust version... But those are philosophical questions we don't do (・∀・)

For myself, I see this being useful for catching problems in development (since enough people run clippy locally, whether directly or via editor plugins) rather than waiting until a PR is posted and waiting for the CI run to fail.

@GuillaumeGomez
Copy link
Member Author

Same as @epage for me. I can think of one extra case: people not ensuring that in CI (bad practice but I think it's definitely not uncommon) and who discover the MSRV breakage when they try to publish.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Very good first iteration! Meow meow, just a few nits and questions, but I'll approve this one already (it could be merged in this state, but I just want to clarify a few things)

///
/// ### Why is this bad?
///
/// It would prevent this project to be actually used with the specified MSRV.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// It would prevent this project to be actually used with the specified MSRV.
/// It would prevent the crate to be actually used with the specified MSRV.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. 👍

Comment on lines +68 to +71
} => Some(RustcVersion::new(
version.major as _,
version.minor as _,
version.patch as _,
)),
Copy link
Member

Choose a reason for hiding this comment

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

Meow (does this work?)

Suggested change
} => Some(RustcVersion::new(
version.major as _,
version.minor as _,
version.patch as _,
)),
} => Some(version),

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't work unfortunately:

error[E0308]: `if` and `else` have incompatible types
  --> clippy_lints/src/incompatible_msrv.rs:72:16
   |
62 |            let version = if let Some(version) = tcx
   |  ________________________-
63 | |              .lookup_stability(def_id)
64 | |              .and_then(|stability| match stability.level {
65 | |                  StabilityLevel::Stable {
...  |
71 | |              version
   | |              ------- expected because of this
72 | |          } else if let Some(parent_def_id) = tcx.opt_parent(def_id) {
   | | ________________^
73 | ||             self.get_def_id_version(tcx, parent_def_id)
74 | ||         } else {
75 | ||             RustcVersion::new(1, 0, 0)
76 | ||         };
   | ||         ^
   | ||_________|
   |  |_________`if` and `else` have incompatible types
   |            expected `RustcVersion`, found `rustc_semver::RustcVersion`
   |
   = note: `rustc_semver::RustcVersion` and `RustcVersion` have similar names, but are actually distinct types
note: `rustc_semver::RustcVersion` is defined in crate `rustc_semver`
  --> /home/imperio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-semver-1.1.0/src/lib.rs:58:1
   |
58 | pub enum RustcVersion {
   | ^^^^^^^^^^^^^^^^^^^^^
note: `RustcVersion` is defined in crate `rustc_session`


#[allow(clippy::cast_lossless)]
fn get_def_id_version(&mut self, tcx: TyCtxt<'_>, def_id: DefId) -> RustcVersion {
if let Some(version) = self.is_above_msrv.get(&def_id) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this light caching, it does indeed work! yeehaw 🤠

Copy link
Member Author

Choose a reason for hiding this comment

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

😃

Comment on lines 111 to 117
fn enter_lint_attrs(&mut self, cx: &LateContext<'tcx>, attrs: &'tcx [Attribute]) {
self.msrv.enter_lint_attrs(cx.tcx.sess, attrs);
}

fn exit_lint_attrs(&mut self, cx: &LateContext<'tcx>, attrs: &'tcx [Attribute]) {
self.msrv.exit_lint_attrs(cx.tcx.sess, attrs);
}
Copy link
Member

Choose a reason for hiding this comment

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

You can use extract_msrv_attr!, if you want :3 although this approach doesn't disgust me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Knowing about this would have saved me so much XD

/// available in your current MSRV.
#[clippy::version = "1.77.0"]
pub INCOMPATIBLE_MSRV,
suspicious,
Copy link
Member

Choose a reason for hiding this comment

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

So... What do you think about putting this lint in correctness?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's pretty low risk (ie, warning will very likely always be right) to keep it into suspicious. Happened to me a few times and I would have loved to know earlier on that I was doing something wrong.

@GuillaumeGomez GuillaumeGomez force-pushed the incompatible-msrv branch 2 times, most recently from 2093bf1 to fb77402 Compare January 25, 2024 10:41
@GuillaumeGomez
Copy link
Member Author

Applied suggestions (thanks!) and replied to questions.

Comment on lines +61 to +67
let version = if let Some(version) = tcx
.lookup_stability(def_id)
.and_then(|stability| match stability.level {
StabilityLevel::Stable {
since: StableSince::Version(version),
..
} => Some(RustcVersion::new(
Copy link

Choose a reason for hiding this comment

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

In case stable gets stabilized, should we guard this to be exclusive to certain crates? Or should we wait until that wished for day to arrive?

Copy link

Choose a reason for hiding this comment

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

(and yes, when its stabilized, I would love to tell clippy or rustc the minimum version for every dependency and have a lint to catch crate dependencies and not just std/alloc/core)

INCOMPATIBLE_MSRV,
span,
&format!(
"current MSRV is `{}` but this item is stable since `{version}`",
Copy link

Choose a reason for hiding this comment

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

Should we spell out MSRV for those less familiar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that. 👍

/// To fix this problem, either increase your MSRV or use another item
/// available in your current MSRV.
#[clippy::version = "1.77.0"]
pub INCOMPATIBLE_MSRV,
Copy link

Choose a reason for hiding this comment

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

I'm a bit hesitant about using "msrv" because (1) it is an acronym and (2) not everyone knows what it means.

Would "incompatible_rust_version` work for a lint name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum... What do you think about incompatible_minimum_rust_version?

Copy link

Choose a reason for hiding this comment

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

No meaningful preference either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go for the more explicit one then.

@GuillaumeGomez
Copy link
Member Author

Renamed and update the error message to clear up confusion.

@blyxyas
Copy link
Member

blyxyas commented Jan 25, 2024

I'm not sure about the new changes.
If a codebase has a MSRV, then either

  1. The codebase is made by 1 author, in which case the MSRV has been set by that author
  2. The codebase is made by multiple authors, in which case the concept of an MSRV should be communicated in a contributors guide, by having a mentor tell you that, or seeing the #![clippy::msrv] attribute.

Hmmm, the case where spelling it would be useful would be if: An author that has not been communicated of the existence of the MSRV in that codebase (No contributing guide / mentorship), is using a function stabilized later than the MSRV, hasn't seen the #[clippy::msrv] attribute and doesn't have access to a search engine / the Rust book.


If you want to cover that edge case, we can presume that the same person that wrote #![clippy::msrv] also wrote #![deny(incompatible_msrv)]. So I'm opposed to changing the lint name, the lint message can be changed with no worries.

@epage
Copy link

epage commented Jan 25, 2024

True, clippy::msrv uses the acronym as well, which is unfortunate. Isn't clippy::msrv defaulted from package.rust-version which most people will be setting instead? And for those users, trying to find a lint for rust-version is going to be hampered by the acronym.

I said clippy::msrv is unfortunate because acronyms impede communication, especially the more specialized they are. I expect most people coming from other ecosystems won't be able to recognize MSRV and MSRV won't be covered in most introductory material, so this creates a gap for people's on-boarding journey. Personally, I feel like this reads a lot less professionally/polished with "MSRV" being used.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jan 25, 2024

I removed the last commit (which renamed the lint). I also agree with @epage but whatever clippy maintainers prefer. :)

@blyxyas
Copy link
Member

blyxyas commented Jan 26, 2024

I agree that acronyms can impede communications, but in this case where we have a clippy::msrv attribute / msrv configuration and this lint in question is suspicious (warn by default).

So the only time where an user would use an attribute would be to either deny the lint or allow it. Both scenarios happen where the user already knows of the lint's existence (and thus, the lint message already explained).

Also, the user can use cargo clippy --explain <LINT> at any time, in the case that the person doesn't have access to a search engine.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

A mini-nit on the test and this is merge-ready!

// Stable since 1.10, so should not warn.
assert_eq!(map.entry("poneyland").key(), &"poneyland");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Stable since 1.10, so should not warn.
assert_eq!(map.entry("poneyland").key(), &"poneyland");
assert_eq!(map.entry("poneyland").key(), &"poneyland");
//~^ ERROR: is `1.3.0` but this item is stable since `1.10.0`

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@blyxyas
Copy link
Member

blyxyas commented Jan 26, 2024

=^w^= great! Could you squash these commits?

@GuillaumeGomez
Copy link
Member Author

Done as well.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

@blyxyas
Copy link
Member

blyxyas commented Jan 26, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2024

📌 Commit 14e1520 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 26, 2024

⌛ Testing commit 14e1520 with merge 8de9d8c...

@bors
Copy link
Contributor

bors commented Jan 26, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 8de9d8c to master...

@bors bors merged commit 8de9d8c into rust-lang:master Jan 26, 2024
8 checks passed
@GuillaumeGomez GuillaumeGomez deleted the incompatible-msrv branch January 26, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants