Linter needs to warn about single-item records

[lukehutch]

For the following code:

abstract class X {
  Future<String?> getNameOrNull(
    String database,
    String table,
  );

  Future<String> getName(String database, String table) async {
    final name = (
      await getNameOrNull(
        database,
        table,
      ),                // (1)
    );
    if (name == null) {    // (2)
      throw Exception('Name is null');
    } else {
      return name;    // (3)
    }
  }
}

The line marked (2) has the warning The operand can't be null, so the condition is always 'false'.

The line marked (3) has the warning A value of type '(String?)' can't be returned from the method 'getName' because it has a return type of 'Future<String>'

The type of name is (String?), which looks deceptively like String?, but it is actually a single-item record, because of the stray comma on line (1)!

It took me a while to figure out what was going on here, because I assumed that the type (String?) was in fact the same as the type String?...

It is very easy to make this sort of mistake, because it is customary to put a trailing literally everywhere you can put one in Dart, especially in Flutter code, because then the formatter makes the structure of the code much clearer.

In my opinion, it shouldn't be possible to create single-item records. Actually, Dart wouldn't interpret (value) as a record. But apparently it does recognize (value,) as a record.

I think the best fix here would be to ignore the empty field at the end of a record definition that ends in a comma before the closing parenthesis, and proceed as if the comma weren't there. That would make (value,) act the same as (value). Although the downside of this is that before record syntax existed, (value,) would not have been valid syntax for a parenthesized expression...

Therefore, maybe the best thing to do here is simply give the user a warning if they try to declare a single-field record.

• Dart version and tooling diagnostic info (dart info):

$ dart info

If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

#### General info

- Dart 3.1.2 (stable) (Tue Sep 12 16:26:23 2023 +0000) on "linux_x64"
- on linux / Linux 6.4.12-200.fc38.x86_64 dart-lang/sdk#1 SMP PREEMPT_DYNAMIC Wed Aug 23 17:46:49 UTC 2023
- locale is en_US.utf8

#### Project info

- sdk constraint: '>=3.0.0 <4.0.0'
- dependencies: process_run
- dev_dependencies: lints
- elided dependencies: 2

#### Process info

|  Memory |  CPU | Elapsed time | Command line                                                                               |
| ------: | ---: | -----------: | ------------------------------------------------------------------------------------------ |
|  447 MB | 0.0% |   1-05:03:59 | dart --enable-vm-service=0 --pause_isolates_on_start --disable-dart-dev -DSILENT_VM_SERVICE=true --write-service-info=file:<path>/vm.json --pause_isolates_on_exit --enable-asserts <path>/main.dart |
|  680 MB | 0.0% |   6-21:13:05 | dart ..<path>/serverpod_cli.dart generate --watch                                          |
|  661 MB | 0.0% |   1-05:11:35 | dart ..<path>/serverpod_cli.dart generate --watch                                          |
|   93 MB | 0.0% |   1-05:03:59 | dart debug_adapter                                                                         |
|   72 MB | 0.0% |  29-08:17:43 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|   77 MB | 0.0% |  29-08:17:24 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|   77 MB | 0.0% |  29-07:48:17 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|   77 MB | 0.0% |  28-04:43:12 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|   76 MB | 0.0% |   1-05:33:23 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|  907 MB | 0.0% |   4-23:04:24 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.72.2            |
| 1281 MB | 0.2% |   1-05:33:23 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.72.2            |
|  105 MB | 0.0% |   1-05:33:23 | flutter_tools.snapshot daemon                                                              |

• Whether you are using Windows, macOS, or Linux (if applicable): Linux

[lrhn]

In my opinion, it shouldn't be possible to create single-item records

That should probably be "single positional item records". We also allow (x: 42), which is a single named field record.

I wouldn't remove it from the language, but we could remove it from the grammar, so you have to write Record.singleton(x) instead of (x,).
It'd be annoying (for the one use I have of them, but I'll probably manage).

We would not allow (x,) as an expression, just because we stop interpreting it as a record. Trailing commas are only allowed where a comma separated list is already possible. So it wouldn't make your code valid, it would just make it not compile, by telling you that a , isn't allowed where you wrote it.

Should it also affect record types and patterns?

Today you can do:

class Opt<T> { // extension type, soon.
  final (T,)? _opt;
  Opt.some(T value) : _opt = (value,);
  Opt.none() : _opt = null;
  bool get hasValue => _opt != null;
  T get value => switch(_opt) {(var v,) => v, null => throw StateError("No value")};
  T? get tryValue => _opt?.$1;
}

Even if I change (value,) to Record.singleton(value), it still uses the corresponding (T,) type and the (var v,) pattern.

We might have to introduce Record1<T> as a type constructor, just to handle that one case,
so you can write:

    final Record1<T>? _opt;
    ///...
      switch(_opt) {Record1($1: var v) => v, ...

Maybe it wouldn't be be too bad if you could generally write Rec(int, int, {int x}) for a record type, if you need to disambiguate.
For singleton positional records, it would be the only way to write it, instead of the forced , being the only way.
Still, it'd be having two ways to write the same thing, one being strictly longer than the other, but only required in one case.

That's like the .new for the "unnamed' constructor, which you can use everywhere, but nobody ever uses it except for tear-offs, where it's required. Orthogonality isn't a feature, if the non-orthogonal alternative is shorter.

Anyway. that's all unlikely to happen now, so marking as analyzer issue.
(Maybe just give a better error message if the user creates a singleton positional record, and seems to assume it should have its element type).

[lukehutch]

I'll let you handle any deeper language design questions, but at this point the main thing I would need as an end user is simply a linter warning if I try to create a single positional item record.

Again the main reason this is necessary is that when working in Dart, you start to train yourself to end basically everything with a comma, to get the formatter to work, and where a comma gives a syntax error (e.g. between } and ) after a list of named function parameters), then you take it out. It's an "innocent until proven guilty" type mindset: put a comma at the end of everything unless you can't put one there :-)

A more thoughtful approach (only putting commas in list contexts) is of course the solution, but the point is that muscle memory will usually override thoughtful approaches, and this issue is likely to come up a lot, for a lot of people.

[pq]

Thanks for the report and @lrhn for the thoughtful response! We've had conversations about this confusion before and I do think it would be nice to do something to help folks stay on the rails. Ideally we could address this with a better error message but also possibly with a more pedantic "avoid singleton positional records" lint.

fyi @dart-lang/language-team @bwilkerson

[srawlins]

Yeah I think changing:

A value of type '(String?)' can't be returned

to

A value of record type '(String?)' can't be returned

or similar, would have the biggest bang for our buck. A rule could be useful as well.

[bwilkerson]

Along those lines we might also consider using semantic highlighting. LSP doesn't have a "record" highlight type, but it does have a "struct", which might be close enough. If typing the trailing comma changed the color of the formerly parenthesized expression to the color of a record, that might be enough to draw attention to the problem.

[lukehutch]

A value of record type '(String?)' can't be returned

That doesn't cover all situations where the user should be alerted. In my case the return type of a lambda was (String?), and the mouseover info showed that as the type, but it didn't register in my brain that this was a record type. It gets even more hairy when a generic type parameter is constrained by type inference to be one of these single positional record types, because then the apparent weirdness that occurs whenever you try to use something constrained by this type parameter can occur far from the source of the problem.

I suggest at a minimum adding a comma to the display of record types that have just one positional field, since that is the only way to construct them anyway.

[bwilkerson]

... adding a comma to the display of record types that have just one positional field ...

+1

[munificent]

In my opinion, it shouldn't be possible to create single-item records.

We debated single-element (and zero-element) records heavily before deciding to support them. See dart-lang/language#1301 and dart-lang/language#2386. For me, the most important motivation for supporting them is that eventually we'd like to be able to support spreading records into argument lists (dart-lang/language#893). For that to work generally across all argument list shapes, we'll need to support records of a single positional element.

it is customary to put a trailing literally everywhere you can put one in Dart, especially in Flutter code

For what it's worth, you can't put a trailing comma in a parenthesized expression (which is why we were able to steal that syntax to mean "single-element record"), index operator, type parameter list, or type argument list.

Yeah I think changing:

A value of type '(String?)' can't be returned

to

A value of record type '(String?)' can't be returned

or similar, would have the biggest bang for our buck. A rule could be useful as well.

+1. Putting the trailing comma in the type wouldn't hurt either: "A value of record type '(String?,)' can't be returned".

Overall, I think it's worth having single-positional-element records. The next question is whether to have syntax for them. We debated that too. I agree with the issue here that the trailing comma can be a little confusing or error-prone, but it more or less seems to work OK. There are no perfect syntaxes, and it's certainly hard to design elegant ones on top of an existing widely-used language.

[lukehutch]

For what it's worth, you can't put a trailing comma in a parenthesized expression (which is why we able to steal that syntax to mean "single-element record", index operator, type parameter list, or type argument list.

Right, that's the whole reason I filed this bug report :-)

The issue is that if you're used to ending almost every line with a comma or semicolon, then it is really easy to accidentally slip one in where it's not supposed to be, and that can wreak havoc at locations far from the erroneous comma.

A similar issue (which I reported in another bug) is the erroneous lambda syntax () => { /* ... block ... */ } -- which I have accidentally typed many times, forgetting that => and {} are mutually exclusive in lambda notation. This parses just fine, but the "block" is actually expected to be in set builder notation. The errors you get from this appear utterly insane sometimes, and also the tooling (VS Code) can end up auto-"fixing" your code in ways that seem completely bizarre. There is no actual indication as to the source of the problem, and it's very hard to spot. I see the trailing comma for single-item positional records as falling in the same category as this.

+1. Putting the trailing comma in the type wouldn't hurt either: "A value of record type '(String?,)' can't be returned".

Right, this is exactly what I proposed in my last comment.

I don't see any downside to adding the trailing comma to the Type.toString result for single type records, since it would bring the Type.toString output in line with the syntax needed to actually construct one of these records.

It's probably only a 2-line change, too, along with some changes or additions to tests.

[oprypin]

Another high-value improvement might be to change the formatter to always re-format single-value records

(
    foo,
)

to

(foo,)

The behavior would be consistent with the behavior as if the comma wasn't there, even for multi-line contents.

final x = (
  max(
    1,
    2,
  ),
);

final x = (max(
  1,
  2,
),);

[lukehutch]

Another high-value improvement might be to change the formatter to always re-format single-value records

This seems like a good move, but i in a weird way it doesn't address the cognitive issue here, where the comma got added by force of habit, precisely because people are used to putting a comma at the end of everything, to force an indent/outdent. Seeing one's code suddenly indented by the formatter might make sense on the surface, because your brain already decided that adding a comma was a good idea, and every time you do that, the indenter takes this action. There would be no surprise factor that would alert the user that you just did something erroneous.

An editor could add a "ghost label" (not sure what these are called -- text that appears at the end of a line in VS Code) that says // Single-element record or something...