Skip to content

Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type #48803

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
Merged
merged 9 commits into from
Nov 6, 2020
Merged

Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type #48803

merged 9 commits into from
Nov 6, 2020

Conversation

cston
Copy link
Contributor

@cston cston commented Oct 21, 2020 •
edited
Loading

Report warnings when assigning default to, or when casting a possibly null value to a type parameter type not constrained to value types or reference types.

T t = default; // warning: may be null

object? o = ...;
t = (T)o; // warning: may be null

Fixes #46044

@cston cston marked this pull request as ready for review October 24, 2020 17:44
@cston cston requested review from a team as code owners October 24, 2020 17:44
@jcouv jcouv added the Feature - Nullable Reference Types Nullable Reference Types label Oct 25, 2020
@RikkiGibson RikkiGibson self-assigned this Oct 27, 2020
RikkiGibson
RikkiGibson reviewed Oct 27, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
@@ -138978,18 +139418,30 @@ static T F1<T>(T x1)
}";
var comp = CreateCompilation(source, parseOptions: TestOptions.Regular9);
comp.VerifyDiagnostics(
// (7,14): warning CS8600: Converting null literal or possible null value to non-nullable type.
Copy link
Member

Choose a reason for hiding this comment

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

It occurs to me that this message may not be quite right. T is a possibly non-nullable type. It is not known to be non-nullable. However, this may not end up being very disruptive/confusing to users in practice.

RikkiGibson
RikkiGibson reviewed Oct 27, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Outdated
[InlineData("where T : notnull")]
[InlineData("where T : I")]
[InlineData("where T : I?")]
public void UnconstrainedTypeParameter_44(string constraint)
Copy link
Member

Choose a reason for hiding this comment

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

nit: test is named UnconstrainedTypeParameter, but it is about testing a variety of type parameter constraints.

RikkiGibson
RikkiGibson reviewed Oct 27, 2020
View reviewed changes
RikkiGibson
RikkiGibson approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@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.

LGTM, just had some minor feedback/suggestions

@cston cston mentioned this pull request Oct 27, 2020
Merged
@runfoapp runfoapp bot mentioned this pull request Oct 29, 2020
Closed
@cston cston requested a review from a team November 5, 2020 06:44
@jcouv
Copy link
Member

jcouv commented Nov 5, 2020 •
edited by cston
Loading 

static void M2<T>(T t, dynamic? d)

Consider adding T? flavors of this test (which would not have the diagnostics) #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:2753 in 80c60b3. [](commit_id = 80c60b3, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Nov 5, 2020 •
edited by cston
Loading

", MaybeNullAttributeDefinition }, parseOptions: TestOptions.Regular8);

Consider adding C# 9 flavor of this test (ie. no warnings) #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:25825 in 80c60b3. [](commit_id = 80c60b3, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Nov 5, 2020 •
edited
Loading

record Rec1(T t = default) // 1, 2

Should we have a single diagnostic here instead? It seems that both warnings are related to the same problem (assigning default to T) #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68235 in 80c60b3. [](commit_id = 80c60b3, deletion_comment = False)

jcouv
jcouv reviewed Nov 5, 2020
View reviewed changes
...edUtilitiesAndExtensions/Compiler/Core/Options/EditorConfig/EditorConfigStorageLocation`1.cs
else
{
typedValue = (T)value;
typedValue = (T)value!;
Copy link
Member

@jcouv jcouv Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How do we know value isn't null here (seems like it could be)? #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.

This change just moves the ! from the return statement to here.

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

@jcouv
Copy link
Member

jcouv commented Nov 5, 2020 •
edited
Loading

Since we're adding a nullable warning, it'll be good to do a validation on runtime and/or VS.
Instructions for runtime repo are here: https://github.com/dotnet/roslyn/blob/master/docs/contributing/Building%2C%20Debugging%2C%20and%20Testing%20on%20Windows.md#testing-on-the-dotnetruntime-repo
We have instructions for VS somewhere else (onenote) #Closed

jcouv
jcouv reviewed Nov 5, 2020
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 8)

@jcouv jcouv self-assigned this Nov 5, 2020
@RikkiGibson
Copy link
Member

RikkiGibson commented Nov 5, 2020 •
edited by jcouv
Loading

Since we're adding a nullable warning, it'll be good to do a validation on runtime and/or VS.

VS validation: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/283878.

  • note that while the PR has a ❌ for CloudBuild, it is just because the build was marked expired due to being too long ago. The build did pass.

dotnet/runtime#43925 resolves new runtime warnings resulting from this change #Closed

@cston
Copy link
Contributor Author

cston commented Nov 6, 2020 •
edited
Loading

record Rec1(T t = default) // 1, 2

The warnings are from separate expressions: the second warning ("CS8600: Converting null literal") is from assigning default to the parameter t; the first warning ("CS8601: Possible null reference assignment") is assigning the parameter t to the field t.

The behavior is the same for reference types: see sharplab.io.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68235 in 80c60b3. [](commit_id = 80c60b3, deletion_comment = False)

@RikkiGibson
Copy link
Member

RikkiGibson commented Nov 6, 2020 •
edited by jcouv
Loading

record Rec1(T t = default) // 1, 2

The warnings are from separate expressions: the second warning ("CS8600: Converting null literal") is from assigning default to the parameter t; the first warning ("CS8601: Possible null reference assignment") is assigning the parameter t to the field t.

The behavior is the same for reference types: see sharplab.io.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68235 in 80c60b3. [](commit_id = 80c60b3, deletion_comment = False)

IIRC, once we stop updating parameter state based on the default value, it will turn into one warning. #Closed

jcouv
jcouv approved these changes Nov 6, 2020
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 9)

@cston cston merged commit ebc69ed into dotnet:master Nov 6, 2020
@ghost ghost added this to the Next milestone Nov 6, 2020
@cston cston deleted the 46044 branch November 6, 2020 19:25
@cston cston modified the milestones: Next, 16.9.P2 Nov 6, 2020
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 9, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
06dc743 
* upstream/master: (519 commits)
  Remove workaround in PEMethodSymbol.ExplicitInterfaceImplementations (dotnet#49246)
  Enable LSP pull model diagnostic for XAML. (dotnet#49145)
  Update dependencies from https://github.com/dotnet/roslyn build 20201109.8 (dotnet#49240)
  Add test for with expression in F1 help service (dotnet#49236)
  Cache RegexPatternDetector per compilation
  Fix RazorRemoteHostClient to maintain binary back-compat
  Further tweak inline hints
  Fix MemberNames API for records (dotnet#49138)
  Minor cleanups (dotnet#49204)
  Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type (dotnet#48803)
  Clean up of EditorFeatures.Cocoa.Snippets (dotnet#49188)
  Fix OK button state handling. Make relation between viewmodels more tightly coupled
  Extend make type abstract to include records (dotnet#48227)
  Remove duplicated implementations of C# event hookup
  Add select all/deselect all buttons
  Consolidate conditional compilation (dotnet#49150)
  Implement IEquatable on Microsoft.Cci.DefinitionWithLocation structure (dotnet#49162)
  Add new document extensions file
  Unify implementations
  Only disable structure tagger provider in LSP scenario
  ...
@jiangzeng01 jiangzeng01 mentioned this pull request Feb 7, 2021
Closed
jmarolf added a commit to jmarolf/assert.xunit that referenced this pull request Feb 16, 2021
@jmarolf
Fix nullable cast
af258f2 
In a future version of the compiler, this cast is a warning (csc version 3.10, change introduce [here](dotnet/roslyn#48803))

This resolves the warning for this case.
@jmarolf jmarolf mentioned this pull request Feb 16, 2021
Merged
bradwilson pushed a commit to xunit/assert.xunit that referenced this pull request Feb 20, 2021
@jmarolf
Fix nullable cast (#46)
352c424 
In a future version of the compiler, this cast is a warning (csc version 3.10, change introduce [here](dotnet/roslyn#48803))

This resolves the warning for this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types
Projects
None yet
Milestone
16.9.P2
Development

Successfully merging this pull request may close these issues.

No warning reported for assignment or explicit cast of possibly null value of unconstrained type parameter type
4 participants
@cston @jcouv @RikkiGibson @Dotnet-GitSync-Bot