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

Allow untyped methods to be called from typed methods #48

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

ptarjan
Copy link
Contributor

@ptarjan ptarjan commented Sep 13, 2017

I'm sure this one will be contentious so I didn't go through and fix all the failing tests before discussing. It solves the problem I raised in #37 .

We need a way to migrate code into RDL and forcing it all to be atomic won't work for our multi-million line codebase. This is the same solution we used for Hack when migrating (we used an any type that is the union of (top and bottom) but I'm not sure why). In this PR I used the top type for parameters and the bottom type for return types. I assume we'll have to do something for blocks as well, but I haven't run into that just yet.

@jeffrey-s-foster
Copy link
Contributor

I'm fine with adding this sort of flexibility, I just wonder what support is needed to gradually move from calling untyped code to adding types for that code. For example, should rdl keep track of what untyped code is being called during type checking so that it can spit that information out later, so the developer can go through that list and add missing types? Or is there something else that would help?

@ptarjan
Copy link
Contributor Author

ptarjan commented Sep 15, 2017

Great news! The thing we did with Hack (which I worked on a bit at Facebook) was having a per-file pragma // strict which would tell the typechecker to force all calls in this file to be to only typed code. So during the migration most people wouldn't put that and just locally type things as they saw fit, then we slowly bubbled that pragma up through the code following the dependency graph until it became the default. It could be a per-function annotation too if that sounds better to you.

Also, what do you think about instead of returning %bot to return a new type like %dynamic which allows "anything goes" rules on it. We were thinking that it shouldn't be directly written by programmers like Array<%dynamic> since that makes unsounds things easy like Array<Integer> is a subtype of Array<%dynamic> which is a subtype of Array<String>, but only used by the typechecker to allow these types of calls to be infectious. That would make #44 not needed and instead switch it to operating on %dynamic.

@ptarjan ptarjan changed the base branch from master to dev September 15, 2017 21:11
@ptarjan ptarjan mentioned this pull request Sep 18, 2017
@jeffrey-s-foster
Copy link
Contributor

Does this ability need to be separate from allowing %dynamic, or should it be controlled by the same flag?

@ptarjan
Copy link
Contributor Author

ptarjan commented Oct 15, 2017

I think it could be the same. Basically we need a migration mode while our code is partially typed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants