consider pointing to required keyword when reporting errors for non-nullable named parameter without default value

[mraleph]

class X {
  final String y;
  X({this.y});
}

produces in analyzer

Analyzing /tmp/test.dart...
  error • The parameter 'y' can't have a value of 'null' because of its type, and no non-null default value is provided. • /tmp/test.dart:3:11 • missing_default_value_for_parameter
1 error found.

This error message seems to indicate that the only way to address this error is to provide non-null default value. However this is not true - you can either provide a default value or use required keyword. I think this error message in both CFE and analyzer should be ammended to list both alternatives to be more user friendly.

This report is based on user report from Reddit

I think this also indicates that we might want to go through all error messages added in NNBD release and look at them from the perspective of UX and whether or not they are immediately actionable and clear even for the users who are encountering them for the first time. /cc @leafpetersen

/cc @stereotype441 @johnniwinther

[Rudiksz]

I understand the @required parameter, but I was talking about optional parameters.

I have a lot of classes with the following code

class X {
  final Y y = Y();
  X({this.y});
}

The issue is actually an assumption I had about Dart in general which turns out to be wrong. I just assumed that if Y is not set in the constructor, then Dart would not bind it to the field and the field would keep the initialized value. That's not the case, Dart will bind null to the value and the non-nullable Dart just surfaced the issue with the error message you posted.

With nnbd I would have to do as below, but that's often not possible because Y does not have a const constructor, and often it can't have one.

class X {
  final Y y = Y();
  X({this.y = Y()});
}

Which means that with NNBD my classes can't have optional constructor parameters, or most of my fields (everything that is not a basic type) have to be nullable.

Or I have to rely on hacks like below, to get something that resembles non-nullable fields:

  AppTabBase({this.icon, @required this.title, this.pages}) {
    title ??= '';
    pages ??= <Widget>[].asObservable();
  }

Which is a big step backwards, and it makes migrating to nnbd quite difficult.

[eernstg]

@Rudiksz, your topic may be a separate issue, but I'll comment here because this comment might suffice. Consider the following approach:

class B ...
class A {
  final B b;
  X({B? b}): this.b = b ?? B();
}

This will allow b to be an optional parameter and B to be non-const, and it will ensure that this.b gets the value when a non-null argument is passed, and otherwise a fresh B is used.

[Rudiksz]

Thanks for the tip.

X({B? b}): this.b = b ?? B();

This is pretty much what I assumed Dart does under the hood, but apparently I have to do it manually, null-safety or not.
I will eventually migrate my data classes to this syntax, if there's no better support from the language but it feels very cumbersome.

[eernstg]

A request for more flexible default values has been made earlier, for instance, dart-lang/language#140.

PS: The topic of this issue is still the error message, as described by @mraleph here. ;-)

[bwilkerson]

On the topic of error messages: I agree that the message could be improved, and I'll look into doing so. I'd also welcome input from the language team, and anyone else that's interested, to help identify other messages that could be improved, either with or without suggestions for how to improve them.

But I do want it to be clear that the analyzer's messages have intentionally been split into two pieces in order to allow IDEs more control over the presentation of those messages. The first is the problem message--a description of the problem that was discovered--and the second is the correction message--a description of possible ways to resolve the problem. In this particular case, the correction message is

Try adding either a default value or the 'required' modifier.

which I think addresses at least part of your concern.

At the moment the command-line analyzer only displays the correction message if the --verbose flag is provided. I think we should strongly consider always displaying the correction message. I also think we should strongly consider providing the rest of the information we have for diagnostics that is currently behind the --verbose flag, such as the context messages (pointing the user to other relevant locations in the code) and the URI of the documentation that further explains why the diagnostic is produced and possible ways to address the issue.

@devoncarew

[bwilkerson]

Proposed change in https://dart-review.googlesource.com/c/sdk/+/174300.

[mit-mit]

At the moment the command-line analyzer only displays the correction message if the --verbose flag is provided. I think we should strongly consider always displaying the correction message

@bwilkerson I agree with that. Do we have an issue tracking that change?

[bwilkerson]

Not that I'm aware of, no.

[devoncarew]

At the moment the command-line analyzer only displays the correction message if the --verbose flag is provided. I think we should strongly consider always displaying the correction message.

Catching up on this thread; I think this does make sense - always showing the correction message.

Separately, I think it would be worth it to do a brief audit of the exiting correction messages to ensure that they do give the user non-trivial advice (isn't something the user could know just by reading the error message or the code).

[mit-mit]

Adjusting priority: this is going to be very, very common for null safety, so I think this is a P1, not a P3!

[bwilkerson]

Note that the message for the diagnostic called out in this issue has been updated by the aforementioned CL.

[devoncarew]

Given the work that Brian's already landed for this, and the tweak to the dart analyze output that's in progress (https://dart-review.googlesource.com/c/sdk/+/174564), I think we can close this issue.

[johnniwinther]

I've created https://dart-review.googlesource.com/c/sdk/+/174800 for the CFE.