evcxr / evcxr

Other
5.5k stars 217 forks source link

`conflicting implementations` when rerun a jupyter cell #260

Open HKalbasi opened 1 year ago

HKalbasi commented 1 year ago

Sorry if it is duplicate. I sometimes re run jupyter cells, and if one of them contains an impl Trait for Type I will get an error like this:

[E0119] Error: conflicting implementations of trait `Mammal` for type `Cat`
   ╭─[command_18:1:1]
   │
 1 │ impl Mammal for Cat {
   · ─────────┬─────────  
   ·          ╰─────────── conflicting implementation for `Cat`
───╯

I expect the new impl to override the old one, similar to functions and structs.

bjorn3 commented 1 year ago

Overriding a trait implementation is not something that can be done soundly as the code for previous invocations is still loaded in the evcxr process and this code may use the previous implementation.

HKalbasi commented 1 year ago

Is dyn Trait your concern? I think between persisting dyn Trait objects and changing trait implementations I would prefer it to reject persisting dyn Trait objects, like how it doesn't persist closures and non static types. But it is possible to persist dyn Traits and remove them only when a change in some trait implementation was detected, with a message like bindings x, y, z was discarded due trait implementation change.

bjorn3 commented 1 year ago

It is also possible to spawn a background thread that uses the old implementation. In addition it is not possible for evcxr to know if some type contains dyn MyTrait, for example because it is wrapped in Box<dyn Any> or a typemap::TypeMap.

One possibility would be to define an entirely new type every time a trait impl is replaced. In that case the old type becomes unreachable except through dyn Trait and in the case of dyn Trait the old impl will remain used.

HKalbasi commented 1 year ago

Wouldn't background threads use the old binary so they won't be affected by the changes in trait implementation? The casting to dyn Any is an interesting problem, the invalidating solution probably needs to invalidate all dyn Traits on each change.

But I tried and the unsoundness problem already exists, in most simple case:

Welcome to evcxr. For help, type :help
>> struct X(i32);
>> let x = X(5);
>> x.0
5
>> struct X(i64);
>> x.0
93939524698117

The general solution to this problem is probably something around your suggestion. A new type on each type change. But keeping compiler errors beautiful would be a challenge.

davidlattimore commented 1 year ago

Huh, that's unfortunate. I guess X(i32) and X(i64) are being assigned the same type-id (in different compilation units), otherwise evcxr would be reporting that the variable x was lost due to its type changing. Presumably it's using only the path to the type (X) to create the type ID and not also including a hash of the contained field types (i32 or i64).

I think probably the best thing to do would be to say that when a type is redefined, all impl blocks for that type get dropped. The starting point for that code is self.unnamed_items.push(item_block) in eval_context.rs in case you're interesting in taking a look.

bjorn3 commented 1 year ago

Presumably it's using only the path to the type (X) to create the type ID and not also including a hash of the contained field types (i32 or i64).

It also uses the -Cmetadata arguments and the crate name. Rustc requires that crates with the same name have distinct -Cmetadata arguments when attempting to link them together to prevent symbol and type id conflicts. When using extern crate/--extern, rustc will check that this holds and refuse to link if there are conflicts, but when using dlopen, it is your own responsibility to handle this.

davidlattimore commented 1 year ago

Thanks, @bjorn3, that makes sense. I guess to make use of that, I'd need to switch back to having each compilation unit use a new crate name and each crate depend on all previous crates. I used to do something like that in very early versions of evcxr, but moved to using the same crate name for all compilations since it allows you to do thing like struct Foo {} in one line, then impl Foo {...} in a second line. If the two lines were put in separate crates, then rustc would reject it due to coherence.

bjorn3 commented 1 year ago

I think you can duplicate the type definition (and all existing trait impls) every time you add or remove a trait impl. It may be confusing for users though as variables using the old type definition could still exist.