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

rustc_skip_during_method_dispatch: decouple receiver & edition #129871

Conversation

GrigorenkoPV
Copy link
Contributor

  • Removes the implicit assumption that arrays are gated behind edition 2021, and boxed slices behind 2024.
  • Addresses // FIXME: We could probably do way better attribute validation here.
  • Abstracts array and boxed_slice away to a GatedReceiver enum, offering a more convenient extension point for when we run into the "add this impl now, but gate the method call syntax behind an edition" situation again.
  • Instead of having a bool field for each possible receiver store a vec1 of all Receiver + Edition pairs.
  • Stable MIR now has a #[non_exhaustive] enum for Editions.

Would be cool if this attribute could be applied inside of an impl instead of to the entire trait declaration. This would eliminate the need for the receiver in the first place. But that's either something that had already been tried and failed, or a story for another day.

Footnotes

  1. An alternative would be to use a HashMap from receivers to editions, but 99.999% of the time it would be empty (currently IntoIterator is the only trait in existence to have this attribute) and the remaining 0.001% it has like two elements, so any asymptotic improvements would probably be crushed by the constant multiplier. Also ThinVec is just 1 pointer wide, while FxHashMap is 4, IIRC. A bit wasteful.

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 2, 2024
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the rustc_skip_during_method_dispatch branch from 51788be to 66d44ce Compare September 2, 2024 00:45
@GrigorenkoPV GrigorenkoPV marked this pull request as ready for review September 2, 2024 00:45
@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks for putting in the effort for generalizing this attribute, but given its inherently hacky nature, I personally don't believe it warrants generalization at the moment. I think the right point at which to decide is when we add another hack implementation in edition 2027, since there's no guarantee that this change is actually well suited for whatever receiver/trait pair we'd need to apply this to next, and the current design is suitable for the two existing implementations at moment.

@GrigorenkoPV
Copy link
Contributor Author

Sounds reasonable.

I will look into whether it is possible to move this from trait declarations to implementations, as this might reduce the "hackiness". But it is probably very not trivial, because otherwise it would've likely already been done. But until then,

@GrigorenkoPV GrigorenkoPV marked this pull request as draft September 2, 2024 18:10
@bors
Copy link
Contributor

bors commented Sep 6, 2024

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

@pnkfelix
Copy link
Member

pnkfelix commented Oct 4, 2024

since this is now in draft status, i'll move it back to waiting-on-author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2024
@Dylan-DPC
Copy link
Member

@GrigorenkoPV any updates of this?

@GrigorenkoPV
Copy link
Contributor Author

GrigorenkoPV commented Nov 17, 2024

@GrigorenkoPV any updates of this?

I initially created this as a step forward to #129872, which, similarly to IntoIter for [T; N] would have to gate method calls behind an edition, in this case, the 2027 edition (or whatever it will be called), which is different from the currently hardcoded 2021 edition for arrays.

However, #129872 should probably go through an ACP first, which I am yet to write.

(Then the ACP itself will probably also take ages to get accepted/declined.)

@Dylan-DPC
Copy link
Member

@GrigorenkoPV thanks. I'm going to close this then since this is blocked on a pr that requires an ACP to be written and merged and that's going to take a while which could cause this to bitrot.

@Dylan-DPC Dylan-DPC closed this Nov 18, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants