dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

proposal: `no_nullable_values_in_interpolated_strings` #3149

Open pq opened 2 years ago

pq commented 2 years ago

no_nullable_values_in_interpolated_strings

Description

Do not use nullable values in interpolated strings.

Details

TODO:

See: https://github.com/dart-lang/language/issues/2053.

Kind

Error

Good Examples

void main() {
  String test = 'hi';
  String anotherTest = '$test';
  print(anotherTest);
}
main() {
  String? str = 'there';
  print('hi $str');
}
String? lastUpdatedTime;

Text('the last updated time is ${lastUpdatedTime!}');

Bad Examples

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

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

Discussion

See: https://github.com/dart-lang/language/issues/2053

lrhn commented 2 years ago

The examples here suggests that the underlying rules would be:

Correct?

Levi-Lesches commented 2 years ago

Given the discussion at dart-lang/language#2053, I'd say having a string that possibly contains the word null being printed on-screen should still be linted. Additionally, via any way strings can make it to the user, which infamously includes Text widgets. Given that, I'd say the lint should be at the string level itself. In fact, given that strings are almost always passed to humans (or other APIs), it should always be a bad sign when a nullable value makes its way into a string, since that can result in "null".

So, you'd end up with:

String? nullable;
String nonNull = "Hello, World!";

final a = "Welcome, $nonNull";          // ok, non-null value
final b = "Welcome, $nullable";         // lint: nullable expression in string interpolation
final c = "Welcome, ${nullable ?? 'user'}"; // ok, non-null value
final d = "Welcome, ${nullable!}";      // ok, assumed to be non-null
final e = "name=$?nullable";            // ok, value is allowed to be null, see dart-lang/language#2053
final f = "$a $b"               // ok, b is unsafe, but using its value is fine
print(b);                   // ok, b is unsafe, but using its value is fine
lrhn commented 2 years ago

The problem is that print("something $var1 $var2"); is a classical debugging thing to do, and it would get really annoying if that couldn't print null values.

In general, print is mainly intended for debugging. If you are writing a console application, you can, and probably should, use stdout.write instead. If you are on the web, a print is definitely debug information.

If we disable this lint only for string literals that are the argument expression of a print call, but don't try to do flow analysis to see where another string ends up, then I think we'll make debug-printing people happy, and won't affect much of anything else.

The alternative is to do flow analysis on the values (which is definitely approximative at best) and consider any string containing a nullable interpolation as "tainted". If it flows anywhere except into print or into another string interpolation (which will then also be tainted), then we lint. I don't think that extra complication is actually worth it. All my debug-prints are string literals (I also don't care about 80 character line length in debug code, so I won't be introducing variables for sub-strings unless absolutely necessary).

If we actually introduce a "$?x" null-aware interpolation syntax into the language, I think we'd make normal interpolations non-nullable at the same time, so no lint would be necessary.

I'd be fine with only allowing "${?nullable}", not "$?nullable". (We'd obviously have to decide what the interpolation puts into the string if the value is null, which will most likely be nothing (empty string, so ?nullable is equivalent to nullable ?? "".)

AlexanderFarkas commented 2 years ago

What if I want to print such values for logging purposes? Either using a package called logger or debugLog. Restricting null acceptance only to print doesn't seem to be either useful or extensible.

AlexanderFarkas commented 2 years ago

@lrhn I suppose "${?nullable}" should print null, because putting null-awareness mark means, in almost all cases, that this value is used for debugging purposes.

If user deliberately refuses interpolation-soundness, they should process 'null-case' themselves (or leave it as is).

srawlins commented 3 weeks ago

I was bit by this recently in dartdoc. Very surprising to see "foo/bar/null" show up as a URI. And hard to debug.

Levi-Lesches commented 3 weeks ago

For what it's worth regarding the earlier concerns about debug printing, if this is kept as a lint rather than a warning, it will be at the same level as avoid_print and therefore shouldn't bother too many people who just want a quick-and-dirty print statement and were okay with ignoring lints in the first place. (I'm also not attached to the $? syntax, I only brought it up from the previous discussion I linked).

lrhn commented 3 weeks ago

Instead of being clever, we could also just say "no nullable values in interpolations" and require you to do "print("something ${var1.toString()} ${var2.toString()}"); if you want the "null"s printed. It's a lint. You are asking for it. (But that may make it ineligible as a recommended lint.)

The $?var1 probably won't happen. I do hope we can make interpolations into "elements" like collection elements, so you can do things like: print("something ${?var1} ${if (var1 != null) var1}, ${for (var x in [?var1, ?var2]) x}");.