-
Notifications
You must be signed in to change notification settings - Fork 214
Dart treats a final nullable property as nullable even after checking that the property is not null #1415
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
Comments
@tatumizer In that situation where you are getting the value from a function, I can understand that even if you check the return value to not be null, it doesn't mean the value won't be null when you run the function again (unless it is a pure function, but that might be more complex). But what I am doing is not running a function so a non-ready-value. I am accessing a class property that is even marked as Also, I know If I am checking the |
I agree with you that, in your example, the error should be shown. As you said, But in my example, void main() {
final Person person = new Person(name: "Leonardo");
person = new Person(name: "Eduardo"); // Error: The final variable 'person' can only be set once
person.jobTitle = "Developer"; // Error: 'jobTitle' can't be used as a setter because it's final
if (person.jobTitle != null) {
// Since person can not be reassigned either its property jobTitle,
// and I have already checked that person.jobTitle is not null,
// it is safe to say that person.jobTitle won't be null never.
showInConsole(person.jobTitle); // Error: The argument type 'String?' can't be assigned to the parameter type 'String'
}
}
class Person {
final String name;
final String? jobTitle;
const Person({required this.name, this.jobTitle});
}
void showInConsole(String text) {
print(text);
} Since those values can not be reassigned and the analyzer knows it, shouldn't it also know that if I'm checking for those values to not be null, those values won't be null inside the In case those values could be reassigned, how could you reassign this so it is reasonable to show an error? final Person person = new Person(name: "Leonardo"); |
In this situation, checking for the property to not be null should be enough since, as you said, the compiler can see the declarations.
In this situation, where the code has changed, since the compiler doesn't know because I'm using a function, it is reasonable to show the null-safety error. Maybe there is something I'm still not getting, but I'm not talking about the second situation. I'm talking about the first situation, which is a different one because the compiler can see that the property won't be null. |
I think this is an important consideration. I think the language team has looked at the various cases in which a developer says "but I know the field isn't overridden," such as private fields (#1167), One consolation is that the runtime behavior might take knowledge into consideration with optimizations. Looking at: if (person.jobTitle != null) {
showInConsole(person.jobTitle!);
} If the compiler has optimizations (based on visibility, exact type, etc), then it may not need to execute that |
Duplicate of #1330 |
label:field-promotion is also related to this issue. |
If fields are promoted in some case and aren't promoted in other case depending on global analysis, it is also confusing very much. |
@tatumizer wrote:
That is not correct. Dart does, at least for some compilers, have modular compilation. I believe the development compiler (dartdevc) is one. Among other things, that's what enables hot reload. If you had to wait for the entire program to be compiled, you'd at most have luke-warm reload. 😁 Also, a package is a program part which is also compiled and run (for testing) independently of the program(s) it's part of. It is vitally important to Dart's approach to object oriented programming (and ditto for most other languages with subclasses) that you can write a superclass without knowing its subclasses ahead of time. If you can write code in an instance method, which is then invalidated by a subclass, then you can't really use that feature safely anywhere. (Basically what @Cat-sushi said, if promotion inside a method depends on a global analysis of the entire final program, then it's incredibly fragile.) Using global information for optimization is great, it enables more possible optimizations. If you could declare that a class cannot have subclasses (or not subclasses outside of the declaring package), then you don't need global information (or seen differently, you have global information because the local declaration ensures global cooperation. And that only works with actual language features, not annotations that might or might not be checked). |
It's about being able to see where things can fail. If you see A Existing code didn't have that possibility, and migrating existing code might (as the path of least rewrite) require adding |
Clearly you guys know more than me in this subject, and that's probably why I still can't understand something that seems obvious for me and probably many of the people with this issue.
In this sentence, I'm saying that the type of So now the analyzer have all the information it needs (the exact type that the variable is and the information about that type). Given that, shouldn't it know that if I'm checking for Sure, in a future the code can change. If instead of using I can even change the But right know, in that line of code I wrote at the beginning (and the structure of |
@tatumizer Many thanks, finally I understand what's happening and why is the shown error "valid". |
I generally create local variables if I have to check-and-use a nullable instance variable. That is a way to avoid the I don't think we'll ever see sound promotion of virtual getters. We might be able to see it on non-virtual getters, but even there I'm somewhat against it because it would probably not work on getters, only fields, and that breaks the field/getter symmetry and locks classes into their current implementation. Interfaces are promises about API and behavior, but only the API can really be checked by a computer. If we know that Our problems here are largely self-inflicted. Because we can't look below the API to see that something really doesn't change (because either it's not guaranteed to be true because the getter is virtual, or because the promotion we'd do would then be fragile and depend on the getter staying a field), we can't promote anything except local variables. So, I want to focus on providing a different and better way to get a promoted variable from any existing value, rather than focusing on linking separate reads of a fundamentally unknowable getter. |
Would it maybe make sense to introduce a syntax that prevents overriding a |
The proposal here: field promotion with runtime checking looks like the best solution. We trade a little bit of safety to A LOT MORE convenience. All the examples showing why fields can't promote don't look realistic. In real codebase, 99% of fields/getters don't change type between 2 consecutive calls. |
I personally learn more toward sealed classes, preventing inheritance If it is impossible to extends/implements a class, we know for sure that a |
Even sealed classes wouldn't take away the getter/field symmetry problem. Say (obvious strawman): That would work even without sealed classes (you can implement the interface, but it better be with a |
Why would that be a problem on a class that cannot be extended/implemented? As it would not be possible to override a |
@tatumizer |
@rrousselGit Getter/field symmetry works both ways: A client of a class cannot see whether a property is implemented with a user written getter or an implicitly induced field getter (it's always a getter, sometimes it comes from a field), and the author is free to change between the two. It's an abstraction layer - the interface only tells you that you can read something, it doesn't promise how it's implemented. If you can get promotion from a final field on a sealed class, but not from a getter on a sealed class, then the author of the class is no longer free to change the field to a getter. It has started mattering to clients how the getter is implemented. The author might not know about this, after all, he might never see the code. So he could think that changing a final field to a getter on his already sealed class is a perfectly good refactoring. Instead it's a breaking change. If that applies to all final fields of sealed classes, without any way to opt out of it, then a sealed class becomes less modifiable than a non-sealed class, which is the opposite of the reason to seal (to allow safely adding members later, because we know that no-one is implementing the class). That is why I'm opposed to any attempt to derive unmodifiability of a getter from its choice of implementation — that will immediately make that implementation choice a part of the public API and break the deliberately introduced abstraction layer. I think this is still a far more complicated approach than giving a way to promote-and-bind, which can be used for arbitrary values, rather than adding even more features to the object interface model, and then maintaining and preserving "unmodifiabilty". The bang is just not worth the bucks, they're better spent on something with more bang. |
@tatumizer |
|
But don't sealed classes qualify as an "opt in"? As the enhanced type-inference would be only available if the class is sealed, which requires an extra keyword on the class definition In the end, the sealed class Example {
Example(this.a, this.b);
final int a;
final int b;
} is equivalent to: class Example {
sealed Example(this.a, this.b);
sealed final int a;
sealed final int b;
} As for getters, I believe int get c => a + b; we can do: late final c = a + b; |
Grammatically correct. |
No. |
@tatumizer |
About this idea:
That's #1188. |
Lol, fair point! But I am speaking purely from a pragmatic standpoint, I do not mind placing ! operators, or having them exist after an explicit null-check, none of that makes my app code more brittle. What I do mind, is putting in bang operators, that are semantically weakly linked to some other line in a method, that must never bee removed. We need some way to say, if this null check was removed, the compiler should light up again. Totally agree, this really should not be necessary at all, but also don't want to wait 3 yrs for a perfect solution, when a good enough will do. |
Isn't that exactly what it would do ? If the null check is removed, the ! isn't inserted and the compiler will light up.. This discussion is very bizar. It's counter intuitive to use ! after a null check. If a getter overrides that null check as someone proposed, you cannot even know, because you are not supposed to go into the source code after a null check.. How do you even work with such variables ? If you take that at face value it makes the null check an anti pattern (which it obviously isn't), because it can't be trusted . At that point, the code the null check is run upon is just not robust enough and there is nothing the language can do against it, so far. I'm talking about this:
This kind of code should be documented and not let the remaining 99.9999% of code suffer from non intuitiveness.. |
You must read them once and cache the result locally. Most seem to agree that this is a flaw in the language design that this hole exists, but also that the ship has sailed and it is what it is. So there's various proposals to help mitigate this, I think the leading one is probably if-vars, which would be syntactic sugar for local var shadowing.
|
I thought about that but is that really a language you'd have fun using?
This hole exists in many languages and again the hole is more in the underlying implementation of the getter than the language itself.. I can get behind the fact that it could lead to hard to debug bugs but well so be it tbh You can make the same argument for a setter..
This makes sens but wouldn't virtually all if null check be if var then? That is if you really wish to be null safe. IMO this is just a non issue in most cases and this proposal would make if var the go to just to be safe on the offchance there is something weird happening in an adjacent library. |
You're preaching to the choir here, I totally agree. No one writes code this pedantically by hand, I'm not sure why the compiler needs to be so strict here, especially when that strictness leads directly to orphaned ! operator's shooting your null safety in the foot. |
This issue makes the code noisy, it makes:
I remember that this was an issue on Kotlin, and they fixed it, is there any possibility to add some priority to this? |
I suppose that you've read the discussion above that provides the information regarding why this is this way and why it's not simple to solve it, at least not in a non-breaking way. There are many proposals that intends to propose this issue completely or partially, you may have a look at them by the label Although I agree that this issue is a little annoying, your three problems may be simply solved by instructing your team. For example, in my team we instruct our developers to never use |
…ready checked as not null. Issue similar to dart-lang/language#1415
…ty is already checked as not null. Issue similar to dart-lang/language#1415" This reverts commit b63b398.
Has this discussion changed at all with the introduction of
Yes, I agree, it'd be pretty non-ideal if using the Flutter's So we have to weigh the tradeoff here. Personally, I think improved convenience and/or safety for null checking attributes is way more important than the API purity and marginal reduction in potential breaking changes, but I understand if the Dart team still comes down on the other side. |
Quite a lot has changed in two years. In 2023, we got pattern matching, which empowers // Equivalent to `final field = obj.field; if (field != null)`
if (obj.field case final field?) {
field.doSomething(); // OK
} And promotion for private fields: if (obj._value != null) {
obj._value.toDouble(); // OK
} Ideally promotion would work for public fields too. But I doubt that this is realistic, given how Dart works. To me, this issue is solved. |
I don't think the pattern matching really helps at all because you're almost always using an early return pattern where you return if null. The alternative-checking if not null and putting the body in the if brackets-tends to be untenable because you end up with unreadable amounts of nesting. Private fields is nice, but also doesn't help in most scenarios. The current place where this most annoys me is with hooks-the code base I'm working in right now is filled with hooks that construct and return a record that includes all nullables like errors, data and "refresh/set" callbacks. Records were used instead of classes because the team considered null type promotion a non negotiable requirement. But now the type signatures and general contract for the hooks are really obtuse. My two cents is that the experience has only improved for a very limited slice of scenarios. |
Early returns are a fair shout out. I wonder if we could have a variant to For example: // !case is the reverse of case
if (obj.field !case final field?) return;
field.doSomething(); |
I often do this, and it's not less readable (I actually think it's more readable than early returns, to have all returns in the same nest level). Most of the time (like 90% at least in my personal experience), you will have only a single level of nesting, not a nesting hell. This is a subjective experience, though. |
if (obj.field !case final field?) return;
field.doSomething(); This is logically impossible to implement if you allow variables introduced in |
I find, in the wild, you often get past one level of nesting and the early return pattern almost immediately gets less nested and easier to parse. FWIW, the current team I'm on feels so strongly they even have a custom linter to enforce the early return pattern. |
I'm not sure about that, but I'm not attached to this exact syntax. This feels reminiscent of the void fn(MyObject obj) {
guard final field? = obj.field else {
return;
}
field.doSomething();
} Or whatever the syntax needs to be. |
Quote: let x = 10
guard let x = someOptional else { // Error: Cannot assign to value of 'x' because it is already declared.
return
} End Quote In dart, there's no such restriction |
Can we get null promotion for nullable fields within a final class? Sort of like we already have with private fields? I think this unlocks 90% of the value (in the Flutter context) without hitting any of the blockers that have come up in this thread so far. Namely:
It would look like this: // Final isn't a problem because you're rarely ever subclassing widgets anyway.
final class FooView extends StatelessWidget {
const FooView({this.foo});
final Foo? foo;
@override
Widget build(BuildContext context) {
if (foo == null) return SizedBox.shrink();
// No `!` needed because from within a final class we would get null promotion on fields.
return Text(foo.bar);
}
}
// In some other file...
final fooView = tester.widget<FooView>(find.byType(FooView));
// Doesn't work. Consumers outside of the class/file see `foo` abstracted as a getter
// so they can't get null promotion.
if (fooView.foo != null) print(fooView.foo.bar); |
It's possible. It does require that all subclasses inside the same library (if any) are also |
@lrhn coming back to this, at Betterment this is one of the limitations of Dart that hurts dev ex the most for us. We have A LOT of named nullable constructor arguments and our build functions are littered with scenarios like: final effectiveFoo = foo;
if (effictveFoo == null) {
return SizedBox.shrink();
}
return Text(foo.bar); We never really have a reason to sub-class I surveyed our Flutter devs for the Dart and Flutter dev ex issues they cared about and had them vote on which one they most wanted fixed and this issue got the most votes-hence why I'm coming back to this now. Does any of this background bump the priority of this issue for you all? |
You can use patterns to bind a tested value: if (foo case final foo?) {
return Text(foo.bar);
}
return SizedBox.shrink(); Works best if the promoted branch comes first. Or it can use a switch: return switch (foo) {
null => return SizedBox.shrink(),
var foo => return Text(foo.bar),
} or the last line could even be: Foo(:var bar) => return Text(bar), There is no current work being done on making more instance variables promotable. If we did go that way, I'd suggest allowing promotion of (in same library only):
|
The cure is worse than the disease here. You save a line, but the result is overwrought and unreadable. |
The constraints you provide here all seem reasonable to me and, if implemented, would address our use case. Patterns and switches don't really help us because:
and (much more importantly than the first two): I appreciate the suggestions, but with real world code I think they'd get strong (and valid) push back in code review for poor readability because of the reasons mentioned above. Here's a real world example (rewritten to protect private source code) that illustrates a not-uncommon level of complexity that would be pretty awkward to handle if we refactored away from early-return syntax: // Just using `this` here to emphasize that these are non-promotable class attributes.
final effectiveOptions = this.options;
final effectiveItemId = this.itemId;
if (effectiveOptions == null) {
return const OptionsLoadingView();
}
if (effectiveItemId == null) return const SizedBox.shrink();
// Note that our data modelling here is a little funky, that's because we're dealing with
// a mid-migration backend so we have to jump through some extra hoops.
final buyOption = effectiveOption.firstWhereOrNull(
(o) => o.optionType == LegacyItemOptions.buy,
);
final secondaryOptions =
effectiveOptions
.where((o) => o.optionType != LegacyItemOptions.buy).toList();
final isMultiButton = buyOption != null && secondaryOptions.isNotEmpty;
return _ButtonRow(
children: [
if (buyOption != null)
BuyButton(itemId: effectiveItemId, isCompact: isMultiButton),
if (secondaryOptions.isNotEmpty)
SecondaryOptionsButton(
itemId: effectiveItemId,
isPrimary: !isMultiButton,
secondaryOptions: secondaryOptions,
),
],
); Out of curiosity, I had ChatGPT rewrite the code snippet once using case syntax and once using switch expressions, and the results are very unreadable, especially the latter. FWIW hand cleaning this up could probably help a bit, but even then it'd still be markedly less readable than the early return pattern: |
@caseycrogers can you make these fields private? Private fields will be promoted. On the other hand, the example you are giving has so much non-trivial code that |
We could make the fields private, but they're all named arguments (they're widgets, so positional arguments are almost never used) and declaring private attributes for named arguments is far too tedious to be practical. This was actually my first intuition for the right solution here. Here's my comment where I outlined different possible approaches to improve Dev Ex for named constructor arguments for private fields: For your comment on complexity/verbosity, my issue with the I've found (and fixed) production code like the following (not actually producing a bug so not high stakes, but illustrates how having redundant references to the same variable in the namespace is confusing): final effectiveFoo = this.foo;
if (effectiveFoo == null) return SixedBox.shrink();
return Column(
children: [
SomeOtherWidget(),
// If check always passes, should just use null promoted `effectiveFoo`.
if (foo case final nonNullFoo?) FooView(foo);
]
); I could reuse the variable name and willfully shadow, but that comes with its own confusion: PS: I hope I don't come off like I'm being willfully difficult here, there are just A LOT of ways to approach this developer need and over the last couple quarters I've put a lot of thought into the different approaches and it's hard to describe upfront off the top of my head all the different constraints/factors here-I'm finding this example/counterexample exercise helpful and hope it is helpful for you all as well |
Uh oh!
There was an error while loading. Please reload this page.
Considering this code:
In the error line,
person.jobTitle
will not be null as it was checked for that before, but the analyzer keeps it with its original signature. Because of that, it is not usable inside theshowInConsole
function.Some workarounds?
!
operator. But I'm not sure about this. Coming from TypeScript, this is a bad practice because if I remove the null check for some reason, it won't show the error and it will be throwing a runtime error.The first workaround is the safest way, I think. But what if I change the type of
cardBrand
in the future? I would need to change it in every location I check for the property to not be null. Also, it could be annoying to create a local variable for every property you need to check.If the analyzer can follow the flow of the first workaround for a local variable so it knows it's not null, shouldn't it do the same for the property of a local variable?
The text was updated successfully, but these errors were encountered: