Skip to content
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

Deprecate Optional #672

Merged
merged 1 commit into from Nov 16, 2022
Merged

Deprecate Optional #672

merged 1 commit into from Nov 16, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Nov 6, 2020

With the introduction of non-null by default in Dart SDK 2.12, existing
users should migrate to non-nullable types. This type will be removed in
Quiver 4.0.0.

See: #666

@cbracken cbracken requested a review from yjbanov November 6, 2020 20:06
@google-cla google-cla bot added the cla: yes @googlebot is happy with the CLA status of this PR label Nov 6, 2020
@cbracken
Copy link
Member Author

cbracken commented Nov 6, 2020

/cc @davidmorgan

@cbracken
Copy link
Member Author

cbracken commented Nov 7, 2020

From offline discussion with @yjbanov, we may want to delay landing this until packages like built_* have been migrated off Optional to avoid filling downstream users' analysis with deprecation warnings.

What I may do is update this to simply update the docs to suggest people avoid adding new dependencies on it, then add the attribute once the major downstream packages have been migrated off.

@davidmorgan
Copy link
Contributor

FWIW built_ never used/recommended Optional, in anticipation of null safety :)

@cbracken
Copy link
Member Author

cbracken commented Nov 9, 2020

Oh - fantastic news!

Copy link
Member

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cbracken
Copy link
Member Author

I've rebased to tip of tree. Once NNBD support is enabled across the board internally, I'll:

  1. Do a small handful of manual migrations to identify any unforseen issues
  2. Write up a short migration HOWTO doc
  3. If it looks feasible, use the large-scale change process to notify everyone, perform the migration, then regret having signed up for it when I get to the final, painful 5% that hits every edge-case I missed in step 1. :)

@modulovalue
Copy link

then regret having signed up for it when I get to the final, painful 5% that hits every edge-case I missed in step 1. :)

Null safe types are not a replacement for an Optional type.
You can't nest nullables, but you can nest Optionals.
i.e. int?? is not allowed in Dart, but Optional<Optional<int>> is.

Optional has a reason to exist.
Think of e.g. an API call that returns a nullable bool. To model its absence an Optional type would be necessary.

I just randomly stumbled across this PR, don't mind me, but you might want to check for that edge-case or decide to keep Optionals :)

@cbracken
Copy link
Member Author

cbracken commented Feb 8, 2021

Think of e.g. an API call that returns a nullable bool. To model its absence an Optional type would be necessary.

@modulovalue Can you explain a bit further? bool, int, etc. in Dart (prior to NNBD) are all nullable types. To model a nullable bool post-NNBD, you'd type it as bool? instead. I may be misunderstanding what you've said above though.

@hoc081098
Copy link

Map<String, Optional<User>> cache; // user by id

// there 3 cases:
cache['some_id'] == null; // -> does not contains in cache
cache['some_id]'.isPresent == true; // -> cache contains a User
cache['some_id'].isAbsent == true; // -> User not exists

Sent from my Redmi 7A using FastHub

@cbracken
Copy link
Member Author

cbracken commented Feb 8, 2021

@hoc081098 you can store a null value in maps today.

Map<String, User?> cache;
cache['some_id'] == null; // -> unknown whether there is a mapping for 'some_id'
cache.containsKey('some_id') == false; // -> not contained in cache
cache.containsKey('some_id') == true; // -> cache contains a mapping for key 'some_id'

In general, it is unsafe to assume that cache[key] == null is equivalent to cache.containsKey(key), since maps allow null values.

@modulovalue
Copy link

@cbracken
The following is valid Swift code:

let i: Int????? = nil // the variable i has the quiver-equivalent type Optional<Optional<Optional<...int>>>

But that above is not expressible in dart without a wrapper

int???? i = null; // compile time error

To simulate the way that nulls work in swift, users would have to declare an Optional wrapper themselves.

In other words, the Optional type allows users to define a nullable nullable value.

An example where that could be useful can be seen here https://github.com/dart-lang/sdk/blob/8432b379e7fd2fae1e872ca20f9275366d49ffe0/pkg/vm_service/lib/src/dart_io_extensions.dart#L184-L185
The vm_service api has collapsed their union types into single types where values that can be null can be absent. (Optional<int?> would be the best way to model that since int?? is not valid dart)

@hoc081098
Copy link

hoc081098 commented Feb 8, 2021

I think nested optional type is a good point to keep Optional 😃.
If we want to keep backward compatibility, make T extends Object.

@modulovalue
Copy link

@cbracken You can ignore my comments, I was wrong. I assumed Optional behaved more like a Maybe/Option Monad but it doesn't at all.

This was referenced Oct 14, 2021
@cbracken cbracken closed this May 3, 2022
@cbracken cbracken reopened this May 3, 2022
@cbracken
Copy link
Member Author

Based on comments in b/258898406 internally, I'm going to rebase and land this.

With the introduction of non-null by default in Dart SDK 2.12, existing
users should migrate to non-nullable types. This type will be removed in
Quiver 4.0.0.
@cbracken cbracken merged commit d0fe5a2 into google:master Nov 16, 2022
@cbracken cbracken deleted the deprecate-optional branch November 16, 2022 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes @googlebot is happy with the CLA status of this PR cleanup nnbd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants