language icon indicating copy to clipboard operation
language copied to clipboard

Records: Why are private entries forbidden?

Open leafpetersen opened this issue 1 year ago • 6 comments

The records proposal forbids using private names in a record:

A field name that starts with an underscore. If we allow a record to have private field names, then those fields would not be visible outside of the library where the record was declared. That would lead to a record that has hidden state. Two such records might unexpectedly compare unequal even though all of the fields the user can see are equal.

I don't understand the argument against allowing this presented here. While it's certainly true that the "hidden" state makes them unequal... that's true of many things in Dart (e.g. classes with private fields). Why is this a problem?

This does in general feel like something that could be useful.

This also feels like a problem if/when we want to use these to reify argument lists, since it is legal (albeit of limited use) to have parameters with private names.

cc @lrhn @munificent @eernstg @jakemac53 @natebosch @stereotype441

leafpetersen avatar Aug 05 '22 23:08 leafpetersen

I don't understand the argument against allowing this presented here. While it's certainly true that the "hidden" state makes them unequal... that's true of many things in Dart (e.g. classes with private fields).

Sure, but with classes, encapsulation is their raison d'être. If a class does define equality (and most don't), the class author is obliged to define it in a way that makes sense to external users of the class.

With a record, the expectation is that they are transparent aggregate value types. What you see is what you get. I think it would be pretty strange to have a record with hidden fields—sort of like a list with secret indexes or something.

Why is this a problem?

I think we probably could make it work if we really wanted to, but it gets weird:

// lib_a.dart
// Doesn't work:
({foo int}) make() => (foo: 1, _bar: 2);
// No width subtyping, so can't encapsulate private field.

// Have do to:
({foo int, _bar int}) make() => (foo: 1, _bar: 2);


// lib_b.dart
import 'lib_a.dart';

main() {
  // OK, because can infer type. But no annotation you could write.
  var record = make();

  print(record.foo); // OK.
  print(record._bar); // Doesn't work. No getter you can call.

  switch (record) {
    case (foo: f): // Doesn't match.
      // No width subtyping so pattern only matches exact shape.
  }
}

Basically, if you ever let a record with a private field name escape the library, then the name is inaccessible but not encapsulated. The resulting object doesn't behave like a record that doesn't have the field. The field is still there and it clearly affects user-visible behavior. You just can't get to it.

If the record never leaks the library, it's fine. But... if it never leaks, why are you using a private name?

This also feels like a problem if/when we want to use these to reify argument lists, since it is legal (albeit of limited use) to have parameters with private names.

This is true. There are other things we'll need to tackle to reify argument lists too. In particular, empty tuples and unary tuples.

But the initial release of records and patterns won't support reifying argument lists, so I'm kicking those down the road for now in order to keep the proposal simpler today. (I am trying to be mindful to not make design choices that make it harder to add those refinements later.)

For now, I don't think private record fields are useful and are kind of problematic, so my default answer is to just forbid them.

munificent avatar Aug 05 '22 23:08 munificent

Yeah, pattern matching is a problem (though I wonder whether we should have a ... pattern for records). For the types, you can always define a typedef.

leafpetersen avatar Aug 06 '22 00:08 leafpetersen

(though I wonder whether we should have a ... pattern for records).

Yeah, I had a TODO in the proposal for that. Lasse suggested not doing it because it may require us to keep around more reflective information for records than we'd like so I took it out for now.

For the types, you can always define a typedef.

Good point.

munificent avatar Aug 06 '22 00:08 munificent

since it is legal (albeit of limited use) to have parameters with private names.

It is not allowed to start named parameters with an underscore. The names of positional parameters should not be meaningful.

natebosch avatar Aug 08 '22 17:08 natebosch

It is not allowed to start named parameters with an underscore. The names of positional parameters should not be meaningful.

Huh, I guess when we've discusses this it's been in the context of relaxing that? Learn something new every day. Oddly enough, the CFE allows it for required named, but then throws an error. I'll file a bug.

leafpetersen avatar Aug 08 '22 20:08 leafpetersen

Yeah, I had a TODO in the proposal for that. Lasse suggested not doing it because it may require us to keep around more reflective information for records than we'd like so I took it out for now.

I wouldn't propose making this polymorphic. That is, var (x : x, ...) = e shouldn't work for all record types, only one specific concrete record type. e.g. if e has type ({int x, int y}), then I would make the above be syntactic sugar for var (x : x, y : _), with a static error if the type wasn't statically resolvable.

leafpetersen avatar Aug 08 '22 20:08 leafpetersen

I wouldn't propose making this polymorphic. That is, var (x : x, ...) = e shouldn't work for all record types, only one specific concrete record type. e.g. if e has type ({int x, int y}), then I would make the above be syntactic sugar for var (x : x, y : _), with a static error if the type wasn't statically resolvable.

Ah, that's interesting. I think it gets more interesting if the syntax appears in a refutable context:

test(Object obj) {
  switch (obj) {
    case (x: 3, ...): print('has 3 x');
  }
}

Will users expect this to be a compile error, or will they expect it to be valid and match any record with a field named x? I suspect the latter. If so, can we live with confounding that expectation?

munificent avatar Aug 18 '22 22:08 munificent

I'm closing this because I don't see a strong push to allow private field names, but feel free to re-open if you want.

munificent avatar Aug 18 '22 22:08 munificent

I continue to find it a bit odd that we're going out of our way to forbid this. It's certainly something I can imagine myself using in various ways. I'd like to discuss this, at least.

leafpetersen avatar Sep 19 '22 12:09 leafpetersen

It's certainly something I can imagine myself using in various ways.

Can you give me a use case? It feels pretty strange to me to have a structural type whose type can't be written outside of the library where it's defined.

munificent avatar Sep 19 '22 18:09 munificent

You can write the type, just use a typedef:

typedef MyData = ({int x, int _nunyaBusiness});

MyData makeMyData(int x) => ({x : x, _nunyaBusiness : something()});

MyData frobIt(MyData foo) {...}

This gives you a type which has some public fields, and some private stuff. You could make it all private, and use functions/extensions for all of the API, or you can have some public and some private stuff there and let people access the public names.

leafpetersen avatar Sep 19 '22 18:09 leafpetersen

Do we want people to write code like that? Using a typedef to create what sort of behaves like a nominal type but is actually just an unspeakable structural type feels like an anti-pattern to me. If I saw code like that in a code review, I would definitely tell the author to just make a class (which will hopefully get much easier with views/structs/primary constructors/whatever).

munificent avatar Sep 19 '22 19:09 munificent

Do we want people to write code like that? Using a typedef to create what sort of behaves like a nominal type but is actually just an unspeakable structural type feels like an anti-pattern to me. If I saw code like that in a code review, I would definitely tell the author to just make a class (which will hopefully get much easier with views/structs/primary constructors/whatever).

Hmm. This seems to me to largely be a matter of taste. I get that using classes for everything is a certain style, and perhaps even one that we will choose to strongly encourage as a team, but frankly, I don't understand why you object so strongly to that style that you'd literally go out of your way to build a prohibition against it into the language. Concretely, make the argument for me. Why is that an anti-pattern, other than that you don't like it? To me, it has a lot going for it:

  • It's going to be highly performant (much more than classes). There are no dynamic calls to worry about, no overriding, no polymorphism, no interface calls... just plain old data. It's likely quite unboxable by the compiler (because identity). Minimal code size. Very inlinable.
  • It's very maintainable: I don't need to worry about people implementing/extending/mixing in etc my nice simple plain old data.
  • It is almost the only way in Dart to introduce a type Foo for which I know absolutely that any instance of Foo is one that I built and control (the only other way I can think of is by using a private class, and returning the instance from a builder function, in which case you have an instance of a type you can't name at all - if you use the typedef trick it becomes implementable, etc).
  • It's eminently understandable code. There's no inheritance, virtual methods, mutability, etc.
  • It's very compact, minimal or no boilerplate.

I don't think I would argue that this is the amazing cool new way of writing all Dart code, but... it feels pretty overbearing to just go out of our way to forbid this for no other reason than that we (some of us) don't like it.

leafpetersen avatar Sep 19 '22 19:09 leafpetersen

I don't see this as a bad thing. It would be great to use this in the near term while structs are being worked on/fleshed out.

Records could even be the starting point to unify the underlying implementations of structs and views. Instead of making the VM, Dart2JS, DDC, etc. aware of how to handle records + structs + views maybe a CFE transform could turn structs and views into records. Because the only difference between a record and a struct is structural typing vs nominal typing.

ds84182 avatar Sep 19 '22 20:09 ds84182

  • It is almost the only way in Dart to introduce a type Foo for which I know absolutely that any instance of Foo is one that I built and control (the only other way I can think of is by using a private class, and returning the instance from a builder function, in which case you have an instance of a type you can't name at all - if you use the typedef trick it becomes implementable, etc).

This is one small concern I have. This is a capability that you can only use indirectly. It feels a little bit like adding a private generative constructor to prevent extends.

natebosch avatar Sep 19 '22 20:09 natebosch

Hmm. This seems to me to largely be a matter of taste.

That is partially what users are asking us to do with the language. Part of language design is picking a set of idioms that we think are the best ways to model a program and enshrining them with syntax, and not enshrining others with syntax if we think they are redundant or less effective.

, but frankly, I don't understand why you object so strongly to that style that you'd literally go out of your way to build a prohibition against it into the language. Concretely, make the argument for me. Why is that an anti-pattern, other than that you don't like it?

I don't want Dart users to see code like:

typedef MyData = ({int x, int _nunyaBusiness});

And have to squint through it to see that the underlying intent is to define a publicly-accessible nominal datatype. The language already has a well-understood way to do that: classes. If classes aren't good enough for defining your API's nominal types, we should fix classes, not provide a set of unrelated features that users can cobble together themselves to route around them.

(I have the exact same concern around people defining extensions on types they control specifically to get static dispatch.)

To me, it has a lot going for it:

  • It's going to be highly performant (much more than classes). There are no dynamic calls to worry about, no overriding, no polymorphism, no interface calls... just plain old data. It's likely quite unboxable by the compiler (because identity). Minimal code size. Very inlinable.

If this ends up being significantly faster than classes, we will see "best practices" articles come out saying to never use classes in Dart. Users will cargo cult this and replace all of their classes with typedefed records. I've seen that kind of behavior happen before. The promise of performance is catnip for programmers and they will abuse their code in all sorts of ways to seek it. (How often do you still see people write ++i instead of i++ because the former is "faster" when that hasn't been true since, like, the 80s?)

Records do support dynamic calls. If overriding, polymorphism, and interface calls are a problem for users, we should give them a way to disallow those for classes too. Type modifiers covers half of that. Modifiers on members would cover the rest.

  • It's very maintainable: I don't need to worry about people implementing/extending/mixing in etc my nice simple plain old data.

Also, it's not clear to me that this is more maintainable. Let's say you want to return some "fast data type", so you do:

typedef MyData = ({int x});

MyData makeMyData(int x) => ({x: x});

Later, you realize that there's an extra bit of hidden state you want to store, so you add:

typedef MyData = ({int x, int _nunyaBusiness});

MyData makeMyData(int x) => ({x : x, _nunyaBusiness : something()});

This seems non-breaking. It's non-breaking to add a private field to a class. But it is a breaking change to add a private field to a record that previously didn't have any. Because there could be code in the wild like:

MyData thing = (x: 123); // Why not? It's just a record.
var (x: x) = thing; // Also fine. It's just a record.

I suppose the idea would be that if you want to expose a record type hidden behind a typedef and ever want to potentially add a private field, you should preventatively add a pointless private field on day one so that users can never rely on it actually behaving like a record type.

This just seems like a brittle, confusing pattern. If you want a nominal type make a class (or struct). If you don't want it to be extended, implemented, etc., we should let you control that.

  • It is almost the only way in Dart to introduce a type Foo for which I know absolutely that any instance of Foo is one that I built and control (the only other way I can think of is by using a private class, and returning the instance from a builder function, in which case you have an instance of a type you can't name at all - if you use the typedef trick it becomes implementable, etc).

The type modifiers proposal would cover that for classes.

  • It's eminently understandable code. There's no inheritance, virtual methods, mutability, etc.

It's understandable once you know the trick. But anyone looking at that first is going to wonder what's going on.

  • It's very compact, minimal or no boilerplate.

All the more reason to do default constructors on classes, right?

I don't think I would argue that this is the amazing cool new way of writing all Dart code, but... it feels pretty overbearing to just go out of our way to forbid this for no other reason than that we (some of us) don't like it.

This is definitely not selling me on it. My opposition to private fields is not just because I don't like this pattern (though I definitely don't). It's that I don't feel like we have a good handle on the implications of private fields on records in general. My experience is that whenever a feature touches on privacy, we run into weird corners:

  • Enhanced default constructors got stuck in part because it wasn't clear how to initialize private fields with them. Does the parameter name become public? What if that collides with a corresponding public field?
  • How do they interact with noSuchMethod()? Can you intercept them?

I would be surprised if there weren't weird interactions if we support records with private fields. For example, how does spreading and record update interact with it? Can I update a record with a private field from another library as long as I don't update the private field? Does it still get copied over? Can I spread a record with a private field from another library into a record in this library? If so, does the new record get that private field or not? What if I'm spreading it in a context type that does have that private field as in:

// a.dart
typedef MyData = ({int x, int _nunyaBusiness});
MyData makeMyData(int x) => ({x : x, _nunyaBusiness : something()});

// b.dart
MyData copy = {...makeMyData(1)};

Does that work?

If we allow private record fields, we're stuck with those implications forever, whatever they are. Given that we're short on time and don't have use cases (at least not ones that I find compelling), I think the safest answer is to make it an error now. Otherwise, I worry it will just be a regretful choice, and given how many other open issues there are to figure out, this doesn't seem like a corner worth spending a lot of time on in order to ensure it isn't a regretful choice. We can always loosen the restriction later if we see user demand.

But, honestly, I don't think I ever want to support this. I think structural types should be transparent. If you want privacy and encapsulation, that's what libraries and nominal typing are for. We could allow function types with private named parameters and say that you can only invoke the function in the library it's defined. But we don't and I haven't seen any demand for it. I think it's just a simpler overall language—easier to learn, teach, understand, and extend—if we don't try to have the structural parts of the language interact with the encapsulation features.

munificent avatar Sep 19 '22 20:09 munificent

Maybe the shorter answer I could have written is: It seems fundamentally confusing to me to have records that you can't pattern match on, and that's exactly what you get if a record with a private field escapes the library where it's declared.

munificent avatar Sep 20 '22 00:09 munificent

I think it would be confusing to allow private named fields of records when we don't allow private named optional arguments, e.g. to constructors.

(The compilers allow required private named arguments, but each compiler is broken in its own way if you try to use them. I suspect that this is unintentional.)

rakudrama avatar Sep 20 '22 03:09 rakudrama

That is partially what users are asking us to do with the language. Part of language design is picking a set of idioms that we think are the best ways to model a program and enshrining them with syntax, and not enshrining others with syntax if we think they are redundant or less effective.

I don't think this is a good example of that though. This is like building in a camel case, or forbidding a method named setX. Yes, we discourage using methods as setters and getters, no we shouldn't build that into our model of the language.

(I have the exact same concern around people defining extensions on types they control specifically to get static dispatch.)

And yet this is a thing that our users are doing, and finding value in. Perhaps we should be more humble here, rather than deciding that our users are just wrong and should be punished?

If this ends up being significantly faster than classes, we will see "best practices" articles come out saying to never use classes in Dart. Users will cargo cult this and replace all of their classes with typedefed records. I

I don't believe this, I think this is a strawman. There are places where it performance matters, places where it doesn't. Extension methods are faster than virtual methods, there is no such cargo cult practices around them that I'm aware of.

Records do support dynamic calls.

You can't make a dynamic call on a private field outside of your own library, was my point.

If overriding, polymorphism, and interface calls are a problem for users, we should give them a way to disallow those for classes too. Type modifiers covers half of that. Modifiers on members would cover the rest.

Yes, it would be nice to get those. We don't have them.

I suppose the idea would be that if you want to expose a record type hidden behind a typedef and ever want to potentially add a private field, you should preventatively add a pointless private field on day one so that users can never rely on it actually behaving like a record type.

This is a really poor counterargument. Of course changing a public thing to a partially or wholly private thing is breaking. That doesn't change the fact that I can build up a bundle of data, in which some or all of the fields are private, and expose its API via extension methods or static functions, and any change I want to make at all to the representation of the private part of that data is 100% non-breaking.

  • Enhanced default constructors got stuck in part because it wasn't clear how to initialize private fields with them. Does the parameter name become public? What if that collides with a corresponding public field?

I'm 100% not a fan of Dart's approach to privacy, but... I think I proposed a simple solution to that, which nobody liked because they don't like seeing these names in public documentation. shrug. In any case, I don't see any issues here with records.

  • How do they interact with noSuchMethod()? Can you intercept them?

There is no noSuchMethod for records, right? So I don't understand what you're asking.

I would be surprised if there weren't weird interactions if we support records with private fields. For example, how does spreading and record update interact with it?

I don't know how spreading and record update work at all, because we don't have it, but...

Can I update a record with a private field from another library as long as I don't update the private field?

I'd say no.

Does it still get copied over?

No.

Can I spread a record with a private field from another library into a record in this library?

No.

If so, does the new record get that private field or not? What if I'm spreading it in a context type that does have that private field as in:

// a.dart
typedef MyData = ({int x, int _nunyaBusiness});
MyData makeMyData(int x) => ({x : x, _nunyaBusiness : something()});

// b.dart
MyData copy = {...makeMyData(1)};

Does that work?

No. All instances of MyData that are ever created are created by makeMyData. That's the nice thing about the abstraction here.

Maybe the shorter answer I could have written is: It seems fundamentally confusing to me to have records that you can't pattern match on, and that's exactly what you get if a record with a private field escapes the library where it's declared.

You seem to tie records very fundamentally to pattern matching in a way that I don't see as warranted. Records exist in any number of languages without pattern matching.

To me, this seems like a pretty clear cut thing we should support. There is an existing feature in Dart (library privacy), which allows names of fields to be private to a library. We're adding a new feature to Dart (records) which have fields. The natural, clean, orthogonal thing is to simply say that fields in records, like fields/variables anywhere else, can be library private. Going out of our way to say that they can't be private feels like something that needs justification, and I'm really not seeing any good arguments here to justify going out of our way to make this an error.

leafpetersen avatar Sep 20 '22 08:09 leafpetersen

I would find it very confusing to allow private fields in records. IMO it's the kind of thing that will be mostly used for unintuitive workarounds.

mateusfccp avatar Sep 20 '22 16:09 mateusfccp

I usually prefer designs with a high degree of orthogonality (i.e., designs supporting "all combinations of all features", whatever that means). The main point is that this provides a semantic space which is simpler (a cube is simpler than most fractals), and this allows developers to use simple mental rules about what's possible and what is not possible.

The next benefit associated with orthogonal designs is that they're all about lifting restrictions, that is, the orthogonal design is more expressive than any non-orthogonal ones (based on the view that the latter is the former plus some exceptions specifying that you can't do this and that). And developers might come up with some really useful and convenient idioms based on some of those weird corner cases that nobody had a use case for.

