Implement #[deprecated_safe]

[ghost]

cc #94978

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

[ghost]

This doesn't need a reviewer yet, I meant to create this is in the draft state. I just wanted a PR to push to while I work.

[estebank]

@skippy10110 no problem, I'll ignore it until you ping me to let me know it is ready for review

[bors]

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

[jhpratt]

I'm assuming the skippy_log stuff is just for debugging purposes?

[ghost]

haha, most definitely :) you should have seen the horrible things i did to the compiler before this!

at one point i also had an atomic id slapped on ObligationCause, and then every creation of ObligationCause or Obligation logged to try and figure out how all this data was moving around... i think i had close to 200 logging points at one time, which made just compiling the compiler itself slow, hence skippy_log :)

[jhpratt]

Oh, I'm sure! I don't see anything that's clearly wrong in your code so far. Just the obvious todos and whatnot.

[ghost]

that's awesome to hear :) i was pretty much just winging it, but now i feel like i can actually justify how the code works and understand all the trait stuff (a little) better

[ghost]

i do have a question about reviews and my commit history

once i get things more wrapped up with a proper implementation of the attribute itself (e.g. a functioning "since" version, can only be used in the right places, etc.) and with lots of tests, proper error messages, etc.... should i rewrite the entire commit history to add each change piecemeal as if i knew what i was doing from the start?

this commit especially "THIS IS A GARBAGE COMMIT, just stashing due to data loss paranoia :)" is one i just kept force pushing over and over again as a snapshot of my whatever my current local state was

[jhpratt]

Ultimately that's up to the reviewer. Personally I'm a fan of cleaning up history, as it makes it clearer if anyone wants to come back later on to look at things.

[ghost]

cool, i'm a fan of tidy things so i'll make sure i do that up then

[ghost]

since you're following along, @jhpratt, i figured i'd ask your opinion on this (copied from the zulip stream):

i have a functioning since key for #[deprecated_safe] now, the semantics i chose are:

• outside of libstd since has no semantic meaning, just like #[deprecated] (applies immediately)
• within libstd, when called by 3rd party code, since will have meaning (doesn't apply until the since version)
• within libstd, when called by libstd itself, since will not have meaning (applies immediately)

so libstd can apply the attribute with e.g. a version of "TBD" without affecting 3rd party code, but it will need to update itself immediately

i felt like adding DEPRECATED_SAFE_IN_FUTURE for use within libstd was overkill, but i could do that if it's warranted

[jhpratt]

That would be in line with how #[deprecated] works to my understanding.

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit e9b4f751ac203500dafff3e3f22b2a54bb61f8e4 with merge 2456894a32f493d26aee0f03d9c17897c452d786...

[bors]

☀️ Try build successful - checks-actions
Build commit: 2456894a32f493d26aee0f03d9c17897c452d786 (2456894a32f493d26aee0f03d9c17897c452d786)

[rust-timer]

Queued 2456894a32f493d26aee0f03d9c17897c452d786 with parent 027a232, future comparison URL.

[rust-timer]

Finished benchmarking commit (2456894a32f493d26aee0f03d9c17897c452d786): comparison url.

Summary:

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	3	0	0	0	3
mean2	0.2%	N/A	N/A	N/A	0.2%
max	0.2%	N/A	N/A	N/A	0.2%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[jhpratt]

Nice work! Appears to be no difference overall.

[ghost]

thanks! there's a cheat 😊, the code does essentially zero work unless:
a) your code was about to fail with a compile error anyway, because #[deprecated_safe] wasn't applied
or b) #[deprecated_safe] was applied, but you haven't updated yet, and you're about to get a warning

[lcnr]

I've long wanted to look into adding a lint if certain predicates are used, mostly to deprecate the Eq and PartialEq impls for function pointers.

While the approach of linting when not in canonical queries works right now, my goal - and of @rust-lang/wg-traits afaik - is to slowly work towards always using queries for trait solving. At this point your code would break.

My expected solution here is to add "this should lint" to the output of a successful selection then let the caller emit the error. I didn't yet look into what's the best way to do this, but without too much thought it might make sense to just change the output of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_trait_selection/traits/struct.SelectionContext.html#method.confirm_candidate to have a lint_on_use: Option<SelectionLint> field. We would then have to propagate this upwards to outside of the query.

Probably makes sense for myself to look into this more, now that someone else also wants that. I will only get to that at some point after the 25th April though.

[ghost]

i was thinking i need a "attach lint for later reporting higher up" facility when selection is successful, since all the the nice error reporting seems to come from returning a SelectionError instead of a Selection, so that's perfect to hear that would be a good direction to go!

i wasn't sure if that would be too obtrusive a change, but it sounds like my hacky don't query change is going the opposite direction of what we want, based on what you're saying

i'll start taking a look at this and see if i can make any progress on it (zulip)

[ghost]

@lcnr, @estebank, i've made some progress on the obligation linting, which affects the "implement deprecated_safe for fn() pointer closure coercions" commit in this PR

assuming we go the route of putting a new linting system in place that this PR can then use, the commit i mentioned would change from:

• https://github.com/skippy10110/rust/commit/10c17928e8a01bcaf4e02303d09d4d23cbccdb6a

to something like:

• https://github.com/skippy10110/rust/commit/1a2ea4716ebae7c40ab58d8c738753cbc9c3a642

the rest of the commits in this PR are unaffected by this

[bors]

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

[JohnCSimon]

Ping from triage:
@skippy10110 - can you please address the conflicts and build failures and post your status?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[wesleywiser]

Visited during compiler team triage meeting. Looks like this is blocked on #96092. Marking as such for now. (If that is incorrect, please let me know and we can adjust the labels again)

[jhpratt]

As this PR's author has deleted their account, I have backed up the state of this pull request on my fork. It is located here. This should make it easier to use existing code if anyone would like to take it over.