Apply deprecation lint to trait method overrides.

[m-ou-se]

Fixes #98990

Marking this as draft, because it's not completely clear whether #98990 is a bug or intentional.

[rust-highfive]

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

[eddyb]

I'm not sure what happened here, but the lack of tests for this is worrying (and may explain how this happened, if it was accidental).

[eddyb]

Hmm is it possible that all calls take Some now and Option can be removed from around it?

[eddyb]

So I was able to backport this on top of 9aaf26e:

diff --git a/src/librustc/middle/stability.rs b/src/librustc/middle/stability.rs
index f5e18e13465..084b3e19c74 100644
--- a/src/librustc/middle/stability.rs
+++ b/src/librustc/middle/stability.rs
@@ -558,8 +558,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
                         let trait_item_def_id = self.tcx.associated_items(trait_did)
                             .find(|item| item.name == impl_item.name).map(|item| item.def_id);
                         if let Some(def_id) = trait_item_def_id {
-                            // Pass `DUMMY_NODE_ID` to skip deprecation warnings.
-                            self.tcx.check_stability(def_id, ast::DUMMY_NODE_ID, impl_item.span);
+                            self.tcx.check_stability(def_id, impl_item.id, impl_item.span);
                         }
                     }
                 }

And the result is more or less what I was expecting: that would've been a behavior change.
The most immediate outcome is that core stopped building:

(click to open core errors)

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:147:5
    |
147 |     fn nan() -> f32 {
    |     ^
    |
note: lint level defined here
   --> src/libcore/lib.rs:68:31
    |
68  | #![cfg_attr(not(stage0), deny(warnings))]
    |                               ^^^^^^^^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:152:5
    |
152 |     fn infinity() -> f32 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:157:5
    |
157 |     fn neg_infinity() -> f32 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:162:5
    |
162 |     fn zero() -> f32 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:167:5
    |
167 |     fn neg_zero() -> f32 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:172:5
    |
172 |     fn one() -> f32 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f32.rs:218:5
    |
218 |     fn integer_decode(self) -> (u64, i16, i8) {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:147:5
    |
147 |     fn nan() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:152:5
    |
152 |     fn infinity() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:157:5
    |
157 |     fn neg_infinity() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:162:5
    |
162 |     fn zero() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:167:5
    |
167 |     fn neg_zero() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:172:5
    |
172 |     fn one() -> f64 {
    |     ^

error: use of deprecated item: never really came to fruition and easily implementable outside the standard library
   --> src/libcore/num/f64.rs:218:5
    |
218 |     fn integer_decode(self) -> (u64, i16, i8) {
    |     ^

error: aborting due to 14 previous errors

---

So even if I did add the comment in that commit, not warning was the existing behavior.
I went to before the PR that commit was from, and I think I found the cause of the behavior:

rust/src/librustc/middle/stability.rs

Lines 525 to 535 in c7ddb89

	// For implementations of traits, check the stability of each item
	// individually as it's possible to have a stable trait with unstable
	// items.
	hir::ItemImpl(.., Some(ref t), _, ref impl_item_refs) => {
	let trait_did = tcx.expect_def(t.ref_id).def_id();
	for impl_item_ref in impl_item_refs {
	let impl_item = tcx.map.impl_item(impl_item_ref.id);
	let item = tcx.associated_items(trait_did)
	.find(|item| item.name == impl_item.name).unwrap();
	if warn_about_defns {
	maybe_do_stability_check(tcx, item.def_id, impl_item.span, cb);

Note how it's conditional on warn_about_defns - that's the only use of warn_about_defns in the entire function, it only exists to silence diagnostics from associated items.

That function has two calls:

• stability checks (with true passed to warn_about_defns):

  rust/src/librustc/middle/stability.rs

  Lines 460 to 461 in c7ddb89

  	check_item(self.tcx, item, true,
  	&mut |id, sp, stab, depr| self.check(id, sp, stab, depr));

• deprecation lint (with false passed to warn_about_defns):

  rust/src/librustc_lint/builtin.rs

  Lines 684 to 687 in c7ddb89

  	stability::check_item(cx.tcx,
  	item,
  	false,
  	&mut |id, sp, stab, depr| self.lint(cx, id, sp, &stab, &depr));

And going backwards past that, looks like it has remained unchanged in behavior since 0cf2d00 (which introduced stability checks for associated items, but didn't enable deprecation checks for them).

So my understanding is that there was an assumed asymmetry:

• stability always matters, whether it's using or defining/implementing something
• deprecation is use-oriented: calling a deprecated trait method warns, but including it in an implementation doesn't (since you're just providing it, to potential users)
  • if we still want to keep this logic we could gate it on whether there's a default (and maybe even give a better message) - after all, there's no way around specifying a non-defaulted method

[bors]

☔ The latest upstream changes (presumably #102051) made this pull request unmergeable. Please resolve the merge conflicts.