We could use var (a, b, ...) to extract the two first fields of a record of type (int, int, {String _secret}) where _secret is private to a different library. Very similar to private instance variables of an instance of a normal class.

I don't really see the problem.

eernstg avatar Sep 20 '22 16:09 eernstg

In my current understanding, any change to the shape of returned Record is a breaking change. If the calling library isn't able to describe the type anymore (except via typedef) because of a private field, does that make it so that some changes can be made to the record shape without breaking calling code?

natebosch avatar Sep 20 '22 17:09 natebosch

The first private field would be breaking, because it breaks a record pattern. But changing a non-empty set of private fields to a different non-empty set would not be breaking.

var (a, b) = foo(); // Breaks when return type changes from `(int, int)` to `(int, int, {T _x})`.

var (a, b, ...) = bar(); // Change of return type `(int, int, {T _x})` to `(int, int, {S _y})` is not breaking.

So the part that's worse than classes is that adding the first private field is breaking.

eernstg avatar Sep 20 '22 18:09 eernstg

I think also expanding the set of public fields is non-breaking, assuming the record also has at least one private field. Right?

natebosch avatar Sep 20 '22 18:09 natebosch

I think also expanding the set of public fields is non-breaking, assuming the record also has at least one private field. Right?

Right. Once a record has a single private field, you give up the ability to destructure it or construct it from a record expression, which are the places that break when changing the record's shape.

