Stream: t-compiler/help

Topic: automatically adding a trait impl


Bastian Kauschke (Apr 01 2020 at 20:43, on Zulip):

Currently working on https://github.com/rust-lang/rust/issues/70509.

I have added a new lang_item and updated libcore.
I now want to add a correct impl of DiscriminantKind to every type. I am stuck here. I tried looking how sized bounds are handled which I still don't fully understand.

What would the best way to automatically add an impl during compilation?

eddyb (Apr 01 2020 at 20:45, on Zulip):

uhhhh

eddyb (Apr 01 2020 at 20:45, on Zulip):

@Bastian Kauschke look at how fn types implementFnOnce automatically

eddyb (Apr 01 2020 at 20:46, on Zulip):

I believe that's in the compiler

eddyb (Apr 01 2020 at 20:46, on Zulip):

ty::FnPtr in select.rs and project.rs I guess

Bastian Kauschke (Apr 01 2020 at 20:46, on Zulip):

/me will look into it. Thanks

eddyb (Apr 01 2020 at 20:46, on Zulip):

project.rs will likely mention a Vtable:: variant that select.rs creates, if I had to guess

eddyb (Apr 01 2020 at 20:47, on Zulip):

(Vtable is a really misleading name for "user or built-in impl")

Bastian Kauschke (Apr 01 2020 at 21:53, on Zulip):

Will be harder as expected as there is no other builtin impl with associated types. Will look into this further tomorrow

eddyb (Apr 01 2020 at 21:56, on Zulip):

@Bastian Kauschke this would suggest there is https://github.com/rust-lang/rust/blob/master/src/librustc_trait_selection/traits/project.rs#L1289-L1293

eddyb (Apr 01 2020 at 21:56, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_trait_selection/traits/project.rs#L1354-L1358

eddyb (Apr 01 2020 at 21:57, on Zulip):

this is for the associated type "Output"

eddyb (Apr 01 2020 at 21:57, on Zulip):

@ecstatic-morse btw we still have ty::ProjectionTy::from_ref_and_name which finds associated types by name, instead of making them lang items (or just taking the first associated type inside the trait)

ecstatic-morse (Apr 01 2020 at 23:25, on Zulip):

Ah yeah. That slipped my mind. I'll open an issue so I don't forget

Bastian Kauschke (Apr 02 2020 at 20:29, on Zulip):

It finally compiles :party_ball: fk yes

Bastian Kauschke (Apr 03 2020 at 10:17, on Zulip):

I am once again stuck. I now correctly generate an impl on the fly in case the type is specific enough.
My behavior for unresolved types is still wrong though. When using e.g. <T as DiscriminantKind>::Discriminant
I don't know the type of Discriminant yet.

I tried 2 approaches: always adding a candidate during selection and returning the projection <T as DiscriminantKind>::Discriminant in project.rs if the assoc type is not yet known. This cases a cycle.

check during selections if the type can already be known and only add a candidate in this case, this is similar to what is done for Sized.
Which cases the error

error[E0277]: the trait bound `T: marker::DiscriminantKind` is not satisfied
   --> src/libcore/mem/mod.rs:935:28
    |
935 | pub struct Discriminant<T>(<T as DiscriminantKind>::Discriminant);
    |

How can I express that a trait is implemented for T without actually caring about any specific impl

eddyb (Apr 03 2020 at 10:21, on Zulip):

fascinating conondrum

eddyb (Apr 03 2020 at 10:21, on Zulip):

you might need to rely on specialization

eddyb (Apr 03 2020 at 10:22, on Zulip):
impl<T> DiscriminantKind for T {
    default type Discriminant = ();
}
eddyb (Apr 03 2020 at 10:23, on Zulip):

and only use the builtin one for enums and generators

eddyb (Apr 03 2020 at 10:24, on Zulip):

cc @nikomatsakis I'm not sure using specialization like this is the best thing to do

Bastian Kauschke (Apr 05 2020 at 09:42, on Zulip):

Big brain move to both use specialization and still forbid impls of the new lang item. :laughing:

trait DiscriminantKind {
    type Discriminant;
}

impl<T> DiscriminantKind for T {
     default type Discriminant = !;
}

impl<T: ActualDiscriminantKind> DiscriminantKind for T {
    type Discriminant = T::ActualDiscriminant;
}

#[lang = "actual_discriminant_kind"]
trait ActualDiscriminantKind {
    type ActualDiscriminant;
}
eddyb (Apr 05 2020 at 09:53, on Zulip):

but you don't need to expose the trait at all

eddyb (Apr 05 2020 at 09:53, on Zulip):

it can be both a lang item and fully private

eddyb (Apr 05 2020 at 09:53, on Zulip):

(this is how Freeze works IIRC)

eddyb (Apr 05 2020 at 09:54, on Zulip):

I wonder if using specialization default + builtin implementation will break

eddyb (Apr 05 2020 at 09:54, on Zulip):

because the builtin thing is not in the specialization graph :P

eddyb (Apr 05 2020 at 09:55, on Zulip):

probably not, the compiler will just handle it separately

Bastian Kauschke (Apr 05 2020 at 09:55, on Zulip):

probably not, the compiler will just handle it separately

I hope so :laughing:
I guess it should work. As this is the same as specializing over Sized

eddyb (Apr 05 2020 at 09:55, on Zulip):

the specialization graph is for moving towards the trait (in the "less specialized" direction), not the other way around, so I think we're fine

eddyb (Apr 05 2020 at 09:56, on Zulip):

anyway there's a reason I want @nikomatsakis to review your PR :P

Bastian Kauschke (Apr 05 2020 at 11:44, on Zulip):

Do we have const traits on nightly yet?

I shot myself in the knee by making mem::discriminant unstabily const... Or I just remove the const attribute for now :thinking:

eddyb (Apr 05 2020 at 11:46, on Zulip):

@Bastian Kauschke why? the intrinsic should still be const

eddyb (Apr 05 2020 at 11:47, on Zulip):

always use the intrinsic, the trait is there just to get the right type for the intrinsic

Bastian Kauschke (Apr 05 2020 at 11:51, on Zulip):

I use the setup from above.

While the compiler knows that DiscriminantKind is implemented for all T. It does not know this for ActualDiscriminantType.
As I have to check that intrinsics::discriminant_value has the correct signature, the used trait must be a lang item.
I don't want to add 2 new lang items though. To solve this I restricted the intrinsic to ActualDiscriminantType and added a discriminant_value method to DiscriminantKind which calls intrinsics::discriminant_value in the specialized impl and is irrelevant otherwise.

eddyb (Apr 05 2020 at 11:51, on Zulip):

you should only use one trait

eddyb (Apr 05 2020 at 11:52, on Zulip):

the two-trait system is funny but it should really not be needed

eddyb (Apr 05 2020 at 11:52, on Zulip):

the intrinsic can already handle all the situations anyway

eddyb (Apr 05 2020 at 11:52, on Zulip):

it's just the trait that you need to make work

eddyb (Apr 05 2020 at 11:52, on Zulip):

and like I said, you can have it be a lang item and completely private

eddyb (Apr 05 2020 at 11:53, on Zulip):

you can even import the intrinsic in a module other than core::intrinsics

eddyb (Apr 05 2020 at 11:53, on Zulip):

you can even import the intrinsic inside mem::discriminant :P (and this is what some other parts of libcore do)

Bastian Kauschke (Apr 05 2020 at 11:57, on Zulip):

the two-trait system is funny but it should really not be needed

But this is such a ****ing beauty. I will never again have the chance to make a mess like this :cry:

By only having 1 trait I can't check that there aren't any unsound impls. Which is unfortunate, but probably ok as long as DiscriminantKind is private. Will use a one-trait approach for now :unicorn:

eddyb (Apr 05 2020 at 12:50, on Zulip):

they don't have to be unsound, just assert in rustc_codegen_llvm or wherever discriminant_value is implemented, that the type matches

eddyb (Apr 05 2020 at 12:50, on Zulip):

then not even libcore could add a bad impl

Bastian Kauschke (Apr 05 2020 at 15:41, on Zulip):

eyyy, everything but codegen works now afaict :muscle:

Bastian Kauschke (Apr 05 2020 at 17:13, on Zulip):

is there a way to only check stage 2 using stage 1?

eddyb (Apr 05 2020 at 17:36, on Zulip):

you mean ./x.py check but using the compiler you built?

eddyb (Apr 05 2020 at 17:36, on Zulip):

cc @simulacrum

simulacrum (Apr 05 2020 at 17:38, on Zulip):

Not easily, check is hard coded for stage 0

Bastian Kauschke (Apr 05 2020 at 18:40, on Zulip):

seems like I just have to get the change right on the first try then :laughing:

Bastian Kauschke (Apr 05 2020 at 18:54, on Zulip):

meh

error[E0599]: no method named `unwrap` found for enum `std::result::Result<usize, <usize as std::convert::TryFrom<<<T as ty::codec::EncodableWithShorthand>::Variant as std::marker::DiscriminantKind>::Discriminant>>::Error>` in the current scope
  --> src/librustc_middle/ty/codec.rs:82:43
   |
82 |     assert!(usize::try_from(discriminant).unwrap() < SHORTHAND_OFFSET);
   |                                           ^^^^^^ method not found in `std::result::Result<usize, <usize as std::convert::TryFrom<<<T as ty::codec::EncodableWithShorthand>::Variant as std::marker::DiscriminantKind>::Discriminant>>::Error>`
   |
   = note: the method `unwrap` exists but the following trait bounds were not satisfied:
           `<usize as std::convert::TryFrom<<<T as ty::codec::EncodableWithShorthand>::Variant as std::marker::DiscriminantKind>::Discriminant>>::Error: std::fmt::Debug`

error: aborting due to previous error
eddyb (Apr 05 2020 at 19:42, on Zulip):

haha

eddyb (Apr 05 2020 at 19:42, on Zulip):

that method was abusing discriminant_value

eddyb (Apr 05 2020 at 19:43, on Zulip):

@Bastian Kauschke you should add a bound to that function, e.g. T: Discriminant<DiscriminantKind = isize> (if the trait is public), or just define a local copy of the intrinsic that just returns isize

eddyb (Apr 05 2020 at 19:43, on Zulip):

(since that's the default discriminant type)

Bastian Kauschke (Apr 05 2020 at 19:47, on Zulip):

I fixed this without restricting DiscriminantKind for now. https://github.com/rust-lang/rust/pull/70705/files#diff-8fbc542a08b9274446b2fbf02bd610b2R80-R87

I am currently considering if changing the discriminant to i8/u8 where possible is worth the effort. Don't know if there is any case were the size of mem::Discriminant actually matters though :laughing:

eddyb (Apr 05 2020 at 19:49, on Zulip):

no, please leave enums unnanotated

eddyb (Apr 05 2020 at 19:50, on Zulip):

#[repr(u8)] disables all optimizations

eddyb (Apr 05 2020 at 19:50, on Zulip):

and the actual tag in memory is byte-sized for most enums anyway

Bastian Kauschke (Apr 05 2020 at 19:51, on Zulip):

#[repr(u8)] disables all layout optimizations

no no no

I mean inside of the compiler. I don't want to tag all enums. I want to change DiscriminantKind::Discriminant for enum Meh { A, B } to u8 automatically

eddyb (Apr 05 2020 at 19:51, on Zulip):

I left a comment

eddyb (Apr 05 2020 at 19:52, on Zulip):

@Bastian Kauschke ah that's unworkable

Bastian Kauschke (Apr 05 2020 at 19:52, on Zulip):

?

eddyb (Apr 05 2020 at 19:52, on Zulip):

the discriminant type needs to be the same as specified on the enum (or the default, isize)

eddyb (Apr 05 2020 at 19:52, on Zulip):

otherwise, some things may stop working as expected

eddyb (Apr 05 2020 at 19:53, on Zulip):

that's the type that explicit discriminant values are type-checked against

eddyb (Apr 05 2020 at 19:53, on Zulip):

it's not the same as the tag the compiler might encode that discriminant as

Bastian Kauschke (Apr 05 2020 at 20:00, on Zulip):

Still think that we could use some kind of special case for simple cases :laughing:
It probably just isn't worth it

eddyb (Apr 05 2020 at 20:35, on Zulip):

it's better to be consistent IMO

Bastian Kauschke (Apr 05 2020 at 20:38, on Zulip):

DiscriminantKind should never be used directly though. I am somewhat in favor of just replacing the current usage in the compiler with a manually implemented function.

Bastian Kauschke (Apr 05 2020 at 20:39, on Zulip):

An unobservable inconsistency is fairly irrelevant tbh :laughing:

eddyb (Apr 05 2020 at 20:41, on Zulip):

you mean with a separate import of the intrinsic?

Bastian Kauschke (Apr 05 2020 at 20:45, on Zulip):

you mean with a separate import of the intrinsic?

While this might actually work, imo this is even worse.

My approach would be the following even if it is not 100 % dry.

impl TyKind {
     fn variant_id(&self) -> u64 {
        match self {
            TyKind::Bool => 0,
            TyKind::Int(_) => 1,
           // ...
        }
    }
}
eddyb (Apr 05 2020 at 20:45, on Zulip):

ah no please don't do that

eddyb (Apr 05 2020 at 20:45, on Zulip):

the reason we use discriminant_value is because that's what libserialize encodes (or, well, it's less likely to get out of sync)

eddyb (Apr 05 2020 at 20:46, on Zulip):

I guess we could abuse serialization and get it that way heh

Bastian Kauschke (Apr 05 2020 at 20:46, on Zulip):

eddyb said:

the reason we use discriminant_value is because that's what libserialize encodes (or, well, it's less likely to get out of sync)

