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.

Sign up for GitHub

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

Jump to bottom

Allow null literals to contribute nullability to type inference #56022

Merged
merged 7 commits into from
Sep 9, 2021
Merged

Allow null literals to contribute nullability to type inference #56022

merged 7 commits into from
Sep 9, 2021

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Aug 30, 2021 •
edited
Loading

Closes #43536

We skip removing conversions from call arguments when the conversion is predefined and applied to a null literal. Note that other kinds of constant-null expressions we do expect to have a natural type, so we do remove their conversion.

Changed approach to instead add special "nullable bounds" to method type inference per #56022 (comment). Will update the PR title prior to merge to avoid messing up email threading.

@RikkiGibson RikkiGibson marked this pull request as ready for review August 31, 2021 15:08
@RikkiGibson RikkiGibson requested a review from a team as a code owner August 31, 2021 15:08
@jcouv
Copy link
Member

jcouv commented Aug 31, 2021 •
edited
Loading

From our offline chat, I'm not sure that's the right approach. I'd suggest reading through https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/nullable-reference-types-specification.md#generic-type-inference
Then it feels like the solution would be to collect a bound that doesn't contribute a type but does contribute an annotation, whereas today if the argument doesn't have a type then it contributes no bound (see MakeExplicitParameterTypeInferences).

In reply to: 909400174

jcouv
jcouv reviewed Aug 31, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 3)

@jcouv jcouv self-assigned this Aug 31, 2021
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@RikkiGibson
Copy link
Contributor Author

@jaredpar FYI I tried building dotnet/runtime with this change and I wasn't able to observe any new nullable warnings. I wasn't able to build everything because of weird variant of dotnet/runtime#42397 but I was able to build ~427 C# projects successfully.

jcouv
jcouv reviewed Sep 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
{
var ordinal = ((TypeParameterSymbol)target.Type).Ordinal;
var typeWithAnnotations = _extensions.GetTypeWithAnnotations(argument);
_nullableAnnotationLowerBounds[ordinal] = _nullableAnnotationLowerBounds[ordinal].Join(typeWithAnnotations.NullableAnnotation);
Copy link
Member

@jcouv jcouv Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Given that we're now only using Join and the starting values are NotAnnotated. I'm wonder if we should just store a boolean. I'll leave that for consideration for you and second reviewer. I don't feel strongly (seems equivalent). #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't feel strongly about it. This did inspire some additional tests for 'null!' and 'null' in a disabled context, so thanks :)

jcouv
jcouv approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for a second review.

@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

1 similar comment
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@RikkiGibson RikkiGibson changed the title Preserve conversions on null literals before nullable type argument reinference Allow null literals to contribute nullability to type inference Sep 9, 2021
@RikkiGibson RikkiGibson merged commit 861b427 into dotnet:main Sep 9, 2021
@ghost ghost added this to the Next milestone Sep 9, 2021
@RikkiGibson RikkiGibson deleted the null-type-arg-inference branch September 9, 2021 22:00
@Cosifne Cosifne modified the milestones: Next, 17.0.P5 Sep 27, 2021
@jcouv jcouv mentioned this pull request Sep 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types
Projects
None yet
Milestone
17.0.P5
Development

Successfully merging this pull request may close these issues.

Type inference handles null and default for reference types differently
4 participants
@RikkiGibson @jcouv @cston @Cosifne