new lint: global_value_marked_deprecated

[Frank-III]

part of #57

I am actually not very sure when I should use public_api_eligible or visibility_limit since we check that for trait method/type/const and enum, enum variant, but not for trait itself, impl_owner itself?

[obi1kenobi]

I am actually not very sure when I should use public_api_eligible or visibility_limit since we check that for trait method/type/const and enum, enum variant, but not for trait itself, impl_owner itself?

This is a bit of nuance, yes:

• visibility_limit is "how is the item declared". A pub trait is public, and without pub it isn't, obviously.
• public_api_eligible is "is there anything on this item directly that would prevent it from being public API?" It's a synthesis of visibility_limit, doc_hidden, and deprecated: non-pub items aren't public API eligible, and hidden items aren't either unless they are also deprecated.
• public_api along the importable_path edge is "is this specific import path public API."

Using visibility_limit to determine public API isn't sufficient by itself. For example, this item has public visibility but isn't public API:

#[doc(hidden)]
pub struct Example;

Using public_api_eligible by itself isn't sufficient either. For example, here's an instance where an item is in principle, by itself able to be public API, but still isn't public API because of surrounding reasons:

mod private {
    pub trait Sealed {}
}

You'll recognize this as a trick common in trait-sealing, where Sealed would be applied as a super-trait to traits we wish to seal. Sealed is public_api_eligible because it's a pub item that isn't hidden. But it isn't public API, because there's no publicly-visible path by which a downstream crate can import it — it cannot be used.

So, concrete advice for queries:

• For an item to be public API, visibility_limit must be public or "default" (like in enum variants, which don't have their own visibility). Including this check is merely an optimization, to quickly prune away results that won't match. Not including it won't make the lint incorrect, since public_api and public_api_eligible already take into account visibility_limit internally.
• If the item is importable (its schema shows an importable_path edge), whether it's public API is governed by the public_api property through the importable_path edge. Ignore the public_api_eligible property on such items.
• For associated items (struct fields, enum variants, trait associated types, ImplOwner methods, etc.), that item is public API if it's public_api_eligible and also the item it's associated with is public API, per the above.
• Enum variants are special, since they are the only associated item in Rust that can be imported directly. Quite frankly, I haven't worked out all the edge cases here, and cargo-semver-checks doesn't handle breakage related to directly importing variants. This is not a very common concern, but if it happens to be something you're interested in tackling, I'd be happy to point you in the right direction! Also happy to suggest other more impactful things as well!

[obi1kenobi]

It would be really nice if we could tell the user whether that specific value is a constant or static. "Global value" is a term we invented for purposes of making the schema make sense, not something well-established in Rust, so it might be confusing to users.

[obi1kenobi]

const fn is not considered a const item btw, it won't get picked up by this lint. It doesn't have a globally-available value per se. It's just a function that can be evaluated at compile-time, so it gets reported by the lint for deprecated functions. This is good! We don't want the same item to get flagged for the same issue by more than one lint — that'd just be frustrating.

[Frank-III]

Yes I knew when I ran the test, I was worried about if it would trigger both! But should I keep it?

[obi1kenobi]

A bit of a judgment call that could go either way — I'd remove them because I think there's little risk of a future false-positive here (where const fn is treated as a public constant item), and there's risk of confusion during future maintenance ("Why is const fn being tested here? Surely there's a good reason I'm just missing?").

[Frank-III]

Thank you for the detailed explanation, learnt a lot again!

• For an item to be public API, visibility_limit must be public or "default" (like in enum variants, which don't have their own visibility). Including this check is merely an optimization, to quickly prune away results that won't match. Not including it won't make the lint incorrect, since public_api and public_api_eligible already take into account visibility_limit internally.
• If the item is importable (its schema shows an importable_path edge), whether it's public API is governed by the public_api property through the importable_path edge. Ignore the public_api_eligible property on such items.
• For associated items (struct fields, enum variants, trait associated types, ImplOwner methods, etc.), that item is public API if it's public_api_eligible and also the item it's associated with is public API, per the above.

I see, very helpful advice on when to use which! I would apply this rule when I add new lints!

Enum variants are special, since they are the only associated item in Rust that can be imported directly. Quite frankly, I haven't worked out all the edge cases here, and cargo-semver-checks doesn't handle breakage related to directly importing variants. This is not a very common concern, but if it happens to be something you're interested in tackling, I'd be happy to point you in the right direction! Also happy to suggest other more impactful things as well!

Yes, I would definitely want to work on deeper things after getting familiar with the whole project more!

[obi1kenobi]

This is actually a false-positive in the pub_module_level_const_missing lint, as of Rust 1.83 which relaxed the rules on when & how static items may be used in const items. Previously, assigning a static to a const item was always a hard error. As of 1.83, certain static items can be assigned to const items so we can't prove breakage merely by saying "this item used to be const but now is static."

Care to fix that lint? All you need to do is flip the baseline type check from ... on Constant to ... on GlobalValue

If you're also interested in writing new lints that flag the new breakage behavior made possible by Rust 1.83, I'd be happy to point you in the right direction there as well!

[Frank-III]

Care to fix that lint? All you need to do is flip the baseline type check from ... on Constant to ... on GlobalValue

Definitely

If you're also interested in writing new lints that flag the new breakage behavior made possible by Rust 1.83, I'd be happy to point you in the right direction there as well!

Would like to know more about this, would you write it down in a new issue or?

[Frank-III]

Care to fix that lint? All you need to do is flip the baseline type check from ... on Constant to ... on GlobalValue

I think we want to flip current to be GlobalValue since now a const became static should be allowed?

[obi1kenobi]

Yes, of course. Thanks for the catch!

[obi1kenobi]

Nice catch!