that makes the current state actually worse IMO :laughing:

eddyb (Apr 05 2020 at 20:47, on Zulip):

we're basically checking that we can make up variant indices that would not be valid discriminants of the enum

eddyb (Apr 05 2020 at 20:47, on Zulip):

so please try to avoid changing it as much as possible

eddyb (Apr 05 2020 at 20:47, on Zulip):

any change there would require far more intensive perf-testing

eddyb (Apr 05 2020 at 20:48, on Zulip):

and care

Bastian Kauschke (Apr 05 2020 at 21:17, on Zulip):

(deleted)

Bastian Kauschke (Apr 12 2020 at 17:42, on Zulip):

@eddyb do you have an opinion on https://github.com/rust-lang/rust/pull/70705#discussion_r403752647 ?

Bastian Kauschke (Apr 12 2020 at 17:47, on Zulip):

The PR still slightly improves perf afaict, even while using TryFrom there. (mostly because Eq for enums now uses i32instead of u64)

eddyb (Apr 12 2020 at 18:29, on Zulip):

@Bastian Kauschke why do you say i32?

eddyb (Apr 12 2020 at 18:29, on Zulip):

enum discriminants default to isize by default which is i64 on the x64 linux that perf runs on

eddyb (Apr 12 2020 at 18:30, on Zulip):

the only thing that uses i32 by default is generators and maybe #[repr(C)] enums?

Bastian Kauschke (Apr 12 2020 at 18:30, on Zulip):

jup, was wrong. Then I don't know why there are minimal perf improvements

eddyb (Apr 12 2020 at 18:30, on Zulip):

there aren't :)

eddyb (Apr 12 2020 at 18:30, on Zulip):

that's just noise

Bastian Kauschke (Apr 12 2020 at 18:32, on Zulip):

I hate computers

eddyb (Apr 12 2020 at 18:37, on Zulip):

I really want us to fix perf to have even less noise

Bastian Kauschke (Apr 12 2020 at 21:07, on Zulip):
error[E0277]: `<<<T as ty::codec::EncodableWithShorthand>::Variant as std::marker::DiscriminantKind>::Discriminant as std::convert::TryFrom<usize>>::Error` doesn't implement `std::fmt::Debug`
  --> src/librustc_middle/ty/codec.rs:81:56
   |
65 |     <T::Variant as DiscriminantKind>::Discriminant: Ord + TryFrom<usize>,
   |                                                                          - help: consider further restricting the associated type: `, <<<T as ty::codec::EncodableWithShorthand>::Variant as std::marker::DiscriminantKind>::Discriminant as std::convert::TryFrom<usize>>::Error: std::fmt::Debug`
eddyb (Apr 12 2020 at 21:12, on Zulip):

lol

eddyb (Apr 12 2020 at 21:13, on Zulip):

@Bastian Kauschke replace .unwrap() with .ok().unwrap()

Bastian Kauschke (Apr 12 2020 at 21:13, on Zulip):

replaced it with unwrap_or_else(|_| unreachable()). Your version is better though :laughing:

Bastian Kauschke (Apr 13 2020 at 12:52, on Zulip):

@eddyb https://github.com/rust-lang/rust/pull/70705#issuecomment-612884955 implemented some of your feedback.

eddyb (Apr 13 2020 at 13:42, on Zulip):

btw it doesn't ping me if you don't press TAB

eddyb (Apr 13 2020 at 13:42, on Zulip):

i.e. it has to look like @**eddyb** before you send it

Bastian Kauschke (Apr 13 2020 at 15:27, on Zulip):

you're going to read it anyways :laughter_tears:

eddyb (Apr 13 2020 at 15:36, on Zulip):

yeah it's just a difference of timing

Bastian Kauschke (Apr 13 2020 at 17:05, on Zulip):

@eddyb Thank you for https://github.com/rust-lang/rust/pull/70705#discussion_r407504945 :heart:

eddyb (Apr 13 2020 at 17:17, on Zulip):

omg did it work :D?

Bastian Kauschke (Apr 13 2020 at 17:19, on Zulip):

jup

eddyb (Apr 13 2020 at 17:20, on Zulip):

phew, glad to not need specialization

eddyb (Apr 13 2020 at 17:22, on Zulip):

btw you have an accidental submodule addition

Bastian Kauschke (Apr 13 2020 at 17:24, on Zulip):

:dolphin: do you know how to cleanly remove this? The last time I tried I just ended up nuking my local rust repo

eddyb (Apr 13 2020 at 17:27, on Zulip):

@Bastian Kauschke generally I use selective addition, either via git gui or IDE integration (I don't use commands like git add), so I just never add a submodule change to a commit

eddyb (Apr 13 2020 at 17:27, on Zulip):

now, this is readding a whole submodule, not just committing a change to it

eddyb (Apr 13 2020 at 17:27, on Zulip):

@Bastian Kauschke the first step is to find which commit it's in, and then you can take it out just like any unwanted file change

eddyb (Apr 13 2020 at 17:28, on Zulip):

the important thing is to use git gui to steal it from a commit, that's the only intuitive way I can think of

eddyb (Apr 13 2020 at 17:29, on Zulip):

okay so it's in this commit https://github.com/rust-lang/rust/pull/70705/commits/22de130b956498a9d8dae9e237d812b116044065

Bastian Kauschke (Apr 13 2020 at 17:30, on Zulip):

It's fixed :laughing:

eddyb (Apr 13 2020 at 17:30, on Zulip):

did you use a rebase, or something else ?

Bastian Kauschke (Apr 13 2020 at 17:30, on Zulip):

apparently git rm path/to/file just works

Bastian Kauschke (Apr 13 2020 at 17:30, on Zulip):

and then an ordinary rebase

eddyb (Apr 13 2020 at 17:31, on Zulip):

well, it's not in the commit which had it, so its' fine now

Bastian Kauschke (Apr 13 2020 at 17:33, on Zulip):

:thumbs_up:

Bastian Kauschke (Apr 13 2020 at 17:34, on Zulip):

Thank you for guiding me through all of this!

eddyb (Apr 13 2020 at 17:36, on Zulip):

@Bastian Kauschke so @bjorn3 left a comment, do you want to address it now, or...?

eddyb (Apr 13 2020 at 17:36, on Zulip):

like I want to do a try build but I just saw that comment just show up

Bastian Kauschke (Apr 13 2020 at 17:37, on Zulip):

idk. I intentionally used mem::discriminant here, as I meant the method, not the type

eddyb (Apr 13 2020 at 17:37, on Zulip):

okay, you can leave a comment about that

Bastian Kauschke (Apr 13 2020 at 17:47, on Zulip):

Whitespace :laughing:

---- [ui] ui/enum-discriminant/forbidden-discriminant-kind-impl.rs stdout ----
diff of stderr:

1   error[E0322]: explicit impls for the `DiscriminantKind` trait are not permitted
2     --> $DIR/forbidden-discriminant-kind-impl.rs:9:1
3      |
-   LL | impl DiscriminantKind for NewType {
+   LL | impl DiscriminantKind for NewType {
5      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl of 'DiscriminantKind' not allowed
6
7   error: aborting due to previous error

Seems like you have to restart bors

eddyb (Apr 13 2020 at 17:57, on Zulip):

@Bastian Kauschke try builds don't run tests :P

Bastian Kauschke (Apr 13 2020 at 17:58, on Zulip):

I already rebased, does that stop the build?

eddyb (Apr 13 2020 at 17:58, on Zulip):

nope

Bastian Kauschke (Apr 13 2020 at 17:59, on Zulip):

great :thumbs_up:

eddyb (Apr 13 2020 at 17:59, on Zulip):

but it's a bad idea because it breaks feedback from the bot once it's done, IIRC :P

eddyb (Apr 13 2020 at 17:59, on Zulip):

perf and crater will still work AFAIK

eddyb (Apr 13 2020 at 18:03, on Zulip):

@Bastian Kauschke note the yellow circle next to https://github.com/rust-lang/rust/pull/70705#ref-commit-9b5b377

eddyb (Apr 13 2020 at 18:03, on Zulip):

so bors is still going

Last update: Sep 28 2020 at 16:15UTC