Sdk: Improve tryParse null handling

[Rockvole]

It would be better if tryParse() dealt with null better. I think it is a common requirement.

int.tryParse(null) ?? 0;

Unhandled exception:
Invalid argument(s): The source must not be null
#0      int.tryParse (dart:core-patch/dart:core/integers_patch.dart:112)
#1      main (file:///work/projects/dart/kbml_parser/main_test.dart:11:7)
#2      _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:279)
#3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:165)

dart --version
Dart VM version: 2.0.0-dev.58.0 (Unknown timestamp) on "linux_x64"

[Rockvole]

There have been discussions about this and it seems like null handling was going to be added to tryParse() from the get-go.

https://groups.google.com/a/dartlang.org/forum/#!topic/core-dev/VmAeXXvg3so

https://github.com/dart-lang/sdk/issues/16319

[Rockvole]

This is my working code :

int i=cellAttributeMap["colspan"]!=null?int.tryParse(cellAttributeMap["colspan"]):null;

I thought tryParse would allow :

int i=int.tryParse(cellAttributeMap["colspan"]);

Setting i to be null when cellAttributeMap["colspan"] is null.

[lrhn]

I don't think we intended tryParse to accept a null argument. A null value is not the same thing as an empty string, and treating it as such is likely to hide errors rather than help you find them. If we had non-nullable types, the argument to both parse and tryParse would be a non-nullable string because there is no reason to pass null - it's easily detectable up-front and just accepting it quietly is dangerous.

The case where tryParse has better null-behavior than parse is that it returns a nullable result, which makes the orElse/onError argument unnecessary. If the string fails to parse, which is something you can't really check any faster than parsing it, then the function returns null.

[Rockvole]

Oh I see now, I can do :

int i=int.tryParse(cellAttributeMap["colspan"]??"");

To pass null through - which is concise enough.

[Rockvole]

Probably would be nicer to throw an ArgumentError or a NullError

[lrhn]

The parse function currently throws an ArgumentError on a null argument, and we plan to keep it that way, so we won't be changing anything wrt. this.

[Rockvole]

Ok, I had no idea I could catch that with an ArgumentException
https://github.com/dart-lang/sdk/issues/33275

[joe776]

Could the documentation then please be improved? Right now it reads "... returns null instead of throwing for invalid inputs." However, in the case of the invalid input null, it still throws, which is a pitfall I learned the hard way and through finding this issue.

[natebosch]

Allowing null could improve the UX since it gives you one place to default instead of 2.

Consider an input expression that may be null _or_ not parseable.

// If we allow null argument
var result = int.tryParse(potentiallyNull()) ?? 0;

vs

var input = potentiallyNull();
var result = input == null ? 0 : int.tryParse(input) ?? 0;

or

var result = int.tryParse(potentiallyNull() ?? '0') ?? 0;

I can see the argument that a null input is definitely _different_ from a non-parseable input, but it's also annoying to need to express the default multiple times.

In either case I'll reopen the issue to either revisit this decision, or to fix the docs.

[lrhn]

When moving to NNBD, the parameter will definitely be non-nullable.
There is no reason to pass null to the function, it is known to not work, so the type will reflect that. We do not accept null now, and we treat it as an argument error, exactly because this is how the parameter is intended - as a non-nullable parameter. (This will also make the documentation issue moot).

It is not the job of leaf functions to handle null-ness for you. We could allow every function to take null as every argument, and return null if any of them could not be handled. That is not what those functions are about, though. They either accept null and do something useful with it, or they do not accept null and make it an error.
Accepting null where it isn't intended is a sure-fire way to mask or do something weird on unexpected and erroneous nulls.

The documentation has been updated since this issue was filed and/or closed. It currently state that:

The [source] must be a non-empty sequence of base-[radix] digits,
optionally prefixed with a minus or plus sign ('-' or '+').
It must not be `null`.

Whenever documentation says "must not" something, you can be fairly certain it will throw an Error if something happens.

The onError parameter has been deprecated and is no longer documented as doing anything. You are directed to using tryParse.

[natebosch]

It is not the job of leaf functions to handle null-ness for you.

IMO the functions that tend to sit near boundaries of external input should have different considerations. Adding explicit and separate null handling adds noise. I suspect that int.tryParse is most likely to be used in the same code as that noise, and less likely to be used in deeper code which would already have nullness handled with NNBD argument types.

I tried looking for comparisons in other languages. The most common 2 I found are C# which has Int32.tryParse baked in, and Ints.tryParse from google guava in Java. C# treats null and a non-integer String identically, Guava matches Dart and throws on null.

From one perspective the fact that there are 2 "types" of bad input is more confusing. From a consistency perspective, though, int.parse does already make a distinction between the inputs 'a' and null - for the former it will call onError and for the latter it will throw. It also makes the expected Error vs Exception distinction which points to a non-null input being the most straightforward migration.

The documentation has been updated since this issue was filed and/or closed. It currently state that:

You are quoting the documentation for int.parse. However you are correct that tryParse was fixed in https://github.com/dart-lang/sdk/commit/6df253db27c2353744956e3a4eb8c1c9723c685a

I was mistaken when I reopened the issue because when I searched for "int.tryParse" in my docs viewer I ended up on BigInt.tryParse which never got the same fix. Sorry for my confusion.
https://api.dart.dev/stable/2.4.1/dart-core/BigInt/tryParse.html

This will also make the documentation issue moot

Agreed. We won't need to worry about the documentation or user confusion caused by runtime failures. The extra noise in the code will still be a concern.