Skip to content

Model nullability with Option<T> #331

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
kennykerr opened this issue Sep 25, 2020 · 6 comments
Closed

Model nullability with Option<T> #331

kennykerr opened this issue Sep 25, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@kennykerr
Copy link
Collaborator

kennykerr commented Sep 25, 2020

Right now any WinRT interface/class/delegate types (reference types) can store a null pointer value. We should change that so that a null pointer value is not a valid state and the developer has to use Option<T> to express that. While this is more syntactically verbose in some cases, at least when you hold a T you know it's valid and can use it directly with no runtime or syntactic overhead. WinRT class constructors (T::new() in Rust) will simply return Result<T> but all other return/out params of reference types will simply return Result<Option<T>> on the assumption that it could be null. This avoids the confusion around whether a given value may be used or whether the is_null check should be performed, which is not idiomatic in Rust.

Beyond constructors, we should be able to avoid the extra Option<T> for activatable factory interfaces since they are exclusively used to implement constructors.

@kennykerr kennykerr added the enhancement New feature or request label Sep 25, 2020
@kennykerr kennykerr changed the title Model nullability as Option Model nullability with Option<T> Sep 25, 2020
@Boddlnagg
Copy link

For reference, in winrt-rust we also made the same change after a while: contextfree/winrt-rust#54
It did make dealing with return values more cumbersome, because now you always have to "unwrap" twice ...

Probably, many methods never actually return null, but there's no way to know that. Maybe at some point additional metadata could be added, so the Option<> wrapper could be elided fer such methods ...? Just dreaming ;-)

@Boddlnagg
Copy link

By the way, in winrt-rust we also added an exception for the Windows.Foundation.IAsync... types because I guess it would really be a bug if such a method would return null. The user is expected to await it, and await doesn't work on null. The question remains what to do if it actually does happen (because some kind of safety check needs to be there) ... it probably would have to panic!.

@kennykerr
Copy link
Collaborator Author

Thanks Patrick, double unwrap is certainly a bit gross. Yes, .NET has the NotNull attribute. We could introduce such an attribute for WinRT, but the challenge is always getting any adoption for such an attribute on the many existing APIs.

I do like the idea of excluding certain common patterns, like constructors and async.

@kennykerr
Copy link
Collaborator Author

Here's the attribute proposal: microsoft/xlang#708

@kennykerr
Copy link
Collaborator Author

Looks like the attribute may not be practical, but I think we can grow the number of return types that are, heuristically, not-null to the following:

  • Constructors
  • Functions that return async interfaces
  • Functions that return collection interfaces

@kennykerr
Copy link
Collaborator Author

#354 fixes this both in modeling nullability with Option<T> and avoiding the double unwrap by folding the degenerate case into the result's Err value so that WinRT APIs simply return Result<T> rather than Result<Option<T>>.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants