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

Don't show nullability analysis for value types or constant values #37965

Merged
merged 3 commits into from Aug 14, 2019

Conversation

@ryzngard
Copy link
Contributor

commented Aug 13, 2019

Fixes #37157

@ryzngard ryzngard requested a review from jasonmalinowski Aug 13, 2019

@ryzngard ryzngard requested a review from dotnet/roslyn-ide as a code owner Aug 13, 2019

@@ -88,6 +88,17 @@ protected override ImmutableArray<TaggedText> TryGetNullabilityAnalysis(Workspac

var typeInfo = semanticModel.GetTypeInfo(bindableParent, cancellationToken);

if (typeInfo.Type.IsValueType)

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Aug 13, 2019

Contributor

how do you know .Type is not null?

This comment has been minimized.

Copy link
@ryzngard

ryzngard Aug 13, 2019

Author Contributor

It's not called Type? 😋

It could be, I supposed. I assumed with the kind check above it might be alright, but that's an educated guess and not a fact.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Aug 13, 2019

Contributor

hrmm............ you raise a good point. It may be the case that iif you have any of those symbols and its any of the kinds checked that you will always have a type...

However, i would not risk it. GetTypeInfo absolutely returns null, and i don't think there's a guaranteed invariant between the two that you can trust.

And yes, it should be Type? :)

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 14, 2019

Member

And if you're wondering @CyrusNajmabadi yes we're chatting about beginning to annotate the compiler APIs as well...

switch (symbolInfo.Symbol)
{
case IFieldSymbol { HasConstantValue: true }: return default;
case ILocalSymbol { HasConstantValue: true }: return default;

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Aug 13, 2019

Contributor

nifty!

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Aug 13, 2019

Contributor

@alrz want to right a feature that looks for things like case IField field when field.HasConstantValue: and convert to this form? Or cases of patterns like is Whatever w && <additional checks on w>?

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 13, 2019

Member

Actually that form was what @ryzngard was trying to write but we couldn't remember then "when" syntax... :-)

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Aug 13, 2019

Contributor

haha. i like this better!

This comment has been minimized.

Copy link
@alrz

alrz Aug 14, 2019

Contributor

case IField field when field.HasConstantValue:

#32386 works on when as well as other expressions.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented on 499890d Aug 14, 2019

👍

@@ -86,8 +86,19 @@ protected override ImmutableArray<TaggedText> TryGetNullabilityAnalysis(Workspac
return default;
}

switch (symbolInfo.Symbol)

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 14, 2019

Member

I wonder if it's worth combining this with the large if block on line 79.

var typeInfo = semanticModel.GetTypeInfo(bindableParent, cancellationToken);

if (typeInfo.Type?.IsValueType == true)

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Aug 14, 2019

Member

Consider sticking a comment to explain why (since we think it's more noise in this case...)

@jasonmalinowski
Copy link
Member

left a comment

Only comments are entirely ignorable.

@ryzngard ryzngard merged commit 1cc7305 into dotnet:master Aug 14, 2019

20 checks passed

WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20190814.38 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (Linux_Test mono) Linux_Test mono succeeded
Details
roslyn-CI (MacOs_Test) MacOs_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-integration-CI Build #20190814.38 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration debug_legacy) VS_Integration debug_legacy succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
roslyn-integration-CI (VS_Integration release_legacy) VS_Integration release_legacy succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.