-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Future.catchError is error prone #34144
Comments
And sorry, some questions here:
|
One interesting way we could roll this out "incrementally" would be to add a flag that:
This way code with the flag could use code without the flag*, and its not a breaking change (for those that implement the Future API) without the flag. This could be a new annotation We could then use it internally, though unless we can make code without this flag use code with it, any published internal code or code used by published internal code (which, I suppose, transitively, is published) would not be able to use it. *it would be an error for code with the flag to import a library that implements Future without the flag, though |
Funny enough I had a similar request here: #30811:
I think the main objection was adding |
That makes sense. +1 to default methods! |
Re.: Rolling out changes incrementally. (That, or add overloading). There is no way to change the current behavior in a non-breaking way without such a feature (or interface default methods, or extension methods, or any other way to add methods to an interface without breaking existing implementations), so unless we get that first, this cannot be changed until Dart 3, and even then it's unlikely that we can change the interface of |
Ping. I think this needs to be addressed in some reasonable way, and we can't wait years to do it. |
I'm all for fixing this, but I don't actually see a possible way to do so right now. We cannot change the The only solution I see is to introduce language features which make some currently breaking API changes into non-breaking changes, e.g., interface default members for adding new members, but definitely not restricted to that. Maybe proper type-based overloading would allow us to have both full signatures, and potentially still allow existing subclasses to implement both signatures with their current single implementation. In any case, that'll be a larger feature and will take time to design and implement. I do want to make such language features a priority, precisely because (As a side not, if we had had |
I can think of at least one option, though it hurts terseness a bit temporarily:
If we wanted to add them as instance methods in 3.0, then we'll need to dance a bit (add new methods to the approximately ~100-200 sub-classes of I'd sign up for work inside of Google and Flutter on this one.
It might not be too late, at least on some of them ( For |
We could also add a lint, i.e. |
is it possible to add a static warning/lint for the specific incorrect usage without changing the implementation in the SDK? Can we hardcode some SDK apis within analyzer to have it treat them slightly differently than they are written. |
I think there was a proposal from @bwilkerson (or @pq?) for a poor-man's union type. For example, something like this: Future<S> catchError<S>(@Union2<S Function(Object), S Function(Object, StackTrace)> Function onError) Unfortunately there was a lot of opposition, and it would need to modify the SDK :( |
Future<T> onErrorOnly<T>(Future<T> future, FutureOr<T> handler(Object error),
[bool test(Object error)]) => future.catchError(handler, test);
Future<T> onError<T>(Future<T> future, FutureOr<T> handler(Object error, StackTrace stack),
[bool test(Object error])) => future.catchError(handler, test); That's definitely doable, but not user friendly. I'd rather wait for extension methods or interface default methods than start adding top-level static helper functions for existing more-usable methods, and then try to migrate people to the less usable version. It also fails to have any of the improvements that I'd actually want on Future<T> onError<E>(FutureOr<T> handler(E error, StackTrace stack), [bool test(E error)]) ... That would allow simple type checks to be handled without a test-argument, and even infer the type, so: future.onError((StateError e) { ... if (e.something) return null; }); would automatically only catch state errors. Going to an intermediate "solution" which is less usable that what we have, and not getting any of the potential advantages either, that seems like a lot of work for little effect, and it won't help all the other functions that have similar issues. More interesting solution would be to allow It would still be breaking to change Future<T> catchError(FutureOr<T> Function handler, [bool test(Object error)]) ... because sub-classes are not implementing it, though :( (My point about sealing |
Are we expected to get either of those in an upcoming release (read: soon)? If not, I don't think its fair to make all current and especially new users of Dart and Flutter suffer because hypothetical features might come out at some point in time.
I don't think its less usable just because, as you write:
That is, not having improvements you want doesn't make it less usable. This is a serious usability issue. I realize you might not write 1000s of lines of application Dart code, so I want to make it clear from users that do, this is a super thorny issue and not easily solvable unless you have a lot of intimate knowledge of Dart, the type system, and core libraries. I did propose a couple of alternatives:
I do not think it is reasonable to say we can't make any improvements at all, anywhere. |
I'm sure we can make some improvements. This is filed as a library issue, and I'm saying I don't think we can make meaningful library improvements with the current Dart feature-set. I don't think static helper functions are viable in practice. Maybe inside Google where we can force people to use them and poterntially fix all the existing code, but for everybody else, there is no discoverability and bad ergonomics. An analyzer special-case is a much more promising. That was what the analyzer did for |
In my opinion this should be our focus. @bwilkerson - can you weigh in on the feasibility? |
Ping @bwilkerson @pq thoughts? |
Sorry, I missed this issue somehow. If we want to flag either all uses of |
I think flag:
|
I would take a shot at starting this but I don't know much about the analyzer code base or where I would write such a check. If there are any pointers or docs I could give it a shot or at least a first pass. |
Are we talking about a hint (on by default for everyone) or a lint (opt in)? |
I imagine a hint (or even a warning if the spec can allow it), given that I don't think any developer wants to add code that will 100% of the time throw a import 'dart:async';
void main() {
final name = Future.value('Brian W.');
name.catchError((int foo) => foo);
} This code will never succeed at runtime. The only reason it doesn't fail statically is because the language lacks a way to express that static check (either union types, overloads, etc). Given that it's part of the core (I mean, |
@lrhn @leafpetersen - would extension methods give us a non-breaking way to do this? |
Extension methods would change the discoverability and ergonomics here. We can't change the |
@aadilmaan - we should mark this as blocked by extension methods and do it once we have the support. It's a P1 ask for google and flutter. |
Is this a request to add an extension method to the platform libraries? I guess it would be something like: extension FutureExt<T> on Future<T> {
Future<T> onError<E>(FutureOr<T> handleError(E error, StackTrace? stack),
[bool test(E error)?]) async {
try {
return await this;
} on E catch (e, s) {
if (test == null || test(e)) {
return handleError(e, s);
}
rethrow;
}
} Then again, anyone can write this extension, it doens't have to be in the platform libraries. (I'd love if that was the |
Yes. Adding it to the platform would allow us to mark |
How does one deprecate an instance method in favor of an extension method? |
This would have other benefits too. Something that's been coming up a lot in VS Code lately is that the debugger can't tell when an exception is "caught" if it uses I've had lots of workarounds suggested, but they all introduce other issues (for example automatically resuming from exceptions - but that would happen for real-uncaught errors too, in which case you may as well just turn off breaking on exceptions). I think encouraging (I filed an issue in the linter repo here -> dart-lang/linter#2212) |
Ping. Now that we have extension methods, do we want to revisit this? If not, is this still a P1? |
I do want to do something, like https://dart-review.googlesource.com/c/sdk/+/151512. |
(Forked from an internal thread; @munificent and others have additional context)
@munificent writes, in response:
You can see a minimal example of this problem in action in the following code sample:
API reference for
<Future>.catchError
:https://api.dartlang.org/stable/2.0.0/dart-async/Future/catchError.html
EDIT: I believe there is another bug, somewhere, about the hidden errors you get if you write:
The text was updated successfully, but these errors were encountered: