Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ignore another dead_code hint for a weak-only null check. #63602

Merged
merged 2 commits into from Aug 13, 2020
Merged

Ignore another dead_code hint for a weak-only null check. #63602

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf75605
Ignore another dead_code hint for a weak-only null check.
stereotype441 Aug 12, 2020
5c64c4f
Clean up comment
stereotype441 Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/flutter/lib/src/painting/image_provider.dart
View file
Open in desktop
Expand Up @@ -672,7 +672,10 @@ abstract class AssetBundleImageProvider extends ImageProvider<AssetBundleImageKe
PaintingBinding.instance!.imageCache!.evict(key);
rethrow;
}
if (data == null) {
// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes ? could be removed. But for now I would keep the if (data==null) for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh ok that makes sense. Yeah we should leave this then.

PaintingBinding.instance!.imageCache!.evict(key);
throw StateError('Unable to read data');
}
Expand Down