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

[inline classes] Validation code does not fit well in constructors #3014

Closed
eernstg opened this issue Apr 21, 2023 · 7 comments
Closed

[inline classes] Validation code does not fit well in constructors #3014

eernstg opened this issue Apr 21, 2023 · 7 comments
Labels
extension-types inline-classes Cf. language/accepted/future-releases/inline-classes/feature-specification.md small-feature A small feature which is relatively cheap to implement.

Comments

@eernstg
Copy link
Member

eernstg commented Apr 21, 2023

One of the responsibilities that an inline class could have is validation of the representation object.

For example, in JS interop, if a given JSObject is accessed via a MyDomClass inline class then the representation object ought to be a JavaScript object whose actual structure and behavior matches the expectation associated with that particular Dom class. For example, a Button should act like a button, and a MenuItem should act like a menu item, and we would want to support some kind of validation or vetting of the representation object, such that it satisfies the assumptions made during the authoring of the inline class members that are operating on the representation object.

The point is that we can't use the representation type to perform the vetting because the representation object needs to satisfy some constraints that the type system cannot express. So the validation would be performed by running regular Dart code, and that Dart code can do whatever is needed in order to check that the representation object is valid.

The simplest example is probably EvenInt where we wish to ensure that the representation object is even (for whatever reason):

inline class EvenInt {
  final int rep;
  EvenInt(this.rep): assert(rep.isEven);
}

In this case we are using the constructor to perform the validation, which is the approach that we've used for a long time in discussions about this topic. It's obvious: The representation object gets vetted when it's first given the type EvenInt, and it will then remain valid (because rep is final, and must be final).

However, this approach does not fit well in the situation where several inline classes are related by a subtype relationship (that is: when some of them "inherit" method implementations from others):

inline class PositiveInt {
  final int rep;
  PositiveInt(this.rep): assert(rep >= 0);
  double get sqrt => ...;  // Won't work unless `rep >= 0`.
}

inline class EvenInt {
  final int rep;
  EvenInt(this.rep): assert(rep.isEven);
  int get half => ...; // Won't work unless `rep` is even.
}

inline class PositiveEvenInt implements PositiveInt, EvenInt {
  final int rep;
  PositiveEvenInt(this.rep): assert(rep >= 0 && rep.isEven);
}

An expression e of type PositiveEvenInt supports invocations like e.sqrt and e.half, using the member implementations in the two superinterfaces PositiveInt and EvenInt.

However, we don't have a mechanism to execute constructors in the superinterfaces. Doing that would not match the semantics because the relationship is a subtype relationship (implements), PositiveEvenInt doesn't inherit the fields from its superinterfaces. PositiveEvenInt does get to run the instance members declared by PositiveInt and EvenInt, but they are more like extension methods (where it is also true that an extension method that is applicable to a receiver of type T is also applicable to a receiver of an subtype of T, and it will run if it's the most specific one).

Besides, even if we were to try to use some ideas associated with constructor invocations in a superclass chain, we have no model for doing such things with several "supers".

In other words, it's just not a good fit to validate the representation object using code in constructors.

We could consider a different design:

Introduce a special semantics for a getter bool get isValid, if it exists: An invocation of isValid in an inline class I will execute isValid in all the superinterfaces of I that are inline classes, recursively, and return the && of all the return values.

This means that we could express the hierarchy safely like this:

// Hypothetically, relying on the special semantics of `isValid`:

inline class PositiveInt {
  final int rep;
  PositiveInt(this.rep): assert(isValid);
  bool get isValid => rep >= 0;
  double get sqrt => ...;
}

inline class EvenInt {
  final int rep;
  EvenInt(this.rep): assert(isValid);
  bool get isValid => rep.isEven;
  int get half => ...; // Won't work unless `rep` is even.
}

inline class PositiveEvenInt implements PositiveInt, EvenInt {
  final int rep;
  PositiveEvenInt(this.rep): assert(isValid);
}

The invocation of isValid in the constructor of PositiveEvenInt will invoke isValid in both superinterfaces (in the order PositiveInt::isValid && EvenInt::isValid, following the textual order in the implements clause, and in general ordered by some breadth-first left to right traversal of the superinterface graph).

This means that we avoid the semantic conflicts associated with any attempt to make the constructors do it, and we get access to a validation mechanism which can be invoked at any time, not just during construction.

Of course, the invocations of isValid and the && on the result can be inlined (if the code is small enough etc, such that it can be inlined in the first place). So if there's a lot of validation going on then it will take some resources, but that would have been true no matter how that code is invoked. In production, if isValid is never invoked except from assert(...), all the validation code would be tree-shaken away.

@eernstg eernstg added question Further information is requested inline-classes Cf. language/accepted/future-releases/inline-classes/feature-specification.md small-feature A small feature which is relatively cheap to implement. and removed question Further information is requested labels Apr 21, 2023
@jakemac53
Copy link
Contributor

I am really not a big fan of a magic thing like this isValid member. That just feels super magical to me. If anything I would create an annotation or something that you could use to mark a member as a "validation function" that must be ran, at least it will be more explicit in that case, and people won't accidentally write a function named isValid and end up having it ran automatically in a way they didn't expect.

Besides, even if we were to try to use some ideas associated with constructor invocations in a superclass chain, we have no model for doing such things with several "supers".

Why can't we just do the same special magic you are suggesting for isValid for constructors? Invoke the super constructors from right to left and call it a day?

@eernstg
Copy link
Member Author

eernstg commented Apr 21, 2023

I agree that there is a danger around magic semantics of selected names.

However, I don't think we can call the constructors throughout the superinterface graph: They will want to initialize the field (which is not inherited anyway), they may have side effects other than validation (say, they could create a record based on several constructor arguments). Finally, there is no obvious way to choose which constructors to run with which actual arguments (unless we manually write all those superinitializations, at least for all the immediate superinterfaces), in the initialization list of every constructor.

A boolean getter doing nothing other than validation doesn't have all those issues.

@jakemac53
Copy link
Contributor

They will want to initialize the field (which is not inherited anyway)
they may have side effects other than validation (say, they could create a record based on several constructor arguments).
Finally, there is no obvious way to choose which constructors to run with which actual arguments (unless we manually write all those superinitializations, at least for all the immediate superinterfaces)

Interesting, I guess I don't really understand how these inline classes work at all then (it is surprising to me that the constructors can even take more than one argument, much less do any initialization or construction of anything that can be long lived). The fact that there is even a real field is surprising to me :). I will read over the most updated version of the proposal today and re-familiarize myself.

@jakemac53
Copy link
Contributor

Ok, after reading through the proposal again, I am confused about the problem. It specifically states that constructors must have exactly one argument, and that argument can only match exactly the one required field. And that one field is not a real field it is really just a way of declaring the name for the wrapped value for use in members etc? Am I understanding that wrong?

If that understanding is correct, it seems like it should be safe to invoke all super constructors (we always have the one wrapped value that is necessary to pass), there is nothing to initialize, and we only need to specify the order in which they are invoked? The "constructors" are really just static methods, not true constructors, there is no object to construct (this is the entire point?). Unless I am missing something :).

@jakemac53
Copy link
Contributor

jakemac53 commented Apr 21, 2023

Ok, actually I think I misread things a bit, its only a compile time error for there to be more than one field. But I am not sure there is a good use case for any constructor taking more than exactly one argument (which "initializes" that field, but not actually). And that restriction would allow us to call super constructors (I think).

@lrhn
Copy link
Member

lrhn commented Apr 21, 2023

One of the responsibilities that an inline class could have is validation of the representation object.

Arguably, no. It's something it can try to do, but you can always create an "instance" with an unvalidated value by doing as InlineClassType.

Inline classes just don't support real sub-setting of the representation type.
If you want to have an abstraction which only allows certain values, then you can do that for all values the go through a constructor, but there is never any guarantee that all values come from constructors.

So inline classes not being particularly good at something they inherently cannot do, isn't a big issue to me.

Still, if someone wants to do this, it shouldn't be (much) worse than it is for classes.
And it isn't, necessarily.

If these types had been "real" classes, you'd have had to implement at least one of the interfaces, which means you cannot invoke its constructor's validation. You have to duplicate the validation in the subclass.

With inline classes, as currently defined, you have to implement both supertypes, and can invoke neither constructor.

If we introduce extends InlineClass (#2967) as a way to avoid having to write the field of the subclass, then you'll presumably also be allowed to chain to that superclass's constructor and get its validation. At that point, inline classes are feature-compatible with normal classes.

With inline classes, we can also take advantage of knowing that constructors aren't allocating, so you can do:

class EvenPositiveInteger implements EvenInteger, PositiveInteger {
  final int _rep;
  EvenPositiveInteger(this._rep) {
    EvenInteger(_rep);  
    PositiveInteger(_rep);
  }
}

and all those constructor invocations really do is to run the assert of that constructor.

I am really not a big fan of a magic thing like this isValid member.

Yeah, that's a diplomatic way to put my opinion 😉 .

If this is a proposal for making sub-setting possible, then I can see where the isValid approach would fit in,
but as it is, it's a solution to a problem we're not actively looking for a solution to.
And it's "too much magic".

Every one of those classes could provide a static bool isValid(int) method, and you could call them explicitly, with no magic required.

@jakemac53
There can be plenty of reasons for constructors to take more than one argument, they're just not the representation object.
Take:

inline class Point {
  final ({double x, double y}) _coordinates;
  Point(double x, double y) : _coordinates = (x: x, y: y);
  double get x => _coordinates.x;
  double get y => _coordinates.y;
  double get distanceToOrigo => sqrt(x * x + y * y);
}

It can be like any other class which has one instance variable, and no superclass.

@eernstg
Copy link
Member Author

eernstg commented Jul 28, 2023

OK, nobody else wants to support 'vetting' of the representation object of an extension type, so I'll close this issue. It is of course still possible to write the same code manually.

@eernstg eernstg closed this as completed Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-types inline-classes Cf. language/accepted/future-releases/inline-classes/feature-specification.md small-feature A small feature which is relatively cheap to implement.
Projects
None yet
Development

No branches or pull requests

3 participants