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

Should Object methods be callable on nullable types? #175

Closed
Tracked by #110
leafpetersen opened this issue Jan 11, 2019 · 21 comments
Closed
Tracked by #110

Should Object methods be callable on nullable types? #175

leafpetersen opened this issue Jan 11, 2019 · 21 comments
Assignees
Labels
nnbd NNBD related issues

Comments

@leafpetersen
Copy link
Member

In NNBD code, is it valid to call methods from Object on nullable types?

int? i = null;
print(i.hashCode);
@leafpetersen
Copy link
Member Author

Note that if not, you're not able to call Object methods on generic parameters that are not known to be non-nullable.

Also, I'm pretty sure we want to allow string interpolation to interpolate null values. So we'd have:

int? i = null;
print(i.toString()); // Error
print("$i"); // Ok

@eernstg
Copy link
Member

eernstg commented Jan 13, 2019

There's an "under the radar rule" that a receiver whose static type is a union type supports access to members that are shared by all summands of that union type. With that, we'd say "yes".

@munificent
Copy link
Member

If we answer "no" to this, then null really isn't an object (lowercase), for any useful definition of the term.

I believe the answer should be "yes". It's useful in practice, makes interpolation consistent with toString(), and lets you use null in maps and sets without needing weird special case handling.

It ain't broke now, so let's not "fix" it.

@matanlurey
Copy link
Contributor

Drive-by comment:

Making Object's methods on Object? not callable will really hurt usability. For example:

class V {
  final String? name;
  const V(this.name);
  int get hashCode => name.hashCode;
  bool operator==(Object o) => o is V && name == o.name;
}

Both .hashCode and .== need to exist on a nullable type or this code is now invalid :-/

@leafpetersen leafpetersen self-assigned this Jan 17, 2019
@leafpetersen
Copy link
Member Author

Decided: yes, they can. @leafpetersen to write this up.

@leafpetersen leafpetersen added the nnbd NNBD related issues label Jan 23, 2019
@gbaldeck
Copy link

gbaldeck commented Jan 24, 2019

So, playing devil's advocate here, if you have the following code in Flutter:

String _initalValueForTextFormField() { //Notice return type is String not String? so the return value should not be nullable
	int? initialvalue = SomeService.getInitialValue();
	return initialvalue.toString(); //issue in question on this line
}

Shouldn't the return statement above give an error so that the end user doesn't see null in the TextFormField? The error message may be something like:

initialvalue could be null so use the !! operator or check to make sure its not null

I used Kotlin's not-null assertion operator (!!) there, not saying you guys will decide on the same thing.

So valid code would end up being this:

String _initalValueForTextFormField() { 
	int? initialvalue = SomeService.getInitialValue();
	return initialvalue!!.toString(); // added the !! operator
}

Or this:

String _initalValueForTextFormField() { 
	int? initialvalue = SomeService.getInitialValue();
	return initialvalue?.toString() ?? "It was null!"; //added null check
}

@gbaldeck
Copy link

gbaldeck commented Jan 24, 2019

There's an "under the radar rule" that a receiver whose static type is a union type supports access to members that are shared by all summands of that union type. With that, we'd say "yes".

@eernstg Doesn't this go against what we are trying to achieve with nullable types? Say we have a custom class C that is unioned with null like below:

C? item = C();

And say that C? can use all the methods and properties that C has without having to use the .? operator, assert not-null operator (!! in kotlin), or do a null check, like this:

item.methodOnC()

Then what is the point of having the nullable types at all? Perhaps I'm not interpreting your statement correctly so please let me know if I'm not.

@munificent
Copy link
Member

Shouldn't the return statement above give an error so that the end user doesn't see null in the TextFormField?

If we assume the return type of toString() is String and not String?, then, no, the code is fine. toString() won't return null, regardless of what you call it on. Calling toString() on null doesn't return null, it returns the four-letter string "null".

And say that C? can use all the methods and properties that C has without having to use the

That's not what he's saying. "shared by all summands" means every branch of the union has to have a member before you can access it, including the Null branch. Since Null has very few members (just toString(), hashCode, etc.), then a nullable type effectively filters out all methods except those that are available on Object. Those are exactly the methods you can call on null safely, so it does what you want.

@gbaldeck
Copy link

gbaldeck commented Jan 25, 2019

If we assume the return type of toString() is String and not String?, then, no, the code is fine. toString() won't return null, regardless of what you call it on. Calling toString() on null doesn't return null, it returns the four-letter string "null".

Right, I understand it returns the four letter word "null". Which is what I am arguing should not be allowed. If you look at the valid code blocks I posted, the first one throws an NPE because of the !! operator and the second one never returns the four letter word "null". It either returns the initalvalue string or returns "It was null!".

So while in theory, sure, letting null have the same functions as Object is fine, since in the example I posted the four-letter string "null" is technically not null. But in practice that example would end up returning string "null" to the user. That defeats one of the purposes of having nullable types in my opinion. When would you ever want the user to see the string "null"?

What I'm suggesting is that at the return statement the programmer is given some sort of warning, error, etc. saying "Hey, this value could be null. Either mark it as a possible NPE or do a null check."

That's not what he's saying. "shared by all summands" means every branch of the union has to have a member before you can access it, including the Null branch. Since Null has very few members (just toString(), hashCode, etc.), then a nullable type effectively filters out all methods except those that are available on Object. Those are exactly the methods you can call on null safely, so it does what you want.

Ah yes. Thank you for the explanation. Haven't had discrete math in almost a decade so I need reminders from time to time.

@leafpetersen
Copy link
Member Author

There are definitely examples of places where one might prefer not to silently be allowed to call Object methods on null, even though they are in fact present. But in thinking through examples, there also seemed to be many more places where it would be really painful not to be able to do so. If you have a nullable type, then you often (though not always) intend for null to be a meaningful object there, usually indicating absence. Think json, or a property that is not always present. If printing out such an object, or computing its hash code, requires special casing null on each nullable field, it rapidly gets really ugly. Note that you'd have to do this in generic code that wasn't restricted to non-nullable types:

class A<T> { 
  T x;
  String toString() {
    String xAsString = x?.toString() ?? "null";  
    return "A{x = $xAsString}";
  }

Overall, this felt like very awkward for fairly limited benefit. Yes, we'd help you catch the cases where you really didn't want to accidentally print out null, but this doesn't seem, to me at least, to justify the cost.

FWIW, Kotlin seems to make the same choice - it allows calling Object methods on nullable types.

@gbaldeck
Copy link

I see your point, and I'm probably not thinking through all the cases where my suggestion would be a problem (I'm definitely not). Two language teams, probably the Swift team too, have come to the same conclusion so it must be the way to go.

@leafpetersen
Copy link
Member Author

Interestingly, as best I can tell Swift objects have no common base class with a common set of methods, and Any and AnyObject don't seem to have any methods that I could find (admittedly on limited amount of poking, but this makes sense given their purpose). So it may simply not come up there - any Swift experts around that can confirm or deny this?

@eernstg
Copy link
Member

eernstg commented Jan 25, 2019

@gbaldeck wrote:

And say that C? can use all the methods and properties that C has without having to use the .?

@munificent already mentioned why this only allows for using members of Object, but I'd like to mention a conceptual reason for doing it in that manner as well:

Let e be an expression whose type is a union of some types T1 | .. | Tk. Let's just consider class types (and ignore function types and oddballs like dynamic).

If we go directly to a shared superinterface of all those class interfaces then we'll find an interface where every member is inherited by each Tj, and they'll be associated with the same dartdoc comment etc.: They mean the same thing.

In contrast, if we allow invoking e.foo(...) just because each one of those Tj interfaces has a foo, and each of them permits this particular invocation, then we could actually come up with a sound type for the expression, and presumably it "would just work" (with a certain run-time overhead), but it would be much harder for a developer who reads this code to maintain an understanding of what the invocation actually does, both at the conceptual and at the technical level. So that's the reason why we generally do not want to give a union-typed receiver anything other than the set of members that are "shared by all summands".

@gbaldeck
Copy link

gbaldeck commented Jan 25, 2019

@leafpetersen That's very interesting that they don't have a shared Object class. I just looked up how toString() is handled by Swift and every class that isn't a shipped type that needs a string representation must implement an interface that requires the description property to be defined.

IMO, despite the union rules between types I feel that Dart should make nulls a special case and not allow the Object properties and methods, like toString(), on null. But I understand you all's choice.

@eernstg
Copy link
Member

eernstg commented Feb 11, 2019

At this point I suspect that we can make a decision: Yes, methods declared on the top type (Object or Object?, cf. #141) can be invoked on a receiver whose type is nullable. @lrhn and @leafpetersen, do you agree?

Note that one case which is affected by this (cf. this comment) is receivers of type X, where X is a type variable with a bound which is a nullable type.

Presumably, the breakage would be far too massive if we were to decide that a type variable with no explicit bound has the bound "non-null object" (spelled Object! or Object, cf. #141 again), so this case would include every usage of a receiver whose type is a type variable with no declared bound.

@lrhn
Copy link
Member

lrhn commented Feb 11, 2019

Agree. We have to allow == on nullable types, if for nothing else then to do == null. It's going to get confusing if we allow if (x == null) ... but not var n = null; if (x == n) .... I don't want to be the one to explain that.
Also generics, if you ask for a HashMap<int?, int>, then it's going to be very inconvenient if you can't call .hashCode on the keys.

If we had designed Dart from the start so that null was a booby-trap, any use of it would throw, then there wouldn't be code happily using null as an Object. Breaking that code now is not worth it, and it will complicate some existing use-cases.

We might have to introduce a synthetic and un-namable top interface type containing the Object methods, so that both Null and Object implement that interface, in order to simplify the specification. All Dart objects will then have those methods, not just Objects.

@leafpetersen
Copy link
Member Author

At this point I suspect that we can make a decision:

We did so a month ago... :)

@leafpetersen
Copy link
Member Author

I've been keeping the decided issues open as reminders, but maybe that's too confusing: they're linked anyway from the header of the main issue. I'll go ahead and close out the decided issues.

@gbaldeck
Copy link

gbaldeck commented May 3, 2019

I know you all have decided on this, and I'm not trying to change your mind. But I've thought of a different solution to the problem I mentioned in this thread a month or two back.

Refresher on the problem:

Dart is evolving as a multipurpose language with an emphasis on the front end. With the addition of non-null by default, Dart provides a great tool to catch null values before they can cause a problem.

In Flutter the toString() method is used a lot in the build() function and many other places. I use it constantly and provide an example below.

By allowing toString() to be called on a nullable variable without having to check if the variable is null, Dart misses the opportunity of providing a great way to stop "null" from being seen on the user interface.

Here is an example:

class PaymentPage extends StatefulWidget {

  double? paymentAmount; //nullable

  PaymentPage(Service service) {
    paymentAmount = service.serverCallToRetrieveAmountDue(); //call to server, could return null
  }

  @override
  Widget build(BuildContext context) {
    return Form(
      key: _formKey,
      child: TextFormField(
        initialValue: paymentAmount.toString(), //null check not required, the user sees null if paymentAmount is null
        decoration: const InputDecoration(
          labelText: 'Amount to Pay',
        ),
        keyboardType: TextInputType.number,
        validator: (value) {
          //validator logic
        },
        onSaved: (value) => paymentAmount = double.parse(value),
      ),
    );
  }
}

In the example above, if paymentAmount is null, the user will see null.

Rather than disallowing the Object methods to be called on nullable types (which is what I was arguing for before), I propose that an analyzer option be added under the "errors" category that will notify the programmer of toString() being used on a nullable variable. Then the programmer can set its importance through the warning, error, and ignore options.

analyzer:
  errors:
    tostring_on_nullable: error

Since the programmer is reminded to do a null check when using toString() on a nullable variable, the code will become:

class PaymentPage extends StatefulWidget {
  double? paymentAmount; //nullable

  PaymentPage(Service service) {
    paymentAmount = service.serverCallToRetrieveAmountDue(); //call to server, could return null
  }

  //This build method should only be run once on app startup
  @override
  Widget build(BuildContext context) {
    return Form(
      key: _formKey,
      child: TextFormField(
        initialValue: paymentAmount?.toString() ?? "0.00", //null check now required because of analyzer option
        decoration: const InputDecoration(
          labelText: 'Amount to Pay',
        ),
        keyboardType: TextInputType.number,
        validator: (value) {
          //validator logic
        },
        onSaved: (value) => paymentAmount = double.parse(value),
      ),
    );
  }
}

This will help a lot in catching a null value before it is seen by the user, which goes along with the benefits of NNBD.

Also, the analyzer option could just as easily be changed to warning so that toString() on a nullable variable can still be used in other places where it might be needed.

@leafpetersen
Copy link
Member Author

Also, the analyzer option could just as easily be changed to warning so that toString() on a nullable variable can still be used in other places where it might be needed.

There's a pretty strong and rich set of lints available from the analyzer (and Flutter makes good use of the linting ability), and this seems to me to be a good candidate for a lint.

@gbaldeck
Copy link

gbaldeck commented May 8, 2019

@leafpetersen A linter option sounds good, just something to remind the programmer that their toString() call on a nullable variable may not be safe. Is there somewhere I can put in a request for this? I would also like to help out if I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nnbd NNBD related issues
Projects
None yet
Development

No branches or pull requests

6 participants