Repository navigation
Some widgets' nullOk parameters are inconsistently applied. #68637
Description
The Actions widget has a number of static methods with a nullOk parameter, which can be used to enable or disable the throwing of errors when the caller's requested object was not found.
Turns out, this parameter is only considered when asserts are enabled. This feels a little iffy to me, as the documentation makes no mention of how the contract can change depending on environment.
Request: Improve docs (and maybe the name of the param, since it's sort of a debug only thing), or alternatively normalize behavior between asserts-enabled and disabled builds.
Affected methods:
Actions.findActions.ofActions.handlerActions.invoke
Thanks!
Activity
gspencergoog commentedon Oct 20, 2020
The name of the parameter is consistent across many different similar static methods, not just
Actions, and has the same behavior there (e.g.MediaQuery.of,Focus.of,Localizations.localeOf, etc.). And yes, it only applies to builds with assertions on, but that was always its intent.I agree that the behavior should be mentioned more explicitly in the docs.
shyndman commentedon Oct 20, 2020
I took a look at all of those, and it appears they are designed to throw regardless of whether asserts are enabled (although
Localizations.localeOfmight throw an NPE as opposed to a well-formedFlutterError).gspencergoog commentedon Oct 20, 2020
Ahh, I see what you mean. OK, I can see making those throw a
FlutterErroron a non-assert build.We seem to actually be quite inconsistent with our methods that take
nullOkas a parameter, perhaps we should try and normalize that across the framework.shyndman commentedon Oct 20, 2020
Couldn't hurt. :)
Thanks!
[-]Actions widget's nullOk parameters only considered with asserts enabled[/-][+]Some widgets' nullOk parameters are inconsistently applied.[/+]gspencergoog commentedon Oct 20, 2020
I'm converting this issue over to cover all the static methods that take a
nullOkparameter.We have a bunch of static member functions that have a
nullOkargument to them, but we're really inconsistent with how we apply it. Sometimes it will return null in some circumstances even ifnullOkis false (e.g.CupertinoDynamicColor.resolve()), sometimes it only works when asserts are enabled (e.g.Actions.find), and other times it throws aFlutterErrorin release mode (e.g.MediaQuery.of()). Sometimes I think the checks are assert-only because of performance, though, so we can't just make them all throw FlutterError all the time. At the least, we should probably update the API docs for each one that describes what behavior is expected.shyndman commentedon Oct 20, 2020
Thanks Greg.
When the time comes, I'd love to be CC'd on that PR if you get a chance.
gspencergoog commentedon Oct 20, 2020
Sure, will do.
gspencergoog commentedon Oct 20, 2020
Related: #67163
gspencergoog commentedon Oct 23, 2020
Here's a design doc for doing this: https://flutter.dev/go/eliminating-nullok-paramters (I know, I'm fixing the name typo in the go link).
gspencergoog commentedon Oct 23, 2020
FYI, in discussions about this, @Hixie was of the opinion (and I agree) that eventually the
FlutteErrorexceptions here should be thrown only from inside of asserts, since we eventually want to remove debug messages from release builds for size reasons (and because they're not really actionable there anyhow).shyndman commentedon Oct 26, 2020
Makes sense. Would exceptions without messages be thrown in the non-assert case?
gspencergoog commentedon Nov 5, 2020
@shyndman Yes, and I added wording to that effect in the docs.
gspencergoog commentedon Dec 4, 2020
This is all done, except for
Localizations.localeOf, since a client (flutter_svg) is still using it. Once dnfield/flutter_svg#443 lands, and then we remove the nullOk fromLocalizations.localeOf, this issue can be closed.shyndman commentedon Dec 4, 2020
Thanks!
It's one of those things where a few people will never realize the thanks they owe — the bug they didn't have to track down. :)
goderbauer commentedon Jan 25, 2021
There's one more nullOK param remaining after all changes linked from this bug are submitted: https://master-api.flutter.dev/flutter/widgets/Actions/invoke.html
gspencergoog commentedon Jan 25, 2021
I'll fix the
Actions.invokenullOkbefore closing.shyndman commentedon Jan 26, 2021
Thanks Greg
github-actions commentedon Aug 6, 2021
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.