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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Suggest using getters in mixins #4767

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d4c9403
Suggest using getters in mixins
KyleFin Apr 14, 2023
f7d8931
Update src/language/mixins.md
KyleFin Apr 14, 2023
d1acde6
Merge branch 'main' into patch-2
MaryaBelanger May 31, 2023
0f4e770
Merge branch 'main' into patch-2
domesticmouse May 31, 2023
76fcba7
Merge branch 'main' into patch-2
MaryaBelanger Jun 9, 2023
b882601
Merge branch 'main' into patch-2
atsansone Sep 15, 2023
32a4756
Merge branch 'main' into patch-2
domesticmouse Sep 18, 2023
84689fc
Merge branch 'main' into patch-2
domesticmouse Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/language/mixins.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ mixin Musical {
}
```

Sometimes you might want to restrict the types that can use a mixin.
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 •
edited by atsansone
Loading

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.

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.

As the following example shows, you can restrict a mixin's use
by using the `on` keyword to specify the required superclass:

Expand Down