Remove unnecessary null checks

[goderbauer]

Part of flutter/flutter#118837.

Dart 3 drops support for non-null safe code, so we can finally turn on the unnecessary_null_comparison lint and remove the unnecessary checks it flags.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[goderbauer]

I thought the original intend of these two tests was that there would be an exception, but rewriting the tests to actually assert that (as shown here) makes the test fail. Needs some more investigation...

[goderbauer]

Interestingly, on the web SDK side these tests are explicitly testing that you can create these with out-of-bounds values:

engine/lib/web_ui/test/ui/color_test.dart

Lines 37 to 47 in bffb983

	test('color created with out of bounds value', () {
	const Color c = Color(0x100 << 24);
	final Paint p = Paint();
	p.color = c;
	});
	test('color created with wildly out of bounds value', () {
	const Color c = Color(1 << 1000000);
	final Paint p = Paint();
	p.color = c;
	});

Maybe that is the intend here after all?

[goderbauer]

Since the color constructor is just removing anything above 0xFFFFFFFF I am going to assume that creating a color with out-of-bounds values should be legal and not throw. I'll adjust these tests accordingly.

engine/lib/ui/painting.dart

Line 108 in bffb983

const Color(int value) : value = value & 0xFFFFFFFF;

[cbracken]

I wonder whether assert(_languageCode != '') or assert(_languageCode.isNotEmpty) is more efficient? My guess is that string equals special cases the zero length check anyway, so probably no real difference and it's a debug-only check anyway.

[goderbauer]

@rmacnak-google Do you happen to know?

[rmacnak-google]

I expect a test against length zero to be completely inlined / a bit smaller than a call to the string equality function. Not a big difference: go with whatever is easiest to read.

[cbracken]

I'd go with isNotEmpty from the readability point of view too.

[goderbauer]

Sounds good, since this PR is so giant, I'll send a follow-up PR with that fix. Thanks, @rmacnak-google.

[goderbauer]

Oh, I guess we cannot make that change:

platform_dispatcher.dart:446:16: Error: Not a constant expression.
  ])  : assert(_languageCode.isNotEmpty),
               ^^^^^^^^^^^^^

[cbracken]

Similar question to above.

[cbracken]

Oops. That's embarrassing. 🤦‍♂️