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

Require View: ViewMarker to avoid the leaky abstraction/workaround fn view() -> impl View + ViewMarker #162

Closed

Conversation

Philipp-M
Copy link
Contributor

This PR adds the Requirement View: ViewMarker.

Motivation: to avoid requiring fn view() -> impl View + ViewMarker and instead just using fn view() -> impl View

I don't see a reason for types that implement View, but not ViewMarker. This requirement has the neat side-effect of avoiding weird error-messages otherwise, in case impl ViewMarker for T has been forgotten for impl View for T.

The whole purpose AFAIK of ViewMarker is a workaround of the orphan rule, that something like this:

impl<T, A, V: View<T, A> + ViewMarker> ViewSequence<T, A> for V

is possible.

So I tried adding this as super trait requirement and it seems to be possible.
I think this makes the leaky abstraction/workaround much more bearable as it's now only required for actual View implementations (AFAICS).

@Philipp-M
Copy link
Contributor Author

Btw. I could for whatever reason not remove the requirement ViewMarker in

impl<T, A, V: View<T, A> + ViewMarker> ViewSequence<T, A> for V

Does someone know why? Is this a bug in Rust?

@Philipp-M
Copy link
Contributor Author

Answering my own comment (so I kinda think it's some form of bug in Rust), I think it's because View<T, A> is generic, while ViewMarker is not, and the typesystem covers cases for non-generic traits but not for generic types (and doesn't expand this for super-traits seemingly). But that's just a guess, I think this needs a deeper dive in the type-system of the Rust compiler to be certain etc.

@Philipp-M
Copy link
Contributor Author

Philipp-M commented Jan 19, 2024

One thing this change introduces though, is that types that implement View can now not implement ViewSequence anymore.

I think this is ok, because I had some weird issues some time back in #141 (comment)

So I doubt that it's generally a good idea to have a type that has manual implementations for View and ViewSequence.

@Philipp-M Philipp-M force-pushed the view-with-supertrait-viewmarker branch from f53ed72 to 9e1665d Compare February 12, 2024 00:50
…-> impl View + ViewMarker`

Motivation: to avoid requiring `fn view() -> impl View + ViewMarker` and instead just using `fn view() -> impl View`

I don't see a reason for types that implement `View`, but not `ViewMarker`. This requirement has the neat side-effect of avoiding weird error-messages otherwise, in case `impl ViewMarker for T` has been forgotten for `impl View for T`.

The whole purpose AFAIK of `ViewMarker` is a workaround of the orphan rule, that something like this:
```rust
impl<T, A, V: View<T, A> + ViewMarker> ViewSequence<T, A> for V
```
is possible.

So I tried adding this as super trait requirement and it seems to be possible.
I think this makes the leaky abstraction/workaround much more feasible as it's only required now for actual `View` implementations.
@Philipp-M Philipp-M force-pushed the view-with-supertrait-viewmarker branch from 9e1665d to 0a0efc8 Compare February 15, 2024 11:04
@DJMcNab
Copy link
Member

DJMcNab commented Jun 15, 2024

I think this can be closed, as this targets I guess Xilem Web Core?

@Philipp-M
Copy link
Contributor Author

Yes, when we don't ever want to return impl ViewSequence we don't need this anymore.
And since I think it's a bigger advantage to have the same type being a ViewSequence and a View as seen with OneOf we likely want to stay with the new marking mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants