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

Record "construction" syntax for typedef #2528

Closed
srawlins opened this issue Sep 28, 2022 · 11 comments
Closed

Record "construction" syntax for typedef #2528

srawlins opened this issue Sep 28, 2022 · 11 comments
Labels
feature Proposed language feature that solves one or more problems records Issues related to records.

Comments

@srawlins
Copy link
Member

srawlins commented Sep 28, 2022

Don't you hate it when you are excited to use Records and try them out:

List<({int height, int width})> gatherPoints() {
  var first = (height: 1, width: 2);
  var second = (heighth: 3, width: 4);
  var more = [
    (height: 5, width: 6),
    (hieght: 7, width: 8),
  ];
  return [first, second, ...more];
}

And you're greeted (theoretically) with a static error that a List<Record> is returned, but a List<({int height, int width})> was expected! Where did you go wrong? What do you change? What a mess!

You may be quick to see my errors here, but imagine a more complicated example, maybe more lines, more inference, type promotion, return values, lots of var this and final that. Perhaps you have just recently changed the name of one field in the record type in the return type, and you're working on cleaning up the record literals.

There has to be a better way!

What if we introduced a record "construction" syntax with a typedef, similar to said support for interface types:

typedef MyList = List<int>;

var x = MyList.of([1,2,3]); // Ooooooh, love!

typedef Rectangle = ({int height, int width});

List<Rectangle> gatherPoints() {
  var first = Rectangle(height: 1, width: 2);
  var second = Rectangle(heighth: 3, width: 4); // 1
  var more = [
    Rectangle(height: 5, width: 6),
    Rectangle(hieght: 7, width: 8), // 2
  ];
  return [first, second, ...more];
}

Ah ha, now I get static errors at // 1 and at // 2, pointing out my spelling mistakes.

I know that my Ron Popeil pitch has persuaded you :). You won't regret it!

Yes I only got the benefit of matching a Rectangle's fields with those in the literals because I took the time to spell out Rectangle, begrudgingly forced to program with characters in the main stretch of the keyboard, but such a small price. And Yes this could be similarly solved by requiring types everywhere (like Rectangle second = Rectangle(heighth: 3, width: 4); or var more = <Rectangle>[), but, no thank you.

@srawlins srawlins added feature Proposed language feature that solves one or more problems records Issues related to records. labels Sep 28, 2022
@TimWhiting
Copy link

TimWhiting commented Sep 28, 2022

Is there something that could be improved for the error message? At least for collection literals?

Such as:

The context type of [first, second, ...more] is List<({int height, int width})>, 
  but second has type ({int heighth, int width}},
  and more has type List<Record>

If the expected context type then could be propagated to more, and a similar error shown for case 2, that would be great.

In other words, it seems like static analysis correctness should use forward flow analysis, but static analysis error reporting should do reverse flow analysis as much as it can.

This would improve static analysis error reporting in general and not just for records.

@srawlins
Copy link
Member Author

Is there something that could be improved for the error message? At least for collection literals?

I think for collection literals, yes.

@lrhn
Copy link
Member

lrhn commented Sep 28, 2022

As you say, that's just:

typedef MyList = List<int>;

var x = MyList.of([1,2,3]); // Ooooooh, love!

typedef Rectangle = ({int height, int width});

List<Rectangle> gatherPoints() {
  Rectangle first = (height: 1, width: 2);
  Rectangle second = (heighth: 3, width: 4); // 1
  var more = <Rectangle>[
    (height: 5, width: 6),
    (hieght: 7, width: 8), // 2
  ];
  return [first, second, ...more];
}

with no new syntax needed.

I don't get the "but no thank you" to that.
It's less characters than what you suggest.

@srawlins
Copy link
Member Author

I don't get the "but no thank you" to that.

I just mean "no thank you" in that I wouldn't want to write type annotations that violate Effective Dart like https://dart.dev/guides/language/effective-dart/design#dont-write-type-arguments-on-generic-invocations-that-are-inferred and https://dart.dev/guides/language/effective-dart/design#dont-redundantly-type-annotate-initialized-local-variables.

I don't live to serve Effective Dart (or rather, I say that I don't live to serve Effective Dart), and it should of course serve me. Maybe writing the type annotations would just be an exception I tack on in my mind, if I want stronger protection against field name mismatches.

It's less characters than what you suggest.

I'm definitely not aiming for less characters with this proposal.

@lrhn
Copy link
Member

lrhn commented Sep 29, 2022

It's actually an interesting example of why types are not necessarily unnecessary, even if they are redundant.

Redundancy is used for validation. If the two things don't match up, then something is wrong. If they do, everything is fine.
If there is something wrong, you fix it until it's right.
Then arguing that "since they match up, the type is unnecessary" is missing the point that the type is only unnecessary now because it has already done its job. (And might do one again it in the future.)

The Effective Dart rules ignores that and tells you to remove all redundancy. Maybe the problem is those rules. 😉

If you write var l = <int>[1];, you get told to remove the <int> because it's redundant. But it protects you from adding , 2.5 to the list.

Adding the ability to write Rectangle(height: 1, width: 2) is a redundancy. You should expect an Effective Dart rule to tell you to drop that Rectangle too, because it's unnecessary and redundant. At that point, it won't actually help you either.

Records are special.

For lists, you can write var l = <num>[1]; if you want something different from the otherwise inferred type. Then the list, and the variable, gets type List<num>.

For records, you cannot, because the record does not have a type separate from the types of its fields.
If you want the variable to have a type different from the record literal, say (int?, int?) p = (null, null);, you need to put the type on the left. (Or do var p = (null, null) as (int?, int?);, but please don't do that.)

I can see why putting the Rectangle on the right, on the literal, looks like it's doing the same thing as the type argument to list literals.
But it's more new syntax, achieving nothing that a type on the left cannot do, so I'd rather loosen the Effective Dart rules instead than having more syntax so you can do the same thing in more ways.

@eernstg
Copy link
Member

eernstg commented Sep 29, 2022

arguing that "since they match up, the type is unnecessary" is missing the point
that the type is only unnecessary now because it has already done its job

This is exactly the motivation for dart-lang/sdk#58773, which is a proposal to adjust the lint
omit_local_variable_types such that it only flags the locations where the type can immediately be determined by taking a look at the initializing expression.

That concept is given a technical interpretation by categorizing certain expressions as having a obvious type (e.g., 42 has an obvious type, and <int>[] has an obvious type, but foo(someExpression) and [1, 1.5] do not). All we need to do is to include record literals in this definition, for example: 'A record literal has an obvious type if it does not contain any named fields, and the value of each field has an obvious type'.

We may or may not want to update the corresponding Effective Dart rule: It can certainly be argued that a type annotation is not 'redundant' if it provides actual help to the author of the code (e.g., to avoid those typos), and later on for every reader or maintainer of the code (to understand what the type is, in order to understand other parts of the code).

@srawlins
Copy link
Member Author

If you write var l = <int>[1];, you get told to remove the <int> because it's redundant. But it protects you from adding , 2.5 to the list.

Yeah I've never been able to square this circle. I don't like the <int>, but I know I've seen it prevent bugs, especially in long lists of > 10 items, where some are spread. When you add another item, if you forget to spread it, then there is no static error, unless you wrote an explicit type argument.

OK I agree this isn't super duper worth a new syntax, and we can see what arises when people start using records, and then use them long enough that they start wanting IDE refactoring support for their record types and record literals.

@leafpetersen
Copy link
Member

Just to follow up briefly, since I don't think I saw this option mentioned above. You can get essentially exactly what you want already as follows:

typedef MyList = List<int>;

var x = MyList.of([1,2,3]); // Ooooooh, love!

({int height, int width}) rectangle({required int height, required int width}) => (height : height, width : width);

List<Rectangle> gatherPoints() {
  var first = rectangle(height: 1, width: 2);
  var second = rectangle(heighth: 3, width: 4); // 1
  var more = [
    rectangle(height: 5, width: 6),
    rectangle(hieght: 7, width: 8), // 2
  ];
  return [first, second, ...more];
}

Ever so slightly more verbose at the definition site, but maybe not too bad?

@srawlins
Copy link
Member Author

Yeah that's not too bad!

@munificent
Copy link
Member

I also don't think we want to do anything special here.

Taking a step back, records are introducing a structural type to Dart. Structural types are well known for making errors less obvious. In the example here, a big part of the problem is that you've got a couple of different record types flowing into a least upper bound and you get Record out. LUB is also well-known for making error messages worse.

So, yes, if you start tossing a bunch of records in places where LUB happens, it's possible to get pretty opaque errors. My hope is that this won't be a huge problem in practice, for a combination of reasons:

  1. While I'm excited about records, I hope that most Dart users continue to use nominal types most of the time. Nominal types are great. We should add things like primary constructors to make it really easy to define new nominal types, even easier than it already is with this. parameters.
  2. I hope we can do something clever in our error reporting where if we see that a LUB of two distinct record types has occurred, we track those two original types. If the resulting Record type ends up appearing in an error message, we can point back to the field mismatches that caused us to move up to Record.
  3. Longer term, it would be really interesting to explore trying to move away from LUB in general. I suspect it rarely does what you want.

@leafpetersen
Copy link
Member

This discussion sparked a thought, which I filed an issue on here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems records Issues related to records.
Projects
None yet
Development

No branches or pull requests

6 participants