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

Should type inference infer nullable types where necessary? #630

Closed
stereotype441 opened this issue Oct 18, 2019 · 10 comments
Closed

Should type inference infer nullable types where necessary? #630

stereotype441 opened this issue Oct 18, 2019 · 10 comments
Labels
nnbd NNBD related issues

Comments

@stereotype441
Copy link
Member

Here's some weird code I was playing with today:

main() {
  void Function(int) f = ([i]) {};
}

If I enable NNBD, the analyzer reports the following error:

error • The parameter 'i' can't have a value of 'null' because of its type, so it must either be a required parameter or have a default value.

This is happening because the type of the parameter i is being inferred from the context type void Function(int), so its type is non-nullable int. However, since i is an optional parameter, it must have a nullable type.

Obviously the user could work around this by giving an explicit type:

main() {
  void Function(int) f = ([int? i]) {};
}

But I wonder whether it would be worth modifying the type inference algorithm so that in contexts where a nullable type is required.

@stereotype441 stereotype441 added the nnbd NNBD related issues label Oct 18, 2019
@leafpetersen
Copy link
Member

Interesting suggestion. It seems reasonable to me.

@leafpetersen
Copy link
Member

@lrhn @munificent @eernstg What do you think of this? On further consideration, I have some concerns about the usability of this. On the one hand, it's nice if the user can write:

 void foo(Object Function(int) f) {
    print(f(3));
  }
  f( ([i]) => i ?? 3);  // inferred as `int Function([int?])
 }

but on the other hand it feels like the error message you get for this code might be surprising:

 void foo(Object Function(int) f) {
    print(f(3));
  }
  f( ([i]) => i.isEven);  // Error, `isEven` can't be called on a nullable type.
 }

@lrhn
Copy link
Member

lrhn commented Jan 30, 2020

If we are inferring the type of an optional parameter with no default-value, then we should infer a nullable type. It must be nullable, and being contravariant, making the context type's parameter type nullable will still be valid.

I have no problem with the error message above. The code writes ([i]) which is a clearly optional parameter with no default value, so it should be clear that i may be null.

I do think readers will quickly internalize an association like:

  • required parameter: not nullable (except perhaps in internal APIs)
  • optional parameter with default value: not nullable
  • optional parameter without default value: nullable

Exceptions from that can exist, but they likely frighten and confuse users.

@munificent
Copy link
Member

If the code were:

main() {
  void Function({required int param}) f = ({param}) {};
}

Would we infer param to have type int? or would we infer required? Overall, my five-minute intuition is inferring a nullable type in Paul's original case is a little too much cleverness for too little practical benefit.

Even when extra clever rules like this do the right thing, it's still another rule to learn and sometimes it's better for users for things to be simple and explicit even if the result is more verbose.

@eernstg
Copy link
Member

eernstg commented Jan 31, 2020

We could have a relatively simple rule about inference of the type annotation on a formal parameter p of a function literal: If the context type specifies that p has type T and p is optional with no default, and T is potentially non-nullable, then we infer T? (thus avoiding the compile-time error caused by the absent default value).

But I tend to support @munificent here:

a little too much cleverness for too little practical benefit.

After all, even though it's not a hideously complex rule, it is a new concept that inference would "correct" a parameter type obtained from the context.

Developers would need to have that extra step in mind whenever they are trying to understand what any particular function literal might mean, for instance in the case where there is an error in the body which arises because of that implicitly added ? (such as @leafpetersen's i.isEven).

But if the developer is reasoning about the code in terms of p having type annotation T, the consequences of p actually having type T? could also arise in much more subtle ways, in a different piece of code, much later.

Moreover, this situation will inherently imply that information is lost: The parameter is optional in the function literal, but it is required in the context type, so the parameter is guaranteed to be provided in all calls based on the context type.

So we are protecting the function literal from a behavior (omitting the argument) that the caller can't do based on the given type, there would have to be a dynamic type test:

void foo(Object Function(int) f) {
  if (f is Object Function([Never]) {
    print(f()); // Only now can we omit the argument.
  }
}

void main() {
  foo(([i]) => i.isEven);  // Error, `isEven` can't be called on a nullable type.
}

Of course, it isn't sound to assume that no such dynamic type test will occur. But the fact that the optionality of the parameter is erased by the context type makes me think that it isn't worth the trouble to make it work. It seems more useful to flag the conflict and ask the developer to resolve it, that is, to infer parameter type int and then raise an error because the parameter is optional and non-nullable, but has no default value.

It would be helpful if the diagnostic message about this situation could indicate that one way to eliminate the error is to make the parameter required.

@lrhn
Copy link
Member

lrhn commented Jan 31, 2020

There are two possible approaches.
Either we infer int or int? for i in void Function(int) f = ([i]) {}.

I don't see adding a default value or making the parameter required as options.

The body doesn't matter, we don't use it for parameter inference, so it's not important whether the body contains an i.toRadixString(16) call or an i.toString() call. If we choose int?, the latter will compile and the former will not.

If we infer int, we know that it will give a compile-time error because there is no default value.
If we infer int? the code might compiler. It might have a down-stream compile-time error because of the nullability, but that's an error at that point.

So, inferring int? is the most friendly choice. An optional parameter with no default value must be nullable, we can see the entirely locally, so choosing to make it non-nullable is choosing to introduce a compile-time error.
That's a valid choice, after all it's a function literal which is optional on a parameter which does not need to be optional. The fix is easy - make the parameter non-optional.
It's just a very strict choice that we make for the user. For named parameters, if they forget a required, we make it a guaranteed compile-time error, even though the code could work. (But then, "could work" is not a big guarantee).

So, I think we should infer a nullable type, and I don't think it's confusing to a user if we point out that this optional parameter is nullable.

For the

main() {
  void Function({required int param}) f = ({param}) {};
}

we will not infer required for param. That's not a type, it's not something to infer by type inference.
If we did that, then for consistency we should also be able to remove [...] around optional positional parameters that don't need to be optional. I don't think that's a good idea.

@leafpetersen
Copy link
Member

Created #938 to discuss the required case.

I don't see a firm consensus in the discussion above. @munificent and @eernstg seem somewhat opposed, @lrhn on the fence? Further thoughts on this?

@eernstg
Copy link
Member

eernstg commented Apr 24, 2020

As usual, I'd like to emphasize readability and comprehensibility.

If we allow a program to be a few characters shorter it gets easier to write that program, but this may be a bad trade-off if it's harder to read what it does. Presumably, we'll be reading any given snippet of code more times than we'll write/update it.

void foo(Object Function(int) f) => print(f(3));

void main() {
  foo(([i]) => i.isEven);  // Error, `isEven` can't be called on a nullable type.
}

In particular, if we infer a nullable type annotation for a non-type reason (like: the parameter i has type int according to the context, but it is optional so we infer int? to avoid the error), and the error on i.isEven comes as a complete surprise to the developer, we haven't been very helpful:

If the [...] was included by mistake, we have masked the actual mistake and created some derived errors elsewhere, and it may not be obvious that they should be resolved by removing those brackets. Note that the brackets may very well be a mistake, because foo won't ever know that the parameter is optional, unless it performs an is check which is tailored to determine exactly that.

If [...] was added intentionally then we are already looking at somewhat exotic code. In this case it seems more helpful to me if the exotic code is well documented (using ([int? i]) {}), such that future readers of the code can see the non-standard parameter type (those readers may well know that foo takes a function that takes an int, and they may be puzzled by i?.isEven ?? false).

So I tend to think that inference of T? where the context provides T in order to avoid a non-type error is a net negative for developers.

@munificent
Copy link
Member

I agree with Erik. I don't think a special rule here carries its weight.

In particular, I'm struggling to come up with a real-world example of reasonable code where this would come into play. As I understand it, this only occurs when you have downwards inference of a lambda expression. So you're creating a lambda that is immediately bound to either a parameter or variable. In the type of that variable, the parameter is not optional. You have no other references to the lambda to invoke it through. Why would anyone ever bother to mark the parameter optional then?

@leafpetersen
Copy link
Member

This is not planned.

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

5 participants