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

Let null contribute its nullability to method type re-inference #43571

Closed
wants to merge 1 commit into from

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 22, 2020

The scenario is Infer(s, null); with void Infer<T>(T t1, T t2). We currently infer string! but want to infer string? instead, so that no warnings are produced.
The problem is that when we re-infer type arguments with nullability, typeless expressions (like the null stripped of its conversions) do not contribute its type or nullability.

Alternatives considered:

  1. If we had a solution for typeless expressions to contribute to method type inference, I would prefer that, but in the meantime, I propose a solution scoped to only affects nullability analysis: when we strip conversions off of arguments, we leave the conversion that gives null its type.
  2. Another possible approach which avoid removing most conversions is what we do for default, switch and other expressions which get a different bound node once target-typed.

Fixes #43536

@jcouv jcouv added this to the 16.7 milestone Apr 22, 2020
@jcouv jcouv self-assigned this Apr 22, 2020
@jcouv jcouv marked this pull request as ready for review April 24, 2020 21:59
@jcouv jcouv requested a review from a team as a code owner April 24, 2020 21:59
@jcouv jcouv marked this pull request as draft April 24, 2020 22:22
@jcouv jcouv force-pushed the nullable-inference branch from a0126ab to 7b66c17 Compare April 25, 2020 19:12
@jcouv jcouv marked this pull request as ready for review April 27, 2020 05:07
var source = @"
class C
{
void M(string s)
Copy link
Member

Choose a reason for hiding this comment

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

string s [](start = 11, length = 8)

s is not used.


public readonly struct Optional<T>
{
public Optional(T value) => throw null!;
Copy link
Member

Choose a reason for hiding this comment

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

public Optional(T value) => throw null!; [](start = 4, length = 40)

Not used.


if (avoidTypelessExpressions &&
conversion.ConversionKind == ConversionKind.ImplicitReference &&
conversion.Operand is BoundLiteral { Type: null })
Copy link
Member

Choose a reason for hiding this comment

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

It feels like the current behavior of null is expected: we pass an untyped BoundLiteral to MethodTypeInferrer. But the behavior of default is unexpected: we pass a typed BoundDefaultExpression to MethodTypeInferrer where the type was only determined after running MethodTypeInferrer in initial binding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, you're saying we should keep the current behavior for null (Infer("", null) would infer string! and warn, rather than string?)?

Copy link
Member

@cston cston Apr 27, 2020

Choose a reason for hiding this comment

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

Not necessarily. I'm suggesting the arguments passed to MethodTypeInferrer in NullableWalker should match the typed/untyped-ness of the arguments passed to MethodTypeInferrer in initial binding. So null and default would both be passed as untyped arguments. Could NullableWalker fix up the nullability of the inferred type arguments after MethodTypeInferrer returns? Such an approach would also need to handle lambda return types so it might get complicated.


In reply to: 416076359 [](ancestors = 416076359)

@jcouv jcouv marked this pull request as draft April 27, 2020 20:03
@jcouv jcouv closed this May 12, 2020
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.

Type inference handles null and default for reference types differently
2 participants