Skip to content

Interpolation is letting a null string bypass null safety feature #2053

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

Open
MarsGoatz opened this issue Jan 7, 2022 · 52 comments
Open

Interpolation is letting a null string bypass null safety feature #2053

MarsGoatz opened this issue Jan 7, 2022 · 52 comments
Labels
nnbd NNBD related issues state-backlog

Comments

@MarsGoatz
Copy link

MarsGoatz commented Jan 7, 2022

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 : #2053 (comment)

EDIT 2:

Summary: #2053 (comment)

@leafpetersen
Copy link
Member

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.

@MarsGoatz
Copy link
Author

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
Copy link
Author

MarsGoatz commented Jan 8, 2022

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 MarsGoatz changed the title Nullable String can be assigned to non nullable via interpolation Interpolation is letting a null string bypass null safety feature Jan 8, 2022
@MarsGoatz
Copy link
Author

Renaming issue

@MarsGoatz
Copy link
Author

MarsGoatz commented Jan 8, 2022

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

@srawlins
Copy link
Member

srawlins commented Jan 8, 2022

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

@cedvdb
Copy link

cedvdb commented Jan 8, 2022

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.

@srawlins
Copy link
Member

srawlins commented Jan 8, 2022

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

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

@MarsGoatz
Copy link
Author

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
Copy link
Author

MarsGoatz commented Jan 8, 2022

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.

@AlexanderFarkas
Copy link

AlexanderFarkas commented Jan 8, 2022

@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
Copy link

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

@MarsGoatz
Copy link
Author

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

@srawlins
Copy link
Member

srawlins commented Jan 8, 2022

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().

@MarsGoatz
Copy link
Author

MarsGoatz commented Jan 8, 2022

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
Copy link
Author

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" 😆

@MarsGoatz
Copy link
Author

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:

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
Copy link
Author

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
Copy link
Author

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.

@rakudrama
Copy link
Member

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()),
    );
  }
}

@MarsGoatz
Copy link
Author

MarsGoatz commented Jan 8, 2022

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.

@cedvdb
Copy link

cedvdb commented Jan 9, 2022

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.

@rakudrama
Copy link
Member

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).

@MarsGoatz
Copy link
Author

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
Copy link
Author

MarsGoatz commented Jan 9, 2022

@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
Copy link
Author

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! 👍🏾

@eernstg
Copy link
Member

eernstg commented Jan 10, 2022

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.

@leafpetersen
Copy link
Member

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

@lrhn
Copy link
Member

lrhn commented Jan 12, 2022

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.

@munificent
Copy link
Member

The original issue discussing this is dart-lang/sdk#57271.

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.

@cedvdb
Copy link

cedvdb commented Jan 12, 2022

@tatumizer how is this issue related to null ?

using toString on null might be a bug:

int a = 3;
int? b = 4;
Text('a: $a, b: $b'); 

Currently we can do this:

int a = 3;
Future<int> b = Future.value(4);
Text('a: $a, b: $b');  

Which is also an user made bug for client facing strings. That's why future was mentioned. It can be extended to any kind of value that's not a number or String. This is a bug too (most likely):

int a = 3;
Result b = Result(4);
Text('a: $a, b: $b'); 

I'm failing to see how those 3 cases are different.

So I gather maybe what you want is an operator that does not call toString behind the scene ?

@leafpetersen
Copy link
Member

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.

Why would you have to do that? I think 'blah ${foo.toString()}' works fine, right?

@Levi-Lesches
Copy link

I believe that comment was regarding making .toString() (or any Object member) on a nullable value an error.

@leafpetersen leafpetersen added nnbd NNBD related issues state-backlog labels Jan 12, 2022
@leafpetersen
Copy link
Member

We discussed this in the meeting this morning. Some notes:

  • There is agreement that this can be a foot gun.
  • There is concern that this might also be a very useful feature when printf debugging and logging.
  • If we disallowed this, we could add a null aware interpolation, e.g. "$?{nullableVariable}" which accepted nullable values.
  • We could alternatively consider trying to generalize the collection literal element syntax to interpolations.
  • Tagged strings, if we shipped them, would provide one way to address this, but this doesn't fix the "pit of failure" here in which the default mechanism leads users astray.
  • This is a good candidate for a lint in the short term.
  • More data on how often interpolation is used on nullable values (and how many of those uses are unintentional) would be useful here.
  • We don't have a lot of team bandwidth to tackle this at the language level this quarter, but there's interest in pursuing this further at some point (e.g. gathering data on how breaking changing this would be).

@pq are you interested in evaluating this as a lint candidate? Part of that evaluation could include doing some data gathering that might feed into a language decision on this in the future.

@pq
Copy link
Member

pq commented Jan 12, 2022

I started a lint proposal in dart-lang/sdk#58615. Feel free to add details or concerns.

I agree that collecting some real-world data would be really useful here.

@Levi-Lesches
Copy link

Levi-Lesches commented Jan 14, 2022

I would really like to see $?. It seems that at the heart of it, everyone on this thread agrees that the word "null" appearing in a user-facing string is a disaster, but it's debatable whether that's worth making every print statement harder to read, and what about APIs that communicate with strings where null may need to be passed? Having $? fixes that: by default, strings are safer, but if for some reason you want to see a programmatic value in a human-readable string, that's fine, just write one more character. As a bonus, you don't make the whole string unsafe: "Hello $user, your report is $?userReport".

@lrhn
Copy link
Member

lrhn commented Jan 14, 2022

We'd probably want both "$?foo" and either "${?foo}" or "$?{foo}". The latter is simpler, but I really would like to allow collection-elements in interpolations, and we want ?expr to be a null-aware collection element, so we might just want the former, so that we won't end up having both.

We also have to specify what $?foo does for a null value. Will it be the "null" string, like now, or will it be the empty string, which is more likely useful in most end-user cases (and would match the behavior as a collection element, ?null introduces nothing, not null).
I just worry that "noting" isn't the right thing in most cases, because there is context. The example

"Hello $user, your report is $?userReport"

is a good example, because a result of Hello Levi, your report is (trailing space and all) does not work any better than appending null would.

The one thing that using an empty string would hurt is debug-prints, where you might want the "null" string to be printed, to distinguish it from the empty string.
You can always do ${foo ?? "any default value"} to choose a different string, and ${foo.toString()} to get null from the null value. It also allows you to define the semantic meaning of null, if it means anything except "nothing here").

The default should be the one that is correct in most cases where you have a nullable value and want to put it into a string. (I guess research is required!)

@Jetz72
Copy link

Jetz72 commented Jan 14, 2022

I like the idea of using the tagged strings proposal for something like this. Rather than just using a "nonnull" tag processor specifically to filter out nullable expressions, it could be broadened to a processor that outputs a "user-facing string" which could combine the null safety with things like i18n support (through a keyless implementation similar to this). Methods responsible for presenting text to the user could be altered or given variants to take in these user-facing strings, encouraging their use where they're appropriate and leaving everything the same where they aren't.

@mmcdon20
Copy link

Maybe this is a point to consider. Often when I write a dataclass-like class, I will define a toString that includes the class name and the values for each of the properties, and this may have nullable values interpolated into it.

void main() {
  final person = Person(firstName: 'Matthew', lastName: 'McDonald');
  print(person);
}

class Person {
  final String firstName;
  final String? middleName;
  final String lastName;

  const Person({
    required this.firstName,
    this.middleName,
    required this.lastName,
  });

  @override
  String toString() =>
      'Person(firstName: $firstName, middleName: $middleName, lastName: $lastName)';
}

The above prints out:

Person(firstName: Matthew, middleName: null, lastName: McDonald)

and seeing "null" in the output is the intended behavior here.

@cedvdb
Copy link

cedvdb commented Jan 15, 2022

We also have to specify what $?foo does for a null value. Will it be the "null" string, like now, or will it be the empty string

Personally I don't think this operator would add value compared to the alternatives. Today you can write:

  • 'value is ${ value ?? '' }'
  • 'value is ${ value ?? 'null' }'
  • 'value is ${ value ?? 'There is no value yet' }'
  • 'value is ${ value ?? 'Nada' }'
  • 'value is ${ translate(value) }'

Such operator is only useful for edge cases for a fraction of people, while helper functions, be it tagged strings or just a function, gives the user the flexibility his project requires. I just don't think this is a decision that should be built-in the language . I'm also against removing toString from null simply because it seems like too much overhead.

@Levi-Lesches
Copy link

