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

toNullable() specialization. #21

Merged

Conversation

artem-zinnatullin
Copy link
Contributor

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
Contributor

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
Contributor Author

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 :)

Copy link
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
Contributor Author

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
Contributor Author

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

Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants