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

Suggest using getters in mixins #4767

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

KyleFin
Copy link
Contributor

@KyleFin KyleFin commented Apr 14, 2023

I've long understood how mixins could provide value, but it took me way too long to understand how to access state in the super class (which makes mixins far more powerful). I'm not sure if this rewording is enough or if it may be helpful to add a simple example of a getter or accessing a member of the superclass in the mixin.

I like the brief docs, but this is a case where one more example would've gone a long way for me.

I've long understood how mixins could provide value, but it took me way to long to understand how to access state in the super class (which makes mixins far more powerful). I'm not sure if this rewording is enough or if it may be helpful to add a simple example of a getter or accessing a member of the superclass in the mixin.

I like the brief docs, but this is a case where one more example would've gone a long way for me.
src/language/mixins.md Outdated Show resolved Hide resolved
For example, the mixin might depend on being able to invoke a method
that the mixin doesn't define.
Sometimes the mixin might depend on being able to invoke a method or access fields
that the mixin doesn't define. To require classes that use the mixin to define these,
Copy link
Contributor Author

@KyleFin KyleFin Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
that the mixin doesn't define. To require classes that use the mixin to define these,
that the mixin doesn't define. Mixins can't define a constructor,
so they can't use constructor parameters to instantiate their own fields.
To require classes that use the mixin to define these,

I don't know that this is important to include in the docs, but this is where I was getting stuck with the existing example from the docs.

Copy link
Contributor

@atsansone atsansone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleFin: Updated the copy, would like a tech review to be sure.

@atsansone atsansone requested a review from lrhn April 17, 2023 14:44
@atsansone atsansone added the review.tech Awaiting Technical Review label Apr 17, 2023
@MaryaBelanger
Copy link
Contributor

Hi @KyleFin, thanks for working on this and apologies for the delay. I just looked around for an example of using a getter in a mixin, or accessing members of the superclass at all, and couldn't find anything in our docs, so I think adding that example is a good point.

In the mean time, I wonder if you could read through this comment from a different-but-related PR, and if the example snippets or the subsequent conversation about the paragraph you edited provide any extra insight for you?

I am planning to restructure the entirety of the mixins page based on that comment thread, and I think your point here is important to factor in. I'm just wondering if you have any thoughts on how your change fits into that conversation.

that the mixin doesn't define.
Sometimes the mixin might depend on being able to invoke a method or access fields
that the mixin doesn't define. To require classes that use the mixin to define these,
you can define getters in the mixin or restrict the types that can use a mixin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is meant by "restrict the types that can use a mixin"?

I'm curious how a mixin restricts types that use it, and feel this point should probably be expanded somewhere.

PTAL @munificent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not Bob 🙃 but I think that wording puts the emphasis on the wrong part of the point its trying to make. It's the wording that is currently on the page, it's not new to this PR:

Sometimes you might want to restrict the types that can use a mixin.

The point though, isn't restricting the types that can use it, but ensuring that any methods the mixin might rely on that are defined by another type are in fact actually defined before the mixin can be used. I.e. that the mixin can only be used with the on directive specifying the superclass that defines whatever it is the mixin relies on (or the abstract way to accomplish the same thing, explained in this comment and (kind of) in the new section I added after this PR was created).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial PR comment says:

it took me way too long to understand how to access state in the super class (which makes mixins far more powerful).

But I think this is a (very common) misunderstanding of the inheritance order that mixins give you. What you most often want is to be able to access state in the mixin's subclass. For example:

/// Can be applied to any type with a [name] property and provides an
/// implementation of [hashCode] and operator `==` in terms of it.
mixin NameIdentity {
  String get name;

  int get hashCode => name.hashCode;
  bool operator ==(other) => other is NameIdentity && name == other.name;
}

class Person with NameIdentity {
  final String name;

  Person(this.name);
}

I think this is probably the kind of code that @KyleFin has in mind. (If not, correct me. :) ). In this example, Person is the subclass of NameIdentity.

So there's no need for an on clause on NameIdentity. (The rule for on clauses is simple: If your mixin doesn't have any super calls, you don't need an on clause.) Instead, all you need is an abstract member declaration. In this case, it's String get name;.

I agree with @KyleFin that users often don't realize they can have mixins that access state on the class they are applied to by calling getters which are defined as abstract on the mixin. I like the idea of adding a little emphasis about that to the docs. It's valuable because the restriction that mixins can't have their own fields with initializers can make them harder to use without knowing this technique.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @munificent's example with an abstract getter is what I had in mind. Thanks for the excellent context! I agree with the plan to rewrite the examples, removing emphasis on on.

I believe my misunderstanding about which is the super/sub-class came from the current wording you can restrict a mixin's use by using the on keyword to specify the required superclass. @munificent's comment clarifies the strange behavior of on. I agree it shouldn't be highlighted in the language tour, but maybe there should be some description somewhere since it's a keyword? Maybe a link to these comment threads? Maybe the on keyword could be marked as deprecated or something to limit incorrect usage?

Maybe on could be omitted from this mixin documentation but there could be on (exception) and on (mixin) entries in https://dart.dev/language/keywords, similar to final (variable) and final (class)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the on keyword could be marked as deprecated or something to limit incorrect usage?

It's not deprecated because it does have intended uses. They're just rare. You need an on clause when you want to have a super call inside the mixin, like:

class SomeBaseClass {
  baseClassMethod() {
    print('Base class!');
  }
}

mixin SomeMixin on SomeBaseClass {
  mixinMethod() {
    print('Calling superclass method!');
    super.baseClassMethod(); // <--
  }
}

class SomeDerivedClass extends SomeBaseClass with SomeMixin {}

main() {
  SomeDerivedclass().mixinMethod();
}

The on clause exists to define the type that super calls are resolved against. In the example here, the on SomeBaseClass clause is what the compiler uses to know that super.baseClassMethod() is OK.

KyleFin added a commit to KyleFin/site-www that referenced this pull request Jul 5, 2023
parlough pushed a commit that referenced this pull request Jul 5, 2023
rmacnak-google pushed a commit to rmacnak-google/site-www that referenced this pull request Sep 5, 2023
@atsansone
Copy link
Contributor

@KyleFin : With #5043 merged, did you still need this reviewed and merged as well? I believe we need additional updates from you. Please advise. Thanks!

@atsansone atsansone added review.await-update Awaiting Updates after Edits and removed review.tech Awaiting Technical Review labels Sep 15, 2023
@KyleFin
Copy link
Contributor Author

KyleFin commented Oct 23, 2023

@atsansone thanks for reaching out! Yes, this PR is more important to me than #5043, and I think it would be valuable to update the mixin docs.

I'll write up what I'd propose for wording and example code. After playing with it a bit more, these are the 3 cases I'm thinking to cover:

  • Define abstract methods (including getters) in a mixin.
    • To be implemented by subclass.
  • Have a mixin implement an interface.
    • Can be implemented in mixin OR subclass.
  • on SuperClass
    • Subclass must extend a class that implements SuperClass

@atsansone
Copy link
Contributor

@KyleFin : Apologies for this slipping between the cracks. Could you make your updates and I'll address this PR when they have been made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.await-update Awaiting Updates after Edits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants