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

Nullable ref type annotation fixes to analyzer APIs #43023

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Nullable ref type annotation fixes to analyzer APIs #43023

merged 2 commits into from
Apr 8, 2020

Conversation

AArnott
Copy link
Contributor

@AArnott AArnott commented Apr 2, 2020

The first commit is pretty straightforward as correct.
My second commit might be a bit more controversial, so I kept it separate in case we want to remove it from the branch before merging.

@AArnott AArnott requested a review from a team as a code owner April 2, 2020 17:44
@jcouv jcouv added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels Apr 2, 2020
@AArnott AArnott requested a review from a team as a code owner April 2, 2020 19:48
@sharwell
Copy link
Member

sharwell commented Apr 2, 2020

Why are we allowing messageargs to be null?

@AArnott
Copy link
Contributor Author

AArnott commented Apr 2, 2020

Why are we allowing messageargs to be null?

Roslyn always allowed this (and in some code paths explicitly pass null in to this params argument. But the annotations suggest it's not allowed. I'm fixing the annotations to match the actual supported behavior.

@cston
Copy link
Member

cston commented Apr 3, 2020

@AArnott, it looks like some uses of Diagnostic.Properties are now resulting in nullability warnings.

@AArnott
Copy link
Contributor Author

AArnott commented Apr 3, 2020

I was only checking against Compilers.sln I see in Roslyn.sln there are some warnings. I'll look into it, @cston .

@AArnott
Allow null property values in Diagnostics
0041de4 
This is permitted by the runtime code in prior versions, so the annotations should signify that.
@cston cston requested a review from a team April 3, 2020 19:40
@AArnott
Copy link
Contributor Author

AArnott commented Apr 7, 2020

Any other reviewers before this gets merged?

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 7, 2020
View reviewed changes
src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs
@@ -53,7 +53,7 @@ private void AnalyzeOperation(OperationAnalysisContext context)
!tree.OverlapsHiddenPosition(switchBlock.Span, context.CancellationToken))
{
Debug.Assert(missingCases || missingDefaultCase);
var properties = ImmutableDictionary<string, string>.Empty
var properties = ImmutableDictionary<string, string?>.Empty
Copy link
Member

Choose a reason for hiding this comment

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

hrmm... who passes null for the value. I'd prefer we attempt to disallow that. If you make this non-null, can you point to any violators?

Copy link
Contributor

@mavasani mavasani Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can restrict the values for the property bag to be non-null. I think I have seen some third party analyzers using null as a value for the property bag entry.

jcouv
jcouv reviewed Apr 8, 2020
View reviewed changes
src/Analyzers/Core/CodeFixes/PopulateSwitch/AbstractPopulateSwitchCodeFixProvider.cs
@@ -58,8 +58,8 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
{
var diagnostic = context.Diagnostics.First();
var properties = diagnostic.Properties;
var missingCases = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingCases]);
var missingDefaultCase = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingDefaultCase]);
var missingCases = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingCases]!);
Copy link
Member

Choose a reason for hiding this comment

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

! [](start = 97, length = 1)

We typically reserve the ! suppressions for things that are checked in the current method (directly or via straightforward helper method) but the compiler is unable to track.
For more complicated invariants, we use assertions instead (Debug.Assert(... is object); or is { } depending on preference).
These two (and two more below in this file) strike me as cases for assertions.

https://github.com/dotnet/roslyn/blob/master/docs/contributing/Nullable%20Annotations.md

Copy link
Member

Choose a reason for hiding this comment

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

An assert seems unnecessary here since bool.Parse() will throw an exception if the argument is null.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya. The ! was just to keep the compiler warning quiet so that the original behavior of throwing from bool.Parse would be preserved.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks

jcouv
jcouv approved these changes Apr 8, 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 2) with a nit.

@jcouv jcouv self-assigned this Apr 8, 2020
@AArnott AArnott merged commit e9c15a7 into dotnet:master Apr 8, 2020
@ghost ghost added this to the Next milestone Apr 8, 2020
@AArnott AArnott deleted the nrtFixes branch April 8, 2020 18:14
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@cston cston cston approved these changes

@mavasani mavasani mavasani approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jaredpar jaredpar Awaiting requested review from jaredpar

Assignees

@jcouv jcouv

Labels
Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types
Projects
None yet
Milestone
16.7.P1
Development

Successfully merging this pull request may close these issues.
6 participants
@AArnott @sharwell @cston @CyrusNajmabadi @mavasani @jcouv