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

When using associated_type_defaults: Why do I need to reimplement method with default impl when overriding assoc type default? #53907

Closed
Boscop opened this issue Sep 2, 2018 · 3 comments · Fixed by #61812
Assignees
Labels
A-associated-items Area: Associated items (types, constants & functions) A-specialization Area: Trait impl specialization F-associated_type_defaults `#![feature(associated_type_defaults)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Boscop
Copy link

Boscop commented Sep 2, 2018

rustc 1.30.0-nightly (02cb8f2 2018-08-29)

I'm using #![feature(associated_type_defaults)] and getting this error with the code below:

error[E0399]: the following trait items need to be reimplemented as `Msg` was overridden: `process`
   --> src\devices\bcr\mod.rs:110:2
    |
110 |     type Msg = ();
    |     ^^^^^^^^^^^^^^
pub trait DeviceSpecific {
	type Dev: Device;
}
pub trait DeviceApp<'a>: DeviceSpecific + Persistable {
	type Event: 'static;
	type BorrowedState;
	fn init_children_sync_viewers(&mut self, v: &Self::BorrowedState);
	fn handle_input(&mut self, e: <Self::Dev as Device>::InputEv, v: &mut Self::BorrowedState, now: u64) -> Option<Self::Event>;
	fn render(&self, device: &mut Self::Dev, v: &Self::BorrowedState);
	fn process<'b>(&'b mut self, device: &mut Self::Dev, events: Vec<<Self::Dev as Device>::InputEv>, v: &'b mut Self::BorrowedState, now: u64) -> Vec<Self::Event> where 'a: 'b {
		let r = events.into_iter().filter_map(|e| self.handle_input(e, v, now)).collect();
		self.render(device, v);
		r
	}
	type Msg: 'static = ();
	fn handle_msg(&mut self, e: Self::Msg);
}

/////

impl<'a> DeviceApp<'a> for BcrAppState {
	type Event = ();
	type BorrowedState = MyBorrowedState<'a>;
	fn init_children_sync_viewers(&mut self, v: &Self::BorrowedState) {
		// ...
	}
	fn handle_input(&mut self, e: <Self::Dev as Device>::InputEv, v: &mut Self::BorrowedState, now: u64) -> Option<Self::Event> {
		// ...
	}
	fn render(&self, device: &mut Self::Dev, v: &Self::BorrowedState) {
		// ...
	}
	type Msg = ();
	fn handle_msg(&mut self, _e: Self::Msg) {}
}

Why do I need to reimplement the process() method (which has a default impl) when overriding the Msg assoc type's default?
Why can't I keep using the default impl?
The process() method isn't even using the Msg assoc type.

@Centril
Copy link
Contributor

Centril commented Sep 2, 2018

The implementation of associated_type_defaults are not as in the RFC;
I have proposed a change to the design of the defaults in rust-lang/rfcs#2532.

@Boscop
Copy link
Author

Boscop commented Sep 2, 2018

So what's the current implementation status of associated_type_defaults?

I've read your RFC but I still have a question: Would it still require any impl of my DeviceApp trait that overrides the Msg assoc type to also override process()?
If yes, why? process() makes no assumptions about the Msg assoc type.

And even without the default groups (in the current implementation), I don't see why it would be required to force impls to also override process() here. So why is it currently implemented this way?
Is the current rule like "one atomic default { .. } block around all items that have a default value"?


With default groups, I'd write this, right? (1)

	default {
		type Msg: 'static = !;
		fn handle_msg(&mut self, e: Self::Msg) {}
	}

But I also want to have a blanket impl fn init_children_sync_viewers(&mut self, v: &Self::BorrowedState) {} (2) and when trait impls override this, I also don't want to force them to override process(), too.
With your RFC, would I still be forced to override process() if I have both (1) and (2)?

@Centril
Copy link
Contributor

Centril commented Sep 2, 2018

So what's the current implementation status of associated_type_defaults?

No one is working on it because it was clear that a different design was needed.

I've read your RFC but I still have a question: Would it still require any impl of my DeviceApp trait that overrides the Msg assoc type to also override process()?
If yes, why? process() makes no assumptions about the Msg assoc type.

From my understanding of your code, process() makes no assumptions about the underlying types of the associated type defaults. When this is the case, you don't need to redefine process(), so this should fit your use case well; in fact, much better than what the old associated items RFC specifies.

With default groups, I'd write this, right? (1)

You only need to do this if you actually depend on Msg = ! in the provided definition of handle_msg, which you don't. Therefore you don't need to put these into a group.

@estebank estebank added A-associated-items Area: Associated items (types, constants & functions) A-specialization Area: Trait impl specialization labels Jan 11, 2019
@jonas-schievink jonas-schievink added the F-associated_type_defaults `#![feature(associated_type_defaults)]` label Aug 29, 2019
@jonas-schievink jonas-schievink self-assigned this Sep 15, 2019
@jonas-schievink jonas-schievink added requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 4, 2020
@bors bors closed this as completed in 3a0d106 Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) A-specialization Area: Trait impl specialization F-associated_type_defaults `#![feature(associated_type_defaults)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants