Skip to content

toNullable() specialization.#21

Merged
artem-zinnatullin merged 3 commits intogojuno:masterfrom
artem-zinnatullin:az/toNullable-specialization
Jun 11, 2018
Merged

toNullable() specialization.#21
artem-zinnatullin merged 3 commits intogojuno:masterfrom
artem-zinnatullin:az/toNullable-specialization

Conversation

@artem-zinnatullin
Copy link
Copy Markdown
Contributor

@artem-zinnatullin artem-zinnatullin commented May 21, 2018

Recent PR #20 made me rethink toNullable(), I think it needs to be abstract with specialized implementation in Some an None.

That gives following benefits:

  • No instanceof check, likely JIT or AOT were specializing the function for us, but now we do it at compile time
  • Proper static nullabily in Some.toNullable(). It lets compiler statically figure nullability if type is known to be Some

Note that because of nullability change in Some.toNullable() the change is API incompatible, but ABI compatible so we're still allowed to release it in a minor update.

cc @Egorand, @AlecStrong

@AlecKazakova
Copy link
Copy Markdown

AlecKazakova commented May 21, 2018

interesting...i wonder if this would mean component1 on None would have proper type Nothing? without having to make component1 abstract

@artem-zinnatullin
Copy link
Copy Markdown
Contributor Author

artem-zinnatullin commented May 21, 2018

Without abstract component1 None won't have component1 on its own, it's not a data class right

With abstract component1 however we're getting proper static nullability in both Some and None because it's declared as nullable in Optional so it works well for None and then compiler adds @NotNull to Some's override so it sees it statically as non-null. I mentioned that in #20 (comment) but maybe wasn't clear enough :)

Egorand
Egorand approved these changes May 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Having Some.toNullable() return T rather than T? is definitely a win. As for component1(), I don't think it makes sense in None, so it should be specific to Some.

@artem-zinnatullin
Copy link
Copy Markdown
Contributor Author

artem-zinnatullin commented May 21, 2018

As for component1(), I don't think it makes sense in None, so it should be specific to Some.

But #20 adds component1 to None (we can continue this discussion there)

@artem-zinnatullin
Copy link
Copy Markdown
Contributor Author

artem-zinnatullin commented May 31, 2018

@ming13 @dmitry-novikov @nostra13 can you PTAL ыы?

arturdryomov
arturdryomov approved these changes Jun 11, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@arturdryomov arturdryomov left a comment

Choose a reason for hiding this comment

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

Looks good, let’s try it out.

@artem-zinnatullin artem-zinnatullin merged commit 71b0934 into gojuno:master Jun 11, 2018
@artem-zinnatullin artem-zinnatullin deleted the az/toNullable-specialization branch June 11, 2018 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arturdryomov arturdryomov arturdryomov approved these changes

@nostra13 nostra13 Awaiting requested review from nostra13

@dmitry-novikov dmitry-novikov Awaiting requested review from dmitry-novikov

+1 more reviewer

@Egorand Egorand Egorand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@artem-zinnatullin @AlecKazakova @arturdryomov @Egorand