Ignore another dead_code hint for a weak-only null check. #63602
Conversation
When dart-lang/sdk#41985 is fixed, the analyzer will begin detecting dead code due to "unnecessary" null checks. Since we want to retain null checks in Flutter even when they appear unnecessary (in order to preserve runtime behavior in weak mode), we need to suppress these dead code hints.
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flutter-dashboard
bot
added
the
framework
| // Note: `key.bundle.load` has a non-nullable return type, but might be null | ||
| // when running with weak checking, so we need to null check it anyway (and | ||
| // ignore the warning that the null-handling logic is dead code). | ||
| if (data == null) { // ignore: dead_code |
There was a problem hiding this comment.
Note: I'm not 100% sure this is the right approach. Is it possible for the user to provide their own implementation of AssetBundle.load? If not, maybe it would be better to change the type of data to a ByteData (removing the ? on line 666) and just remove this check.
Actually, that might be a reasonable approach anyhow, even if it is user-overridable, since the user will still get an exception when data gets dereferenced on line 682; the only difference will be that the exception will be marginally less comprehensible, and the key won't be evicted from the image cache. But maybe that's ok.
Anyhow, I'm open to suggestions.
There was a problem hiding this comment.
yes ? could be removed. But for now I would keep the if (data==null) for now.
There was a problem hiding this comment.
I think we can probably just drop this. If the method is not supposed to return null, we can assume it's handled by throwing an exception.
I'm not quite clear though on when weak checking would be used. Could you explain a bit more about how that would happen?
There was a problem hiding this comment.
@dnfield
Weak checking is when the user's app contains some files that are opted in to null safety (e.g. flutter) and others that aren't (e.g. their app and packages). It does all the static checks for null safety, but the type system is fundamentally unsound at the boundary between opted in and opted out code. For example, if you invoke an abstract method in an opted-in class that has a return type of int, that's supposed to guarantee that you'll never receive null as a result. But if the concrete implementation of that abstract method is in opted-out code, it could return null without causing a compile time errors. That's the sense in which it's unsound: your opted-in code can have a variable of type non-nullable int that contains a null value. Effectively, non-nullable types can't be completely trusted if there is some opted-out code somewhere in the program. Once the entire program has been migrated to null safety, the unsoundness goes away and ints are really ints.
Because of this, we have code in Flutter that looks to be dead code, but we want to keep it around until most users have upgraded their apps to null safety, so that we can continue reporting useful exceptions to them if they pass null to us where they shouldn't. The type system thinks that code is dead but the reality is it's not, so we have to add the // ignore: comment. The reason I'm having to make this CL now is because fixing a bug in the analyzer that was preventing some of the "supposedly dead" code from being recognized.
Does that help? If not, I'm happy to discuss over VC.
There was a problem hiding this comment.
Ahhh ok that makes sense. Yeah we should leave this then.
Description
When dart-lang/sdk#41985 is fixed, the
analyzer will begin detecting dead code due to "unnecessary" null
checks. Since we want to retain null checks in Flutter even when they
appear unnecessary (in order to preserve runtime behavior in weak
mode), we need to suppress these dead code hints.
(This is a follow-up to #63007; an additional case appeared before I had a chance to land the fix for dart-lang/sdk#41985).
Related Issues
dart-lang/sdk#41985 (PR must land before this issue can be fixed)
Tests
N/A since this change does not affect behavior, it merely avoids dead code hints.
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.