language icon indicating copy to clipboard operation
language copied to clipboard

Interpolation is letting a null string bypass null safety feature

Open MarsGoatz opened this issue 2 years ago • 52 comments

void main() {
  String? test;
  String anotherTest = '$test';
  print(anotherTest);
}

I tried this on dartpad this prints null.

Feel like this is a hack and shouldn't be allowed? So submitting an issue.

EDIT:

Please go through this example for complete context https://dartpad.dev/f2093518fb25acb7e5de6908174e6d2e

Also, this reply : https://github.com/dart-lang/language/issues/2053#issuecomment-1007886973

EDIT 2:

Summary: https://github.com/dart-lang/language/issues/2053#issuecomment-1008101529

MarsGoatz avatar Jan 07 '22 22:01 MarsGoatz

The value that is assigned to anotherTest here is not null, it is the string 'null'. That is, the expression '$test' is equivalent to ${null.toString()}, and null.toString() returns the non-null string 'null' , which is then interpolated and printed out. So there is no violation of the type system here.

You might object to the fact that a nullable value can be interpolated, instead of requiring it to be non-null, but when we discussed this it was universally felt that this would be user-unfriendly on balance. It's quite painful not to be able to print out nullable values without conditionally checking whether they are null or not and printing out different things in the two cases.

leafpetersen avatar Jan 07 '22 23:01 leafpetersen

I understand your comment about the type system here and there are two things I feel about this:

  • String interpolation here is undoing all the "good things" that come with null safety
  • String interpolation is dangerous to use in production with null safety on

I would recommend

  • Adding this to official documentation, something similar to "watch out for pitfalls"
  • Maybe a lint rule?

MarsGoatz avatar Jan 08 '22 00:01 MarsGoatz

The pitfall here, IMO, interpolation is letting the null string bypass the null safety feature.

Consider this example:

Let's say we have a data variable called String? lastUpdatedTime and consider the following ways to show a text in Flutter using Text widget which takes in a non nullable string:

Text('the last updated time is  ' + lastUpdatedTime)
Text('the last updated time is $lastUpdatedTime')

In the first scenario, the compiler will complain about type system, which in turn you might add a condition to see if it isn't null(or add a !) and then show the Text widget but in the second case, you have a situation you might end up with "the last updated time is null".

That is the pitfall and my concern here. But saying that, the core reason is, null has a toString method which spits out "null". So even if you use option 1 with int, you will end up with "null" string.

I personally am not going to use interpolation anymore in my production code because I often use nullable strings.

Anyway, Thank you for the reply, null! ;) Appreciate the explanation about how interpolation works.

MarsGoatz avatar Jan 08 '22 05:01 MarsGoatz

Renaming issue

MarsGoatz avatar Jan 08 '22 05:01 MarsGoatz

Here is a quick example that demonstrates the issue more clearly with Flutter UI. The idea is the data takes 2 seconds to load. Without interpolation the experience is much better because null safety kicks in and you end up showing a better UX instead of "null" strings.

https://dartpad.dev/f2093518fb25acb7e5de6908174e6d2e

MarsGoatz avatar Jan 08 '22 06:01 MarsGoatz

I wouldn't be opposed to a lint rule that warns about a nullable-typed expression in a string interpolation.

srawlins avatar Jan 08 '22 07:01 srawlins

If there was a lint rule there needs to be an alternative to print potentially null value (there is the + apparently but you lose interpolation).

This is such a common use case, every class that has nullable members and a toString override will have to use this, that seems catostrophical. No one is going to use that lint rule.

cedvdb avatar Jan 08 '22 10:01 cedvdb

If there was a lint rule there needs to be an alternative to print

What do you mean, "an alternative?" An alternative lint rule?

srawlins avatar Jan 08 '22 15:01 srawlins

If there would be an alternative to interpolation where you would be forced to use bangs on nullable values, that would be a great solution. Appending with + also suffers from "null" string for types other than String because you would be forced to pull out the toString() method.

MarsGoatz avatar Jan 08 '22 15:01 MarsGoatz

What are the consequences of removing toString() from null other than interpolation breaking and being forced to use bangs on nullable values? Because that is the root cause of all this.

MarsGoatz avatar Jan 08 '22 15:01 MarsGoatz

@MarsGoatz I could produce such code: String myNullString = "null"; It's absolutely equivalent to your case. Is it "null safety bypass" here? Obviously not.

AlexanderFarkas avatar Jan 08 '22 15:01 AlexanderFarkas

Null has toString, that's why it could be interpolated. I don't see any inconsistency here.

AlexanderFarkas avatar Jan 08 '22 15:01 AlexanderFarkas