@cedvdb, I agree that there shouldn't be a decision made as to what the language is ("Nothing" vs "null" vs '"", etc), but I do still believe there is value in a $?value if it meansa null-safe version of the "current behavior", making the default $value non-nullable. Think of it as:

// Common use-case 1: print a string with no nulls in it:
final nonNull1 = "Value is: $value";			// new
final nonNull2 = "Value is: ${value!}";			// current

// Common use-case 2: print a string with 'null' in it:
final nullable1 = "Value is: $?value";			// new
final nullable2 = "Value is: ${value ?? 'null'}";	// current

Note that this doesn't involve removing Null.toString(), but instead avoids calling it by using null-aware operators.

@cedvdb
Copy link

cedvdb commented Jan 17, 2022

I do still believe there is value in a $?value if it meansa null-safe version of the "current behavior"

Why specifically null values though ? Why isn't Future or any other instance not a problem that also need safe guarding ? Is it because it is less frequent ? If so the problem will remain for those other cases.

void main() {
 final a = Future.value(3);
 // this could be a user facing string
 print('value of a: $a');
}

I find it would be better to have an operator that only works with String, number. This way you really can safe guard user facing strings.

void main() {
 final a = Future.value(3);
 // compilation error
 print('value of a: #a');
 // accepted
 print('future: #{a.toString()}');
}

Here you gotta go out of your way to show a "dev string". Be it null, Future, or a class instance.

@lrhn
Copy link
Member

lrhn commented Jan 17, 2022

We have, for better or worse, made null special already, with all the null aware operators and language features around it (instead of doing the reasonable thing and never introducing it to begin with 😈).
Going down that track, we are open to finding other places where a nullable value should perhaps be restricted to being non-nullable instead. We have the tools to make the conversion possible.

On the other hand, string interpolation is inherently a low-level and permissive operation. It allows any object, because every object has a toString method. Because of that, it's just as inherently untyped. It's convenience over safety, and anything we do to make it safer will cost in convenience.

The way to make a type-safe interpolation is to have a helper function String valueString(String value) => "value of a: $value"; and use that, instead of inlining the untyped interpolation. Obviously, most people won't bother with that. Too inconvenient!

We have more functions which accept Object?, like StringBuffer.write - which would be the obvious alternative to interpolation, so why is interpolation special?
Anything which calls toString is suspicious, because it's not clear that you'd want to accept null. It's also not clear that you wouldn't, but the string "null" is probably only useful for debugging, and logging, and JSON conversion, and a few more things that I haven't thought of yet.
I'd half seriously propose that we remove toString from Null and only have it on Object. Any place that expects to do toString on an argument, you'd have to pass x ?? "null", but that would also force you to decide which string to use to represent null. But it would also affect code which does more things with a value than just convert it to a string, like List.toString and every other generic class with a potentially nullable type argument, which won't be able to do toString on the values. Doing x?.toString() ?? "null" is not pretty either.
(But also, why does List.toString exist today, except for debugging?)

As might be clear, I'm very unclear about what would be the best thing to do here, or whether doing anything is worth it.

@cedvdb
Copy link

cedvdb commented Jan 17, 2022

I'm very unclear about what would be the best thing to do here

What about

an operator that only works with String, number

This kills two birds with one stone.

I don't find the argument that null is a special value relevant in this instance, I also find removing toString from null misses the point. This is a wider issue that people forget that toString is called in the background and that null has a toString method, but in reality this can happen with other values than null. This is an issue with any value that is not a string or a number because the resulting string might not be appropriate for display. Future being the other primary example beside null.

It can be accomplished with tagged strings but language supports might make sens since dart is advertised as a client language.

we are open to finding other places where a nullable value should perhaps be restricted to being non-nullable instead

I for one like null safety but focusing on null here is imo a mistake. It is not an instance where null is a problem it's any value that is not supposed to be used in UI and when not using internationalization that's probably any value beside strings and numbers. With internationalization it's a non issue since interpolation isn't used.

@lrhn
Copy link
Member

lrhn commented Jan 17, 2022

This is a wider issue that people forget that toString is called in the background ...

I don't think they forget that. That's why they happily use types other than String in an interpolation. If anything, they rely on toString being called. They just sometimes pass in the wrong value, into something which is inherently untyped and therefore can't stop them.

This is an issue with any value that is not a string or a number because the resulting string might not be appropriate for display.

