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

Add nullability info field into discard expression #66309

Merged
merged 10 commits into from Feb 23, 2023

Conversation

TomatorCZ
Copy link
Contributor

Regards the issue and PR.

Problem
Different behavior of discard expression and local variable in out arguments during inferring type argument of generic method.

using System.Diagnostics.CodeAnalysis;

#nullable enable

interface IOperation<T> { }
class StringOperation : IOperation<string?> { }

class C {
    void M() {
        Test(new StringOperation(), out string? discard1); // 1: no warning
        Test(new StringOperation(), out string? _); // 2: CS8620
        Test(new StringOperation(), out var _); // 2: CS8620
        Test(new StringOperation(), out _); // 3: CS8620
    }

    void Test<T>(IOperation<T> operation, [MaybeNull] out T result) => result = default;
}

Reason

BoundDiscardExpression doesn't contain information about nullability resulting in losing information about it between binding and the inference. See code.

Solution

I added field holding examined nullability info.

@TomatorCZ TomatorCZ requested a review from a team as a code owner January 7, 2023 12:34
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 7, 2023
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 2)

@jcouv jcouv self-assigned this Jan 9, 2023
@jcouv jcouv added this to the 17.6 milestone Jan 9, 2023
@RikkiGibson RikkiGibson self-assigned this Jan 13, 2023
@TomatorCZ
Copy link
Contributor Author

TomatorCZ commented Jan 14, 2023

I added an indicator if the user defined type of discard expression argument(out _). Then, I consider the argument to be nullable if an user defined it as nullable, or if he omitted the type(e.g. out _, out var _, out string? _). Otherwise I respect user's intention to make the discard non-nullable(e.g. out string _).

@jcouv jcouv dismissed their stale review February 9, 2023 17:56

As Rikki pointed out, I read the flag in reverse :-/

@TomatorCZ
Copy link
Contributor Author

Done with fixing the review pass

@jaredpar jaredpar closed this Feb 21, 2023
@jaredpar jaredpar reopened this Feb 21, 2023
@jaredpar
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@@ -377,7 +377,7 @@ private BoundExpression SetInferredType(BoundExpression expression, TypeSymbol t
{
var pending = (BoundDiscardExpression)expression;
Debug.Assert((object?)pending.Type == null);
return pending.SetInferredTypeWithAnnotations(TypeWithAnnotations.Create(type));
return pending.SetInferredTypeWithAnnotations(TypeWithAnnotations.Create(true, type, true));
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't clear to me why we don't just want to call TypeWithAnnotations.Create(type) here. An oblivious annotation seems fine here, just as we have on DeconstructionVariablePendingInference above.

});
}

private static void checkDiscard(SemanticModel model, DiscardDesignationSyntax discard, string type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming these to CheckDiscard to match conventions elsewhere in the project.

var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
comp.VerifyDiagnostics(new[] {
// (25,77): warning CS8601: Possible null reference assignment.
// static void TestA<T>(IOperation<T> operation, out T result) => result = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider assigning default! or similar since these warnings aren't really the interesting part of the test.

Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

Done review pass. Looking in good shape to me, just a few small things to address.

@TomatorCZ
Copy link
Contributor Author

Done with fixing review pass

@RikkiGibson
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@RikkiGibson
Copy link
Contributor

@jcouv for a second review

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 10)

@RikkiGibson RikkiGibson merged commit 3a37d24 into dotnet:main Feb 23, 2023
@ghost ghost modified the milestones: 17.6, Next Feb 23, 2023
@RikkiGibson
Copy link
Contributor

Thanks for the contribution @TomatorCZ!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants