Initial implementation of dyn* by eholk · Pull Request #101212 · rust-lang/rust

eholk · 2022-08-30T19:27:46Z

This PR adds extremely basic and incomplete support for dyn*. The goal is to get something in tree behind a flag to make collaboration easier, and also to make sure the implementation so far is not unreasonable. This PR does quite a few things:

Introduce dyn_star feature flag
Adds parsing for dyn* Trait types
Defines dyn* Trait as a sized type
Adds support for explicit casts, like 42usize as dyn* Debug
- Including const evaluation of such casts
Adds codegen for drop glue so things are cleaned up properly when a dyn* Trait object goes out of scope
Adds codegen for method calls, at least for methods that take &self

Quite a bit is still missing, but this gives us a starting point. Note that this is never intended to become stable surface syntax for Rust, but rather dyn* is planned to be used as an implementation detail for async functions in dyn traits.

Joint work with @nikomatsakis and @compiler-errors.

r? @bjorn3

rustbot · 2022-08-30T19:27:49Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

src/test/ui/async-await/dyn-star-trait-const.rs

compiler/rustc_typeck/src/check/cast.rs

rustbot · 2022-08-30T19:44:10Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors

left some initial comments, mostly nits! the code looks pretty good

compiler-errors · 2022-08-30T19:33:50Z

compiler/rustc_typeck/src/check/cast.rs

Can you explain why this is needed? Why can't this live in CastCheck, that is?

When a dyn* cast check succeeds, there aren't any deferred checks we need to worry about, so it doesn't really make sense to build a CastCheck.

I guess another way to do this would be to make CastCheck::new return Result<Option<CastCheck>, ErrorGuaranteed>, but this way seemed cleaner to me. I'm happy to change it though if you think that's better.

compiler/rustc_traits/src/chalk/lowering.rs

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs

compiler/rustc_middle/src/ty/mod.rs

compiler-errors · 2022-08-30T19:42:19Z

compiler/rustc_middle/src/ty/layout.rs

nit: Is it possible to put #[instrument] on the caller?

If you can, I haven't figured out how to do it...

compiler/rustc_middle/src/ty/layout.rs

compiler/rustc_const_eval/src/interpret/cast.rs

compiler/rustc_codegen_cranelift/src/value_and_place.rs

src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs

est31 · 2022-08-30T20:38:07Z

For context: https://smallcultfollowing.com/babysteps//blog/2022/03/29/dyn-can-we-make-dyn-sized/

bjorn3

You missed the pointer_kind function in rustc_typeck/src/check/cast.rs. I think it needs to return Some(PointerKind::Thin) or None for dyn*. It didn't error because .. was used. Please also check other usages of Dynamic. For example I think you didn't change dyn* anywhere to be sized.

bjorn3 · 2022-08-31T09:28:01Z

compiler/rustc_codegen_cranelift/src/base.rs

FIXME reminder

What's the best way to test with the cranelift backend? I can make sure this doesn't crash cranelift.

Is this something we should fix as part of this PR, or would it be okay to do it as a follow up?

compiler/rustc_const_eval/src/interpret/cast.rs

compiler/rustc_middle/src/ty/layout.rs

compiler/rustc_type_ir/src/sty.rs

compiler-errors · 2022-08-31T18:38:44Z

For example I think you didn't change dyn* anywhere to be sized.

dyn* is sized here: https://github.com/rust-lang/rust/pull/101212/files#diff-34d5893dd95fe09f9a0fd3341efacd1c21853bb34ba29d9d79bb9af26bb8a0a0R1874

eholk · 2022-09-02T17:05:55Z

@bjorn3

You missed the pointer_kind function in rustc_typeck/src/check/cast.rs. I think it needs to return Some(PointerKind::Thin) or None for dyn*.

I just pushed a change that makes pointer_kind error for dyn*, like the other sized types do. That sort of seemed reasonable based on the surrounding code and comments, but I'm not 100% confident in that choice. What do you think?

bjorn3 · 2022-09-02T18:39:22Z

I just pushed a change that makes pointer_kind error for dyn*, like the other sized types do. That sort of seemed reasonable based on the surrounding code and comments, but I'm not 100% confident in that choice. What do you think?

I think this is reasonable, but I'm not familiar with this code at all.

r? rust-lang/compiler

RalfJung · 2022-09-02T18:49:46Z

compiler/rustc_type_ir/src/sty.rs

Who decides the representation, where is that information (about the representation choice) encoded?

In the interpreter, it seems to always be a pointer to the real object (judging from the cast code, which is the only new code for this atm).

The layout code sets the representation here: https://github.com/rust-lang/rust/pull/101212/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9R627-R633

We construct these values here: https://github.com/rust-lang/rust/pull/101212/files#diff-db08b7c8b00530c7183cf2e42f25dc93b02da93fb40edadbd009eee6ad60e3a1R275-R289

The first link does not set how things are represented, via pointer or via direct inlining. I was wondering how that is decided.

However the first link does say that the data part must be non-null, which should also be documented here.

Not sure where the second link is supposed to go; links into the 'diff' view of a PR are notoriously unrelaible...

This should be more stable for the second link: https://github.com/rust-lang/rust/blob/242f9c128ebd5e107fc45ea3725e4e9745ff355b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs#L275...L289

At the moment the data part is made by basically transmuting whatever we are casting into it. The tests so far all work by casting a usize into a dyn*, for example: src/test/ui/dyn-star/method.rs.

Longer term we want to do this conversion using a family of lang traits called something like IntoDynPointer. I discuss this some in my blog post on async functions in traits. These would let types override the behavior. By default the implementation would probably be a straight cast for things that are already pointers and Box::new(self).into_raw() for things that are not, but we'd like to give flexibility in where async function return futures go.

The non-null requirement in the first link is incorrect, since it's totally reasonable to do 0 as dyn* Debug. I'll update that.

Oh I see, so something somewhere checks that the LHS of an as dyn* is ptr-sized?

It will in the future, but it doesn't now iirc...

compiler/rustc_type_ir/src/sty.rs

compiler/rustc_const_eval/src/interpret/cast.rs

cjgillot · 2022-09-03T15:06:54Z

compiler/rustc_type_ir/src/sty.rs

You can derive HashStable_Generic.

I tried and got:

error[E0405]: cannot find trait `HashStableContext` in the crate root --> compiler/rustc_type_ir/src/sty.rs:33:5 | 33 | HashStable_Generic | ^^^^^^^^^^^^^^^^^^ | | | not found in the crate root | in this derive macro expansion | ::: /home/ericholk/.cargo/registry/src/github.com-1ecc6299db9ec823/synstructure-0.12.6/src/macros.rs:94:9 | 94 | / pub fn $derives( 95 | | i: $crate::macros::TokenStream 96 | | ) -> $crate::macros::TokenStream { | |________________________________________- in this expansion of `#[derive(HashStable_Generic)]`

A lot of other code in this file implements HashStable also, so I'm guessing there's some more fundamental reason we can't use this.

You'll need to create an empty trait HashStableContext at the crate root, and implement it for ich::StableHashingContext in rustc_query_system. This is a bit convoluted, but I haven't found a better solution around the cyclic dependency issue.

Given that this crate already has a lot of manual implementations for HashTable, I think it makes more sense to do the same here and then do a separate PR that moves as many as we can over to derive all at once. What do you think?

I opened #101549 to do this for the other types. If that merges before this one (likely) then I'll update it here as well.

That PR merged so I've updated this to use HashStable_Generic here too.

compiler-errors · 2022-09-14T16:41:58Z

@bors r+ rollup=never

rollup=never is since this might have perf effects or we might want to blame it for ICEs

bors · 2022-09-14T16:42:00Z

📌 Commit 0faafbf has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2022-09-14T18:10:59Z

⌛ Testing commit 0faafbf with merge 6153d3c...

bors · 2022-09-14T20:52:12Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 6153d3c to master...

rust-timer · 2022-09-14T22:09:21Z

Finished benchmarking commit (6153d3c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.8%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	4.9%	[4.4%, 5.4%]	2
Regressions ❌ (secondary)	3.4%	[3.3%, 3.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	4.9%	[4.4%, 5.4%]	2

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

bjorn3 · 2022-09-15T16:14:13Z

Implemented for cg_clif in bjorn3/rustc_codegen_cranelift@879c86f.

flip1995 · 2022-09-22T14:44:15Z

src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs

+        Rvalue::Cast(CastKind::DynStar, _, _) => {
+            // FIXME(dyn-star)
+            unimplemented!()
+        },


@eholk This doesn't look right. This gives me the feeling that Clippy might start panicking on every project that tries to use this nightly feature.

Is there a plan to address those unimplemented! macros?

Specify `DynKind::Dyn` ref: rust-lang#101212 (comment)

Resolves 5542 Prior to rust-lang/rust#101212 the `ast::TraitObjectSyntax` enum only had two variants `Dyn` and `None`. The PR that introduced the `dyn*` syntax added a new variant `DynStar`, but did not update the formatting rules to account for the new variant. Now the new `DynStar` variant is properly handled and is no longer removed by rustfmt.

Specify `DynKind::Dyn` ref: rust-lang#101212 (comment)

Initial implementation of dyn* This PR adds extremely basic and incomplete support for [dyn*](https://smallcultfollowing.com/babysteps//blog/2022/03/29/dyn-can-we-make-dyn-sized/). The goal is to get something in tree behind a flag to make collaboration easier, and also to make sure the implementation so far is not unreasonable. This PR does quite a few things: * Introduce `dyn_star` feature flag * Adds parsing for `dyn* Trait` types * Defines `dyn* Trait` as a sized type * Adds support for explicit casts, like `42usize as dyn* Debug` * Including const evaluation of such casts * Adds codegen for drop glue so things are cleaned up properly when a `dyn* Trait` object goes out of scope * Adds codegen for method calls, at least for methods that take `&self` Quite a bit is still missing, but this gives us a starting point. Note that this is never intended to become stable surface syntax for Rust, but rather `dyn*` is planned to be used as an implementation detail for async functions in dyn traits. Joint work with `@nikomatsakis` and `@compiler-errors.` r? `@bjorn3`

RalfJung · 2023-02-06T14:39:25Z

compiler/rustc_middle/src/ty/layout.rs

            }

+            ty::Dynamic(_, _, ty::DynStar) => {
+                let mut data = scalar_unit(Int(dl.ptr_sized_integer(), false));


This sounds wrong -- if you are storing a pointer here, this should use a pointer layout, not an integer layout. Integers don't carry provenance, pointers do, so this makes a difference.

I am seeing some strange behavior in trying to get this to work in Miri and it looks like provenance is getting lost... so I am guessing this might be the reason. I see in an earlier commit this was using Pointer. Why did you change this to ptr_sized_integer?

Note that if something is either a pointer or an integer, then it must use a pointer type, not an integer type. Ptr-sized integers are a strict subset of pointers.

I'm trying to fix this as part of #107728 but that leads to invalid LLVM IR errors...

RalfJung · 2023-02-06T14:45:01Z

compiler/rustc_middle/src/ty/layout.rs

+                ty::Dynamic(_, _, ty::DynStar) => {
+                    if i == 0 {
+                        TyMaybeWithLayout::Ty(tcx.types.usize)
+                    } else if i == 1 {
+                        // FIXME(dyn-star) same FIXME as above applies here too
+                        TyMaybeWithLayout::Ty(
+                            tcx.mk_imm_ref(
+                                tcx.lifetimes.re_static,
+                                tcx.mk_array(tcx.types.usize, 3),
+                            ),
+                        )


Is there some kind of implicit invariant that this must be consistent with layout_of_uncached? Nowadays these two functions are not even in the same crate. That seems very fragile. Am I missing something?

Cc @oli-obk

Resolves 5542 Prior to rust-lang/rust#101212 the `ast::TraitObjectSyntax` enum only had two variants `Dyn` and `None`. The PR that introduced the `dyn*` syntax added a new variant `DynStar`, but did not update the formatting rules to account for the new variant. Now the new `DynStar` variant is properly handled and is no longer removed by rustfmt.

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 30, 2022

rust-highfive assigned bjorn3 Aug 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2022

compiler-errors reviewed Aug 30, 2022

View reviewed changes

src/test/ui/async-await/dyn-star-trait-const.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Aug 30, 2022

View reviewed changes

compiler/rustc_typeck/src/check/cast.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

compiler-errors reviewed Aug 30, 2022

View reviewed changes

eholk force-pushed the dyn-star branch from 31d8ee3 to d2ac08e Compare August 30, 2022 19:45

compiler-errors reviewed Aug 30, 2022

View reviewed changes

compiler/rustc_codegen_cranelift/src/value_and_place.rs Outdated Show resolved Hide resolved

src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bjorn3 requested changes Aug 31, 2022

View reviewed changes

RalfJung reviewed Aug 31, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/cast.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 31, 2022

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 31, 2022

View reviewed changes

compiler/rustc_type_ir/src/sty.rs Outdated Show resolved Hide resolved

eholk force-pushed the dyn-star branch from 4481585 to 85dbcf8 Compare August 31, 2022 18:46

This comment has been minimized.

Sign in to view

eholk force-pushed the dyn-star branch from f4b0c77 to 14e9d67 Compare September 2, 2022 17:59

rust-highfive assigned cjgillot and unassigned bjorn3 Sep 2, 2022

RalfJung reviewed Sep 2, 2022

View reviewed changes

compiler/rustc_type_ir/src/sty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Sep 3, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/cast.rs Outdated Show resolved Hide resolved

cjgillot reviewed Sep 3, 2022

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2022

bors merged commit 6153d3c into rust-lang:master Sep 14, 2022

rustbot added this to the 1.65.0 milestone Sep 14, 2022

bors mentioned this pull request Sep 14, 2022

Add TyAlias into rustc_type_ir TyKind enum #97974

Closed

crlf0710 added the F-dyn_star `#![feature(dyn_star)]` label Sep 17, 2022

mejrs mentioned this pull request Sep 21, 2022

rustfmt eats dyn_star syntax rust-lang/rustfmt#5542

Closed

flip1995 reviewed Sep 22, 2022

View reviewed changes

TaKO8Ki mentioned this pull request Sep 30, 2022

Specify DynKind::Dyn #102506

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022

Rollup merge of rust-lang#102506 - TaKO8Ki:specify-dyn-kind, r=lcnr

de9a8cf

Specify `DynKind::Dyn` ref: rust-lang#101212 (comment)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022

Rollup merge of rust-lang#102506 - TaKO8Ki:specify-dyn-kind, r=lcnr

c7fe64a

Specify `DynKind::Dyn` ref: rust-lang#101212 (comment)

ytmimi mentioned this pull request Sep 30, 2022

Handle dyn* syntax when rewriting ast::TyKind::TraitObject rust-lang/rustfmt#5552

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022

Rollup merge of rust-lang#102506 - TaKO8Ki:specify-dyn-kind, r=lcnr

a877758

Specify `DynKind::Dyn` ref: rust-lang#101212 (comment)

This was referenced Oct 14, 2022

Kani does not yet support dyn* model-checking/kani#1784

Closed

Update Kani to Rust nightly-2022-10-11 model-checking/kani#1788

Merged

RalfJung reviewed Feb 6, 2023

View reviewed changes

RalfJung mentioned this pull request Feb 6, 2023

Tracking issue for dyn-star #102425

Closed

10 tasks

Uh oh!

Conversation

eholk commented Aug 30, 2022 • edited by RalfJung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 30, 2022

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Aug 30, 2022

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

est31 commented Aug 30, 2022

Uh oh!

bjorn3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

compiler-errors commented Aug 31, 2022

Uh oh!

This comment has been minimized.

eholk commented Sep 2, 2022

Uh oh!

bjorn3 commented Sep 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Sep 14, 2022

eholk commented Aug 30, 2022 •

edited by RalfJung

Loading