I could see how TheType could be a nullable property. Maybe we should make it explicit as TheType?? I think it's a good example to have the assertion in the setter body.

In this example _theProperty is non-nullable, but you're right that the property could just as easily have been created as a nullable one.

However, I don't think it's good to have the assertion in the setter body; based on how this example is currently written, there's no way for the non-nullable value to be null.

Yes. What I was suggesting was that you update this to be an explicitly nullable type to remove the ambiguity and keep the assertion.

Okay, so then it would look like this:

TheType? get theProperty => _theProperty;
TheType? _theProperty;
void set theProperty(TheType? value) {
  assert(value != null);
  if (_theProperty == value) {
    return;
  }
  _theProperty = value;
  markNeedsWhatever(); // the method to mark the object dirty
}

This would work, though it has the downside of being a bit more verbose. (In my opinion, being concise & readable is even more important in the style guide than anywhere else in this repo.)

Is there a downside to how I have it currently?

Similar to the other example, I would consider the intention of the guidance here. This is about providing guidelines for performing dirty checks in setters. Does this change improve on that guidance? IMO, I don't think verbosity is a bad thing in docs meant to educate for working on the framework.

IMO, I don't think verbosity is a bad thing in docs meant to educate for working on the framework.

I suppose we could agree to disagree here. There's a good reason "TL;DR" is so prevalent throughout the internet: if something is more verbose, the opportunity cost of reading & understanding is higher, ergo people become less likely to do it. In the case of this example, removing irrelevant code is a straight improvement IMO.

I really appreciate the time you've spent helping me out here. Since this PR would require two approvals to merge, maybe now would be a good time for another reviewer to step in with a fresh perspective.

There's a good reason "TL;DR" is so prevalent throughout the internet: if something is more verbose, the opportunity cost of reading & understanding is higher, ergo people become less likely to do it. In the case of this example, removing irrelevant code is a straight improvement IMO.

I am not sure this is really relevant. This is a change specifically within the context of the Flutter org. Not the internet at large. :)

I will leave this to another reviewer to make a pass. Honestly, I don't know this change is worth this much debate.

I agree that this example should change one way or another, because the assertion is no longer necessary and could confuse people. Also, the assertion is not important to this section of the style guide. My interpretation of the two options are:

Option 1:

TheType _theProperty;
void set theProperty(TheType value) {
-   assert(value != null);
  if (_theProperty == value) {
    return;
  }
  _theProperty = value;
  markNeedsWhatever(); // the method to mark the object dirty
}

Option 2:

-TheType _theProperty;
+TheType? _theProperty;
-void set theProperty(TheType value) {
+void set theProperty(TheType? value) {
  assert(value != null);
  if (_theProperty == value) {
    return;
  }
  _theProperty = value;
  markNeedsWhatever(); // the method to mark the object dirty
}

I like option 1. Option 2 is oddly specific and could distract from the point of this section. You might do something like it if _theProperty was supposed to be null during initialization or something and then never nonnull again... but then why assert instead of making the setter nonnullable? These are questions that the reader shouldn't be thinking about :)

These could also be nullable, the sample code here does not provide enough information to determine if they are or not. IMO, I think this one is ok as is.

I disagree here as well: the best practice is to avoid null-asserts in constructors.

class Foo {
  Foo({this.bar}) : assert(bar != null); // BAD

  Object? bar;
}

Let's say that bar is a nullable field but you still want the constructor argument to be non-nullable. You could do it like so:

class Foo {
  Foo({required Object this.bar}); // GOOD

  Object? bar;
}

It's much better for null-safety to be enforced by a type annotation than by using an assert, since the latter makes it much easier for mistakes to go unnoticed until runtime.

Right, I think we might be over-indexing on code that lacks enough context to actually suggest this.

Fair enough haha

But the point I was trying to make is: validating a constructor argument with an assert(bar != null) is never a good practice, so I'd like this example to be changed to reflect that.

I appreciate you want to make the code snippet as accurate as possible. Given that this code snippet is meant to facilitate guidance around documenting parameters, and not to illustrate best practices around unnecessary null-asserts, does this change improve on the intended guidance?

Yes it does!

Originally, this example documentation said:

  /// The [bar] argument must not be null.

At the time it was written, there wasn't a good way to enforce that a non-null argument is passed into a class constructor, so the best practice was to include this advice where applicable.

But thanks to null safety, this advice is no longer warranted: setting the class member as non-nullable means that static analysis will enforce it.

I think that assert(bar != null) should go since it is not best practice. I don't want to confuse users or LLMs that might read it.

I agree that this example should change one way or another, because the assertion is no longer necessary and could confuse people. Also, the assertion is not important to this section of the style guide. My interpretation of the two options are:

Option 1:

TheType _theProperty;
void set theProperty(TheType value) {
-   assert(value != null);
  if (_theProperty == value) {
    return;
  }
  _theProperty = value;
  markNeedsWhatever(); // the method to mark the object dirty
}

Option 2:

-TheType _theProperty;
+TheType? _theProperty;
-void set theProperty(TheType value) {
+void set theProperty(TheType? value) {
  assert(value != null);
  if (_theProperty == value) {
    return;
  }
  _theProperty = value;
  markNeedsWhatever(); // the method to mark the object dirty
}

I like option 1. Option 2 is oddly specific and could distract from the point of this section. You might do something like it if _theProperty was supposed to be null during initialization or something and then never nonnull again... but then why assert instead of making the setter nonnullable? These are questions that the reader shouldn't be thinking about :)

I think that assert(bar != null) should go since it is not best practice. I don't want to confuse users or LLMs that might read it.