Why numbers, then? The toString of double is ... intricate. If you intended for

var x = 0.1 + 0.2;
print("$x");

to print 0.3, you'll be surprised. (And many people have been).
If you expect

var billion = 1000000000;
var bignum = billion * billion * billion;
print("$bignum");

to print a one followed by a lots of zeros ... you'll be surprised again (it's "1e+27").
And just one bad division by zero, and you'll be telling people that they owe you $ Infinity.

If anything, I'd reject doubles too. They are at least as dangerous as a DateTime object.

So, strings and integers, and then we might as well just say strings only.

And, suddenly, interpolations stops being that much more convenient than just using +,

Arguably, you shouldn't use strings at all when creating user-facing text. Every text should be templated, internationalized and every template parameter should be formatted deliberately (preferably in a locale specific way), after passing security validation.

The "perfect" is a very high goal. I don't think we're aiming for that. The question is if there are low-hanging fruits that can catch some easy-to-make mistakes, without getting in the way of interpolations actually being convenient.

@cedvdb
Copy link

cedvdb commented Jan 17, 2022

without getting in the way of interpolations actually being convenient.

The thing is that it would not change anything about current interpolation. You'd have to use a special token value #value (instead of $ it's #).

And, suddenly, interpolations stops being that much more convenient than just using +,

Fair enough, it's a arguably a nicer syntax though.

Arguably, you shouldn't use strings at all when creating user-facing text

Agreed, that is principally why I was against "catching the low hanging fruits" since to me this is a non issue and you are going to impact people for which it is a non issue by throwing the low hanging fruits at them. I hope it won't change how it is today, that is I hope null will keep working in interpolation as it does today. If something needs to be done I suggest using an alternative token like # for invalid null interpolation so it does not impact those who do not use interpolation in UI, but use it extensively for dev messages, which is probably the majority of serious production apps. Having to even think about $? is just another mental gym that I don't want to be subject to, that is why I'm pushy in the other direction.

I guess I've made my point so I'll unsub from here to not be too dense.

@rrousselGit
Copy link

Without debating about the other aspects of this issue, I do think that's a quite important feature.

It would bring a significant improvement for code-generators, who use string templates / interpolation quite a lot.

@beauxq
Copy link

beauxq commented Feb 19, 2022

I think it's significant what lrhn pointed out.
If it were a nullable int, toString is needed for + but not for interpolation.
So it makes sense to work the same way with nullable string.

That's kind of how python works:

>>> a = None   
>>> print(f"jkl{a}")
    jklNone
>>> print("jkl" + a)   
    Traceback (most recent call last):
      File "<pyshell#11>", line 1, in <module>
        print("jkl" + a)
    TypeError: can only concatenate str (not "NoneType") to str
>>> print("jkl" + str(a))
    jklNone

This gives a safer way to make UI strings (use + instead of interpolation).

@cedvdb
Copy link

cedvdb commented Jul 13, 2022

This is a wider issue that people forget that toString is called in the background ...

I don't think they forget that.

The issue is not "forgetting" more so than no future changes compilation error, which is kind of the point of type safety. While I object the premise of null being the issue here, I still think that a stronger string interpolation would be helpful:

class MyClass {
   String value;
} 

class MyClassUser {
   final MyClass myClass;
   foo() {
      return 'value is: ${myClass.value}'; // I did not forget that it calls toString in the background.
   }
}

Which is later changed to:

class MyClass {
   SomeNewWrapperClass value;  // oops now every string interpolation is faulty, but the compiler won't tell you
} 

A new syntax like with # that would require either a String ,Numbers or boolean would be nice

final txt = 'value:  #{myIntVar}, myValueObject: #{myValueObject.toString()}'; 

Admittedly as soon as toString() is called on a variable the same issue as mentioned above can be back, but at least it protect one level... As writing this I'm not even sure this is a good idea.

@lrhn
Copy link
Member

lrhn commented Jul 13, 2022

There is no type safety issue because there is no type issue. You can call toString on any object, so you calling toString on something you expect to be a Foo, but which is really a Bar, won't trigger any warnings. After all, why would it?
You can change the behavior of Foo.toString too, without affecting any types. The precise value of a method call is not a type issue, only its type is.