I'm not sure if adding support for record update and/or spreading would change the answer here.

munificent avatar Sep 20 '22 18:09 munificent

If we add "rest patterns" (as Erik's example above suggested, and which we should do at some point in the future), then you can presumably deconstruct some of a record that you know the shape of, even if it uses private names:

var recordWithPrivateName = otherLibraryFunction(); // (int, int, {int _private@otherLibrary}) inferred
var (int x, int y, ...) = recordWithPrivateName;  // Destructure ignoring some (unspeakable) fields.

At that point, adding a public field to a record with a private field is no longer non-breaking. Adding a private field might be (as long as there is no other way to get to the old shape, say a different function from the same library which still uses the old shape, infer a variable with that type, and try to assign the new record to that variable).

lrhn avatar Sep 20 '22 20:09 lrhn

At that point, adding a public field to a record with a private field is no longer non-breaking

I didn't follow this, can you expand on why?

leafpetersen avatar Sep 20 '22 21:09 leafpetersen

At that point, adding a public field to a record with a private field is no longer non-breaking

I didn't follow this, can you expand on why?

I think he meant "no longer breaking".

It sounds like the rest of the language team is in favor of removing this restriction, so I'll go ahead and take it out.

munificent avatar Sep 20 '22 22:09 munificent

It sounds like the rest of the language team is in favor of removing this restriction, so I'll go ahead and take it out.

I want to push back against private field names for the initial release. We have a compelling feature without them, we can add them later, and they complicate the implementation.

Private field names introduces a new thing: a private name used in the interior of a type term that is not just another type, and seems like something that cannot be alpha-renamed to a public name. This requires extra design in the web compilers which use a string-heavy representation of many things in the type system, including the expression of any type. It might end up being easy to do this, but I feel like we are adding a feature when we have been careful to take away as much as possible in order to get the MVP out.

rakudrama avatar Sep 20 '22 22:09 rakudrama

This requires extra design in the web compilers which use a string-heavy representation of many things in the type system, including the expression of any type.

@leafpetersen @eernstg @lrhn, does this change your opinion on allowing them?

munificent avatar Sep 21 '22 00:09 munificent