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

Need clarification on type inference in migrated libraries when importing legacy libraries #348

Closed
stereotype441 opened this issue May 10, 2019 · 4 comments
Labels
nnbd NNBD related issues

Comments

@stereotype441
Copy link
Member

The NNBD spec pull request (#293) contains the following text:

When static checking is done in a migrated library, types which are imported
from unmigrated libraries are seen as legacy types. However, for the purposes
of type inference in migrated libraries, types imported from unmigrated
libraries shall be treated as non-nullable. As a result, legacy types will
never appear as type annotations in migrated libraries, nor will they appear in
reified positions.

I'm trying to understand how to interpret this paragraph in light of the fact that type inference and static checking are interleaved.

For example:

// In a legacy library
List<int> f() { ... }

// In a migrated library
List<T> g<T>(List<T> l) { ... }
main() {
  f()..add(null); // (A)
  g(f()..add(null))..add(null); // (B)
  g(f()..add(null))..add(0); // (C)
}

To analyze (A), the analyzer goes through the following steps:
(1) Figure out the type of f() (List<int*>*)
(2) Figure out the argument type expected by f()..add (int*)
(3) Check whether Null is assignable to that type (it is, so there is no error at (A))

To analyze (B), the steps are as above, and then:
(4) Perform type inference to determine the implicit type argument passed to g
(5) Figure out the argument type expected by g(f()..add(null)).add
(6) Check whether Null is assignable to that type

Based on the spec text quoted above, I believe (4) is supposed to infer a type argument of int (as opposed to int*). Meaning that (5) determines that g(f()..add(null)).add requires an int, so the assignability check at (6) will fail, and there will be an error at (B).

But how does this happen? The spec text says "for the purposes of type inference in migrated libraries, types imported from unmigrated libraries shall be treated as non-nullable", which seems to imply that back in step (1), we should have gotten a type of List<int>. But then step (3) would have found an error at (A), which I don't think we want.

I'm wondering if the rule we really want is something like this:

When static checking is done in a migrated library, types which are imported
from unmigrated libraries are seen as legacy types. However, when type inference
is performed in migrated libraries, types are converted to non-nullable types
(or potentially non-nullable types, in the case of type parameters) at
the point where the inference occurs. As a result, legacy types will
never appear as type annotations in migrated libraries, nor will they appear in
reified positions.

I believe this would imply that all the calls to f()..add(null) in the example above are ok, and all the gs are type inferred as g<int>; therefore (A) and (C) are ok, but there's an error at (B).

@stereotype441 stereotype441 added the nnbd NNBD related issues label May 10, 2019
@stereotype441
Copy link
Member Author

CC @bwilkerson

@leafpetersen
Copy link
Member

I agree with this interpretation, and I see how the text is unclear. I will update it. The intention is that when missing type annotations or type parameters are filled in by inference in a migrated library, the legacy annotations are erased from those types. Does that agree with your interpretation?

@stereotype441
Copy link
Member Author

I agree with this interpretation, and I see how the text is unclear. I will update it. The intention is that when missing type annotations or type parameters are filled in by inference in a migrated library, the legacy annotations are erased from those types. Does that agree with your interpretation?

Yes, that agrees with my interpretation, thanks!

@leafpetersen
Copy link
Member

This has been clarified in the spec.

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

No branches or pull requests

2 participants