Enable `trust_return_value_nullability` #2791

cgwalters · 2022-12-01T13:15:59Z

Enable trust_return_value_nullability

Several commonly used APIs currently unnecessarily return
Option<T>, like ostree_deployment_get_csum().

Flip the gir flag for this to on; I think our annotations are
correct.

Add docs and fix annotations for ostree-repo-file.c

The code here is not great, embarassing we've gone this
long without docs for some of these public API functions too.

I think this is right though.

openshift-ci · 2022-12-01T13:16:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cgwalters · 2022-12-01T13:16:54Z

From chat on Matrix:

yes, you need to set (nullable) on those
I believe (not nullable) is useless and should be dropped as an annotation
the problem is a lot of APIs lack the nullable annotations, that is why the default is to assume everything returned by the C apis is nullable
so you have to carefully review all the API changes

So basically we need to go over the API surfaces affected here and choose whether they do need nullable.

rust-bindings/src/auto/bootconfig_parser.rs

cgwalters · 2023-03-14T21:37:50Z

OK rebased this on #2814 and lifting draft.

jlebon

Looks sane to me, though I didn't review the nullability of all the APIs.

jlebon · 2023-03-15T14:24:17Z

What's the behaviour if an API we thought was not nullable actually is? Does the Rust code panic on encountering the null?

cgwalters · 2023-03-15T17:36:37Z

Does the Rust code panic on encountering the null?

Hmm I was going to say yes, but apparently the answer is no?
https://docs.rs/glib/0.17.5/src/glib/shared.rs.html#550
That's not great...
I guess we could try to add enable_trust_but_verify_return_value_nullability = true to change debug_assert! -> assert!?

jlebon · 2023-03-15T18:52:20Z

Does the Rust code panic on encountering the null?

Hmm I was going to say yes, but apparently the answer is no? docs.rs/glib/0.17.5/src/glib/shared.rs.html#550 That's not great... I guess we could try to add enable_trust_but_verify_return_value_nullability = true to change debug_assert! -> assert!?

Could be interesting. I guess it depends how much we trust the current annotations to be correct? I think they should be too, but I haven't had to pay close attention to them before. I wouldn't be surprised if there's at least one or two that needs tweaking.

It doesn't look like there's that many APIs being changed, so I'm happy to split it up and sanity-check their nullability if you'd like. Otherwise, I'd vote to just ship this and do any fixups as follow-ups. At least it's early in the development cycle.

Several commonly used APIs currently unnecessarily return `Option<T>`, like `ostree_deployment_get_csum()`. Flip the gir flag for this to on; I think our annotations are correct.

The code here is not great, embarassing we've gone this long without docs for some of these public API functions too. I think this is right though.

Since we did a bunch of API changes due to nullability and other introspection cleanups.

- commit parents are optional - remote URLs are optional

Unfortunately, the nullability of the output value here is dependent on whether the `default_value` parameter is provided. There's no way to express this in introspection or Rust.

To pick up the latest introspection changes.

cgwalters · 2023-03-17T12:28:08Z

OK. I did a full audit and found a few more APIs that did need nullable. I think this is now good to go! It will be a nice ergonomic improvement in some cases, biggest one is Sysroot::repo() now correctly just giving Repo.

This adapts to the changes from ostreedev/ostree#2791 in particular.

cgwalters · 2023-03-17T14:36:15Z

Yep this made things better ostreedev/ostree-rs-ext#467

tintou · 2023-03-17T15:36:06Z

src/libostree/ostree-repo.c

@@ -982,7 +982,7 @@ _ostree_repo_remote_name_is_file (const char *remote_name)
 * underneath that group, or @default_value if the remote exists but not the
 * option name.  If an error is returned, @out_value will be set to %NULL.
 *
- * Returns: %TRUE on success, otherwise %FALSE with @error set
+ * Returns: (nullable): %TRUE on success, otherwise %FALSE with @error set


This can't be null as it is a gboolean

Right, I messed this up; PR incoming

This adapts to the changes from ostreedev/ostree#2791 in particular.

openshift-ci bot added the do-not-merge/work-in-progress label Dec 1, 2022

bilelmoussaoui reviewed Dec 1, 2022