That's not particular to interpolations, it applies to any operation which accepts any object.
If you do identical(foo, something.foo), then you don't get a warning when something.foo changes to be a Bar instead of a Foo. Or foo == something.foo.
All of those operations accept any object, so it's not a type problem that the type of the operand changes.
It might be a logic problem for the program, but the compiler has no way to know, because you never told it.

In the example ${myClass.value}, there is no type issue with changing the declaration to SomeNewValueClass value, so it's not something the type system can catch.
If you choose to do:

  MyClass value = myClass.value;
  return 'value is ${value}';

then you'll get a warning when the type of myClass.value stops being a MyClass, but that's because you asked for it.
If you wrote:

  Object? value = myClass.value;
  return 'value is ${value}';

you wouldn't get any warnings, and you wouldn't expect any.
If you required strings only, and wrote:

  return 'value is #{myClass.value.toString())}';

then you are back to square one, because you can call toString on anything. (Like you noticed.)

You can't fix a type problem using an inherently untyped feature. The moment an interpolation, or you, or anyone, calls toString on a value, they have stopped needing to care what type the value is. Any caring should happen before that.

We could perhaps remove toString from Object?, and T? in general. Then you'd have to cast to either null or T before calling toString.
The problem with that is that it will break a lot of generic code. You'd no longer be able to do class C<T> { ... something(T value) { .... log(value.toString()); } } because the bound of T is Object?. Any feature we remove from Object? is a feature you can no longer use on something typed by an unbounded type parameter. That's never going to fly.

The alternative, to make print take Object as argument and require interpolation values to be non-nullable, is technically possible. It requires migrating all existing code so that "$x" becomes "${x ?? 'null'}" whenever x is potentially nullable. That's a big migration, I doubt we'll consider it worth the effort. But possible. That'll only catch accidental nulls, but it's probably a low hanging fruit.

Or, most likely, we can introduce a lint warning about nullable expressions in interpolations. Then you can fix them if you care, and don't have to if you don't.

@cedvdb
Copy link

cedvdb commented Jul 13, 2022

I think you are speaking in general terms, but where this issue is most likely to be a problem is in UI, or some output text. I start with the premise that the values that are going to be rendered in UI are either String, the name of a person, or a number, his age; Boolean don't even fall there. The rest is the role of localization. If that assumption is true toString() won't ever be called explicitely in a UI.

The argument is that # will therefor catch the issues I mentioned above, as no one should use toString explicitely with it (if the above assertion holds), or else be subject to issues. And toString would not be called implicitely so there is some sort of typesafety.

Note:

  • I changed my previous comment while you were sending yours, the # accepts numbers.
  • I still object to giving null a special treatment, so maybe this is not the right thread to discuss this.

@lrhn
Copy link
Member

lrhn commented Jul 13, 2022

So you are asking for a new feature, which is used to create strings "intended to be rendered in the UI".

That feature is like string interpolation, except that the expressions can only be strings or numbers. (I'd say integers, doubles are right out!)
Honestly, presenting integers in a UI is also much deeper than just a sequence of digits. You may want thousands separators, conversion to 1.2M instead of 1200000, etc. Nothing in visible display is just "plain text'.
Plain text is an illusion.

So, maybe just strings.

That's very specialized. Not all strings are going to be rendered, which is not a problem since we'll still have the normal string interpolation to create the rest.

It's also not particularly user friendly. If you have an object, and want a string representation of it, you can do

  "#{foo.toRenderString()} ..."

but you can also do that today with $. The only difference is that today you get .toString() for free, whether you want it or not.
But you can also write "#{foo.toString()}..." by accident and not get any warnings about it. Takes slightly more effort, but far from being impossible.

If your object actually uses its toString for its UI-compatible representation, then you'd be incentivized to just use a normal interpolation. Nothing stops you from that, it's not like "UI-intended strings" is recognizable in the type system.

What you should be doing instead is to write proper macro functions:

String renderFoo(Foo foo) => "value is: {$foo.value}"; // Double checked and tested!

Again, you can do this today, you don't need a different string interpolation for that, one which doesn't actually solve all the problems.

All in all, I don't think a restricted interpolation is actually the best way to solve the problem.
Special casing nullability will catch some very-likely-mistakes.

Rejecting anything non-string in an interpolation is just a weaker feature than normal interpolation, but not a much safer one. It's possible, but, IMO, not worth the necessary effort (new syntax costs a lot of developer resources).

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

No branches or pull requests