@AlexanderFarkas Please go through the whole thread for context. Also have a look at this example https://dartpad.dev/f2093518fb25acb7e5de6908174e6d2e

MarsGoatz avatar Jan 08 '22 15:01 MarsGoatz

What are the consequences of removing toString() from null other than interpolation breaking and being forced to use bangs on nullable values?

You wouldn't be able to call toString on nullable values. For example, print(nullableValue). You don't want to replace this with a ! because that may throw an exception if nullableValue is null. You would be forced to write your own value, like print(nullableValue ?? 'null'). I can see this being annoying, and hard to read with multiple string interpolations. '$a$b$c' turns into '${a ?? 'null'}${b ?? 'null'}${c ?? 'null'}'.

In any case, I can see where I want to explicitly control what text is used in lieu of a null value. I would use a lint rule that warned about nullableValue.toString().

srawlins avatar Jan 08 '22 15:01 srawlins

Thanks @srawlins!

Wouldn't print(nullableValue ?? 'null'), though annoying, be more in alignment with what Dart's null safety is by definition?

w.r.t the lint rule, I am with @tatumizer if it can get annoying, specially with false positives.

MarsGoatz avatar Jan 08 '22 15:01 MarsGoatz

We have other cases that are potentially more harmful than that. See this comment (Note: the function parameter could be of type int? - and the problem would be still there)

I can imagine - "Hey User, You bank balance for today is null" :laughing:

MarsGoatz avatar Jan 08 '22 16:01 MarsGoatz

Hey @leafpetersen,

I have more context now, thanks to replies from you, @srawlins and @tatumizer. So here is the summary. If you haven't read already, I would recommend you go through:

  • this reply https://github.com/dart-lang/language/issues/2053#issuecomment-1007886973
  • this example here that shows you the consequences of bypassing null safety in interpolation.

Here is my take on this:

With dart introducing null safety, you get used a certain paradigm, i.e, you simply can't assign a right hand expression which contains a nullable variable to a left hand variable which is non nullable. IMO, this is very central to what has been introduced as part of null safety.

So both interpolation and appending with + breaks that paradigm. I would say, interpolation and appending with + are not in parity with null safety for what ever reason.

The root cause is null has a toString() method which prints "null" instead of forcing the dev to handle "what if the value is null" breaking away from the paradigm. This can have unforeseen consequences, particularly from an app developer point of view.

I am by no means a dart expert but here are my recommendations:

  • Removing toString() from null. Obviously a breaking change. Handles both interpolation and appending with +.
  • Add another way of interpolation which forces null safety, using another symbol, e.g., ^ or % etc. Not a breaking change and devs can migrate if they want to. Only handles interpolation.

I feel linting might not be the solution here if it was possible, only a duct tape.

MarsGoatz avatar Jan 08 '22 18:01 MarsGoatz

I would go for something that explicitly suggests that nullable objects aren't allowed and make it agnostic of business logic, e.g., user strings or server side strings.

In the end whatever we chose should nicely blend in with existing dart ecosystem and we don't want devs to feel "oh this doesn't feel like dart".

MarsGoatz avatar Jan 08 '22 21:01 MarsGoatz

tagged strings.

This is pretty cool! Thanks for the share, learning something new everyday.

I would go with nonnull maybe, more explicit or nonnullable. This sounds so much like Lombok from Java.

But TBH, can't help but feel this is contradictory to what dart should provide by default. In an ideal dart language we should be using nullable tag but here we are discussing the other way around.

If Flutter delivers on its promises, in five years, we will looking at tons of Dart users. I would honestly take the opportunity to make the right fix now and bring interpolation and appending with + in parity with Dart's null safety.

MarsGoatz avatar Jan 08 '22 22:01 MarsGoatz

This is an interesting example (thanks for the dartpad, that was very helpful!)

All of the suggestions above basically boil down to having (1) a compile-time check to ensure that you (2) insert a run-time check at each use, and perhaps when doing this also (3) realize that the State needs special cases to handle the pre-valid state.

One option not mentioned yet is to use late fields, with non-nullable types (unless null really is a valid value). Using late variables allows you to say at the declaration that (2) run-time checks should be inserted automatically, but does miss the opportunity to make you think (3) about the pre-valid state.

Explicitly modelling the pre-valid → valid transition, together with late fields, would look like this:

class _InterpolationExampleState extends State<InterpolationExample> {
  late String name;
  late String motivationalQuote;
  bool _ready = false;

  @override
  void initState() {
    super.initState();
    Future.delayed(Duration(seconds: 2), () {
      setState(() {
        name = 'Awesome Dev';
        motivationalQuote = 'Flutter is awesome!';
        _ready = true;
      });
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Interpolation'),
      ),
      body: Center(
          child: _ready
              ? Text('Hey $name. Your motivational quote is  $motivationalQuote')
              : CircularProgressIndicator()),
    );
  }
}

rakudrama avatar Jan 08 '22 22:01 rakudrama

Representing nullable value as non nullable using late keyword sounds like a hack to me.

IMO, You use late keyword when you are sure the object will never be null within your app's core data. One example would be third party API's response returning a value after a while but you are sure it won't be null but if it's going to be null, I usually never use late keyword.

MarsGoatz avatar Jan 08 '22 23:01 MarsGoatz

Thanks @srawlins!

Wouldn't print(nullableValue ?? 'null'), though annoying, be more in alignment with what Dart's null safety is by definition?

w.r.t the lint rule, I am with @tatumizer if it can get annoying, specially with false positives.

I get what you are saying, we all want some form of truth in the language. That is the language is logically correct and finds bugs at compile time every time, everywhere. But it just seems that for every instance where you wouldn't want interpolation for null values you'd have a dozen instance where you would want it. Meaning that this is an edge case.

I really wish Djikistra dream was doable but in practice it seems impractical so we end up with things that are somewhat correct. At the end of the day, tests will tell you if your program works correctly, not whether the code compiles or not.

cedvdb avatar Jan 09 '22 01:01 cedvdb

Representing nullable value as non nullable using late keyword sounds like a hack to me.

Quite the opposite 😉. Representing the state of the container (pre-valid, valid) as one of the values of the data model (name==null) is the hack. It is very convenient to use null as a temporarily-missing value, but the trick of using null like this is how we got to the problem of printing 'Hey null, ...' in the first place.

Let us consider a modification where the program allows certain features without logging in. This is represented by the name being null. In this case we display 'Guest'. Now you have the problem of not only changing the display logic to show 'Guest', but revising the ad-hoc coupling of the container state and the data values.

You can increase the chances of finding use-before-ready errors several ways. One is to use late variables. Another is to have a small layer of abstraction methods:

  String? _name;
  String? _motivationalQuote;

  bool ready = _name != null && _motivationalQuote != null;
  String get name => _name!;
  void set name(String value) { _name = value; }
  String get motivationalQuote => _motivationalQuote!;
  void set motivationalQuote(String value) { _motivationalQuote = value; }

This basically amounts to an ad-hoc implementation of late (i.e. a getter /setter which check for prior assignment).

rakudrama avatar Jan 09 '22 18:01 rakudrama

This would be very inconvenient: print and log statements are too common. The simplest fix would be: no tags; introduce a special escape symbol for nullable values. E,g, "$\foo" (which means: if foo is null, that's OK). But:

today, it's too late to make this change (unless the old code can be auto-fixed). in the context of print statement and its friends (log etc), this limitation still looks ridiculous. The escape symbol for nullable variables could have other uses, but this is beyond the scope of this thread.

@tatumizer I think I agree with you about print and log statements that they are too heavily used. I like the idea of escaping a nullable value but as you said it might be too late.

I am hoping Dart team can come up with tagged word for forcing devs to handle null values in interpolation, maybe nonnull. So the devs can start with this tag and downgrade as needed, at least thats what I would do.

MarsGoatz avatar Jan 09 '22 22:01 MarsGoatz

@rakudrama I understand the point you are making and I appreciate the examples.

So, as a Dart user via Flutter, with null safety feature, I am thinking that Dart forces you to handle your null values. I understand that interpolation isn't really null and string "null". In fact, this is more dangerous as I think of it as a silent failure.

I am no Dart expert by any means but If I have to handle null values in my logic anyway, i.e., map nullable value to a non null bool value to see if the value is ready, what was the point of null safety? I would have done that pre null safety too.

e.g.

With null safety

String? name;

//in Flutter UI, because dart will complain, null safety kicks in
name != null? Text('Your name is ' +  name) : ShowSpinner();

//with interpolation you could end up with
Text('Your name is  $name');

Your example suggests:

String? name;
bool isNameReady => name != null;

//in Flutter UI
isNameReady? Text('Your name is  $name') : ShowSpinner();

Without null safety

String name;
bool isNameReady => name != null;

//in Flutter UI
isNameReady? Text('Your name is  $name') : ShowSpinner();

MarsGoatz avatar Jan 09 '22 23:01 MarsGoatz

I think everybody understands the problem now (your original post was too abstract, hence the confusion).

I think as an end user of Dart, I look at different things and as a bare bones Dart developers you and others look at different things. There was definitely a gap and as you and others asked me the right questions and we exchanged information, we bridged the gap. I would call that a success.

The root cause of the problem, as I mentioned earlier, is that "null" has a number of different meanings. "Null" standing for "key not found" is not the same as "null" standing for "use default". These two nulls are as different as "foo" and 42, they have nothing in common. And they are both different from "null" resulting from uninitialized int?, which doesn't mean anything at all except that the variable was not initialized. This "uninitialized" (quasi-)meaning doesn't even belong to the same semantic universe as the other two. (BTW, this is not a new argument: those who followed the old dart's mailing list are aware of it)

Thank you for providing me the bigger picture here! I think I understand where the issue stems from.

null may mean something for the program, but it certainly doesn't mean anything for the user reading the message. This "null" should never be observed as such, except by the programmer debugging the code.

Couldn't have said it better myself, you hit bulls eye here. Cheers! 👍🏾

MarsGoatz avatar Jan 10 '22 12:01 MarsGoatz

I think the underlying conflict here is (1) toString() is a low-level mechanism which is mostly suitable for debugging and in quick-and-dirty programming to solve an immediate problem; hence, (2) any usage of toString() which attempts to impose domain specific constraints on it is going to cause frustrations.

For instance, in an application context it is quite likely that it is always a bug if an interpolated string ends up containing null because one of the interpolated expressions turns out to have the value null. So Dear null, regarding your letter... may always be a bug, but DEBUG: x is null, y is 42 uses the completely general support for transforming any object into a string, and it's probably useful to know that x is null, rather than having a crash at that point because interpolation insists on throwing if it encounters null.

So should this have been the other way around?

We'd probably have to provide the most low-level operation that anyone could ever need in any case, and then any "higher level" features would have to be built on top of that. For instance, how about creation of strings that are safe in some sense? Maybe they can't be more than 80 chars long, maybe they can't contain Unicode code points that give rise to a switch in writing direction, maybe they are not allowed to contain database commands that could enable a black hat attack, etc.etc.

I tend to think that in the situation where a 'null' string appears in application domain data because an expression was unexpectedly null, the remedy isn't to fix toString(), the fix is to use one or more application domain specific methods, say formatAsString, that does toString() in exactly the manner which is appropriate for that application domain, and accepts exactly the arguments that this application domain needs in order to provide the required level of customizability.

The null object wouldn't have the method formatAsString, so the check that would eliminate null in the output would now be a check that we always use formatAsString(...) in interpolation expressions.

This kind of check might need to be rather expressive, because we might want to ensure that one of many "appropriate" methods is used, not just a single specific method, and it would need to be under developer control, such that we can all choose exactly the name we want to use for that "application domain toString" thingy, and check it in exactly the manner which is appropriate for the given application domain.

Perhaps we could delegate that task to enhanced string literals? Cf. tagged strings. Perhaps myValidator'A String with ${some.expressions()}' would allow myValidator to perform a set of operations (such as validity checks) as well as a custom-defined interpretation/expansion of 'A String with ${some.expressions()}'. In particular, it could call formatAsString(with, some, arguments) on each of the interpolated expressions.

eernstg avatar Jan 10 '22 15:01 eernstg

I've put this on the agenda for the team to talk about at one of our next meetings.

leafpetersen avatar Jan 11 '22 05:01 leafpetersen

Everything here assumes a string. If the example had been

int? age;
var str = "Age: $age";
// vs
var str2 = "Age: " + age.toString(); // toString call is needed.

then the + and interpolation are completely analogous.

The main issue here is that some functions and language features accept any object. That's a feature. You can do foo == null without issues because the == operator accepts any object. Same for the identical and identityHashCode functions, map, set and list literal entries, etc. In some places, any object can be used including null. And string interpolation is one of those. It works for null. It also works for integers and futures, and if you do:

FutureOr<String> name = Future.value("yup");
// mistaken logic here allows you to flow through
var s = "Name is: $name";

then it won't complain that name is not a string, because interpolation works with futures too. (Although it's almost certainly not what you want because Future's toString is useless).

The one issue that string interpolations have is that the underlying .toString() call is invisible. People can forget that it's there and assume that if "Name: $name" works, then name must not be null. That's just not true.

lrhn avatar Jan 12 '22 00:01 lrhn

The original issue discussing this is #175.

I agree that it's very annoying to have null sneak into a user-facing string through string interpolation. But at the same time, it would be really annoying to have to do 'blah ${foo?.toString() ?? 'null'}' every single time you want to interpolate a value where you are OK with "null" appearing. For better worse, users use print() and string interpolation very heavily to debug code and it's handy to be able to stringify any object, even null.

I can't imagine us taking toString() off Null or making it a compile error to interpolate a nullable type. But I could definitely see us doing what Erik suggests: add tagged strings and then have some domain-specific tag processors for user-facing strings that know how to properly handle null, escape, sanitize, etc.

munificent avatar Jan 12 '22 01:01 munificent