mirror of
https://github.com/flutter/flutter.git
synced 2026-01-19 12:12:02 +08:00
0b34640871
Flutter is built on top of GLib, and GLib's standard error mechanism
uses a structure `GError` and a function `g_set_error`. This function
takes a pointer to a pointer, setting it to a new `GError` for the outer
caller to receive. The model is designed to capture the first error that
occurs; as such, `g_set_error` checks if the supplied (by reference)
pointer is _already_ pointing at something. If it is, then it simply
emits a warning about this new error and discards it, assuming that
`error` already points at a presumably more-pertinent `GError`.
This semantic is sensible, but it isn't automatically obvious. It makes
sense when a function is calling multiple other functions, each of which
could return an error, ensuring that the error that gets returned is the
first one that happened. But, if you're only calling one function and
you don't have prior exposure to this design, it might not be
intuitively obvious that it has this intentional behaviour. Instead, the
`GError** error` parameter just looks like any old `out` parameter. That
can lead to code being written that doesn't bother to initialize the
error variable; after all, if no error occurs, it's irrelevant, and if
an error _does_ occur, then the function will overwrite it anyway,
right? (This latter being the incorrect assumption.)
```
// INCORRECT CODE
GError *error;
if (!fl_method_call_respond_success(method_call, result_value, &error)) {
g_warning("Failed to send Flutter Platform Channel response: %s", error->message);
g_error_free(error);
}
```
GLib handles the fact that this isn't automatically obvious to someone
with no prior experience with straightforward documentation:
> GLib ▶ set_error
>
> Does nothing if `err` is `NULL`; **if `err` is non-`NULL`, then `*err`
must be `NULL`**. A new `GError` is created and assigned to `*err`.
Since this is, ultimately, the function used internally by Flutter API
functions to return errors through this channel, Flutter effectively has
this same semantic. As such:
```
// CORRECT CODE
GError *error = nullptr;
if (!fl_method_call_respond_success(method_call, result_value, &error)) {
g_warning("Failed to send Flutter Platform Channel response: %s", error->message);
g_error_free(error);
}
```
(edited per @robert-ancell) In addition, `g_autoptr` can be used to make
a smart pointer that automatically ensures that the error, if any, is
not leaked.
```
// CORRECTER CODE
g_autoptr(GError) error = nullptr;
if (!fl_method_call_respond_success(method_call, result_value, &error)) {
g_warning("Failed to send Flutter Platform Channel response: %s", error->message);
}
```
Flutter should make a similar mention in its documentation so that
people who are just discovering the system don't make the same mistake I
did. 🙂
This PR updates the inline documentation comments for public functions
that take `GError** error` parameters to warn concisely about this
situation. Note that it is not required (at least by `g_set_error`) that
they poin at `NULL`, specifically, just that they not be
_uninitialized_.
## Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.
---------
Co-authored-by: Robert Ancell <robert.ancell@canonical.com>