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

Constify TypeId ordering impls #101698

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Constify TypeId ordering impls #101698

merged 1 commit into from
Jan 17, 2023

Conversation

raldone01
Copy link
Contributor

@raldone01 raldone01 commented Sep 11, 2022

Tracking issue: #101871

Adding const ordering to TypeId allows rtti crates to optimize some casting scenarios (without transmuting to u64). This would also prevent these crates from breaking if the underlying type is changed from u64 to something different.

Feature gate: #![feature(const_cmp_type_id)]

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 11, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

note: impl defined here, but it is not `const`
--> $SRC_DIR/core/src/any.rs:LL:COL
|
LL | impl const PartialEq for TypeId {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a bit weird? “impl defined here but it is not const” but it is const as shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that is because this test doesn't use the const_trait_impl feature flag.
That always leads to this error if used this way.

The same error happens in src/test/ui/consts/const_cmp_type_id.rs if #![feature(const_trait_impl)] is removed, this is probably more of an issue in const_trait_impl than here.

@raldone01 raldone01 changed the title Consitfy TypeId ordering impls Constify TypeId ordering impls Sep 11, 2022
@onestacked
Copy link
Contributor

Makes public stable implementations of Ordering traits unstably const, so

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 14, 2022
Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@scottmcm scottmcm added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. I-lang-nominated Nominated for discussion during a lang team meeting. labels Sep 24, 2022
@scottmcm
Copy link
Member

Nominating for team discussions. The ordering of TypeIds becoming const-visible in particular feels potentially scary to me.

@raldone01
Copy link
Contributor Author

raldone01 commented Sep 24, 2022

Some additional background that may help in the discussion:

I have created a crate called trait_cast_rs. It provides a custom Any type. That allows casts to dyn Traits.

Internally it has a const slice of function pointers with their associated TypeIds. Making TypeId const comparable would allow the crate to sort the slice at compile time. When downcasting at runtime a binary_search is performed to find the correct function pointer.

This has the advantage that the array must not be sorted on every program launch and the lookup slice can be a constant.

I currently use transmute<_, u64>(type_id) to sort the slice at compile time.

#[stable(feature = "rust1", since = "1.0.0")]
pub struct TypeId {
t: u64,
}

#[stable(feature = "rust1", since = "1.0.0")]
impl StructuralPartialEq for TypeId {}
Copy link
Member

Choose a reason for hiding this comment

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

Per T-libs (see #102371 (comment)) we're going to wait on derive_const (or similar) before accepting more changes like this to library.

@rustbot label +S-Blocked

Copy link
Member

Choose a reason for hiding this comment

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

Please do not make TypeId "structural match" - instead, that needs to be removed - as per e.g. #77125 (comment) - sadly the one PR trying to do it was rejected and I'm not seeing a replacement just removing "structural match":

Copy link
Contributor

@onestacked onestacked Oct 13, 2022

Choose a reason for hiding this comment

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

the impl StructualPartialEq was added because the test for issue-73976 failed as it used matches!.
It could be changed to a normal == though, but removing the structural match would be a breaking change (nightly only since const_type_id isn't stable yet).

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added a commit that does those changes.

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 28, 2022
@scottmcm
Copy link
Member

We discussed this in the lang meeting this week. It didn't result in anything that needs to block this for nightly, but I've added a note to the tracking issue that this precludes some possible implementations, and thus stabilizing it would need to be considered very carefully.

@scottmcm scottmcm removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 28, 2022
@m-ou-se m-ou-se added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Oct 4, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Oct 4, 2022

Nominating this for the compiler team for input, since the compiler is the one generating the type ids. So question for the compiler team: will this get in the way of any future plans there might be with future typeid implementations? (And if so, is that a problem?)

@apiraino
Copy link
Contributor

apiraino commented Oct 6, 2022

Issue was discussed in T-compiler meeting, I'm leaving here some notes; more will probably follow-up in this issue to reach a wider audience.

Leaving I-compiler-nominated for the moment.

@apiraino
Copy link
Contributor

@scottmcm during T-compiler meeting (see previous comment), some questions arised (here, for example). Can you provide a bit of feedback to complement the discussion there? thanks

@rustbot label -i-compiler-nominated

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Oct 13, 2022
@eddyb
Copy link
Member

eddyb commented Oct 13, 2022

Adding const ordering to TypeId allows rtti crates to optimize some casting scenarios (without transmuting to u64).

If you want the hash, you should have never transmuted, but instead written a Hasher that is most efficient for write_u64 (ideally it should accept any data then truncate and/or mix but we found out the hard way that we can't really break people requiring write_u64 because it's kind of common in code that doesn't misuse transmute).

You can do this on stable Rust, and it should work all the way back to 1.0 AFAIK, so it's a good option to consider.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 16, 2023
@onestacked
Copy link
Contributor

Why did the CI not run?

@scottmcm
Copy link
Member

Hmm, I'll try to re-kick it.

@bors r-

@scottmcm scottmcm closed this Jan 16, 2023
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 16, 2023
@scottmcm scottmcm reopened this Jan 16, 2023
@WaffleLapkin
Copy link
Member

@bors r=scottmcm
github is/was having some issues with github actions

@bors
Copy link
Contributor

bors commented Jan 16, 2023

📌 Commit 73f8dc788b0e63d3ed4fd88fbbfd9ffa70c8acb9 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2023
@onestacked
Copy link
Contributor

Should probably r- untill CI passes

@scottmcm
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 16, 2023
@onestacked
Copy link
Contributor

@scottmcm can you r+?

@WaffleLapkin
Copy link
Member

@bors r=scottmcm

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit 7355ab3 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#101698 (Constify `TypeId` ordering impls)
 - rust-lang#106148 (Fix unused_parens issue for higher ranked function pointers)
 - rust-lang#106922 (Avoid unsafe code in `to_ascii_[lower/upper]case()`)
 - rust-lang#106951 (Remove ineffective run of SimplifyConstCondition)
 - rust-lang#106962 (Fix use suggestion span)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 09faa26 into rust-lang:master Jan 17, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 17, 2023
@onestacked onestacked deleted the feat/const_cmp_typeid branch January 18, 2023 09:06
yvt added a commit to r3-os/r3 that referenced this pull request Mar 18, 2023
`core::any::TypeId` implements `const PartialEq` as of
[rust-lang/rust#101698][1].

[1]: rust-lang/rust#101698
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 26, 2023
…atch, r=dtolnay

Remove structural match from `TypeId`

As per rust-lang#99189 (comment).

> Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

rust-lang#99189 (comment)

> Landing this PR now (well, mainly the "remove structural equality" part) would unblock `const fn` `TypeId::of`, since we only postponed that because we were guaranteeing too much.

See also rust-lang#99189, rust-lang#101698
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2023
…ch, r=dtolnay

Remove structural match from `TypeId`

As per rust-lang#99189 (comment).

> Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

rust-lang#99189 (comment)

> Landing this PR now (well, mainly the "remove structural equality" part) would unblock `const fn` `TypeId::of`, since we only postponed that because we were guaranteeing too much.

See also rust-lang#99189, rust-lang#101698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.