View reviewed changes

rust-bindings/src/auto/bootconfig_parser.rs Outdated Show resolved Hide resolved

cgwalters mentioned this pull request Feb 9, 2023

libostree: Enhance the annotation coverage #2814

Merged

openshift-merge-robot added the needs-rebase label Mar 11, 2023

cgwalters force-pushed the enable-trust-return-nullability branch from 0f84f80 to 47c1ae7 Compare March 14, 2023 21:37

openshift-merge-robot removed the needs-rebase label Mar 14, 2023

cgwalters marked this pull request as ready for review March 14, 2023 21:37

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 14, 2023

cgwalters force-pushed the enable-trust-return-nullability branch 3 times, most recently from c499526 to b3780e8 Compare March 14, 2023 23:51

jlebon approved these changes Mar 15, 2023

View reviewed changes

cgwalters added 6 commits March 17, 2023 08:13

Enable trust_return_value_nullability

351d9ff

Several commonly used APIs currently unnecessarily return `Option<T>`, like `ostree_deployment_get_csum()`. Flip the gir flag for this to on; I think our annotations are correct.

Add docs and fix annotations for ostree-repo-file.c

e962c2f

The code here is not great, embarassing we've gone this long without docs for some of these public API functions too. I think this is right though.

rust: Bump semver

3f27cff

Since we did a bunch of API changes due to nullability and other introspection cleanups.

lib: Fix two nullable annotations

cbec85f

- commit parents are optional - remote URLs are optional

repo: Fix nullability for remote options

c4d03d2

Unfortunately, the nullability of the output value here is dependent on whether the `default_value` parameter is provided. There's no way to express this in introspection or Rust.

rust: Renerate bindings

61a4a83

To pick up the latest introspection changes.

cgwalters force-pushed the enable-trust-return-nullability branch from b3780e8 to 61a4a83 Compare March 17, 2023 12:27

jlebon approved these changes Mar 17, 2023

View reviewed changes

jlebon enabled auto-merge March 17, 2023 13:22

jlebon merged commit 7e93837 into ostreedev:main Mar 17, 2023

cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request Mar 17, 2023

Update to ostree 0.18

3a2ef11

This adapts to the changes from ostreedev/ostree#2791 in particular.

cgwalters mentioned this pull request Mar 17, 2023

Update to ostree 0.18 ostreedev/ostree-rs-ext#467

Merged

tintou reviewed Mar 17, 2023

View reviewed changes

cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request May 1, 2023

Update to ostree 0.18

9f905f3

This adapts to the changes from ostreedev/ostree#2791 in particular.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `trust_return_value_nullability` #2791

Enable `trust_return_value_nullability` #2791

cgwalters commented Dec 1, 2022 •

edited

Loading

openshift-ci bot commented Dec 1, 2022

cgwalters commented Dec 1, 2022

cgwalters commented Mar 14, 2023

jlebon left a comment

jlebon commented Mar 15, 2023

cgwalters commented Mar 15, 2023

jlebon commented Mar 15, 2023

cgwalters commented Mar 17, 2023

cgwalters commented Mar 17, 2023

tintou Mar 17, 2023

cgwalters Mar 17, 2023

cgwalters Mar 17, 2023

Enable trust_return_value_nullability #2791

Enable trust_return_value_nullability #2791

Conversation

cgwalters commented Dec 1, 2022 • edited Loading

openshift-ci bot commented Dec 1, 2022

cgwalters commented Dec 1, 2022

cgwalters commented Mar 14, 2023

jlebon left a comment

Choose a reason for hiding this comment

jlebon commented Mar 15, 2023

cgwalters commented Mar 15, 2023

jlebon commented Mar 15, 2023

cgwalters commented Mar 17, 2023

cgwalters commented Mar 17, 2023

tintou Mar 17, 2023

Choose a reason for hiding this comment

cgwalters Mar 17, 2023

Choose a reason for hiding this comment

cgwalters Mar 17, 2023

Choose a reason for hiding this comment

Enable `trust_return_value_nullability` #2791

Enable `trust_return_value_nullability` #2791

cgwalters commented Dec 1, 2022 •

edited

Loading