Remove the nullable wrappers from the Workspaces layer #40030

jasonmalinowski · 2019-11-27T01:18:54Z

The compiler merged support for top-level symbol nullability in #39498, so we can now delete our own wrappers that were performing the same thing. This is a mostly mechanical change. Commit-by-commit might be slightly more helpful as the similar types of changes were grouped together.

Closes #39737
Fixes #36093
Fixes #39693 (or was fixed by compiler work, unsure)
Closes #36099
Fixes #38047

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions_CreateEqualsMethod.cs

src/Workspaces/Core/Portable/Utilities/NullableHelpers/NullableExtensions.cs

src/Features/CSharp/Portable/Completion/CompletionProviders/ObjectCreationCompletionProvider.cs

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

...table/GenerateMember/GenerateParameterizedMember/CSharpGenerateParameterizedMemberService.cs

src/Features/CSharp/Portable/SignatureHelp/TupleConstructionSignatureHelpProvider.cs

src/Features/Core/Portable/GenerateType/AbstractGenerateTypeService.State.cs

src/Workspaces/Core/Portable/CodeGeneration/CodeGenerationSymbolFactory.cs

src/Workspaces/Core/Portable/Shared/Extensions/SemanticModelExtensions.cs

src/Workspaces/Core/Portable/CodeGeneration/CodeGenerationSymbolFactory.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationTypeSymbol.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractNamedTypeSymbol.cs

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

…ted base Our constructed type symbol was holding onto an abstract type symbol; this allowed a constructed type to be created atop another constructed type, which would form a chain. That isn't really necessary, and doesn't match the usual mental models. This switches to holding onto a stronger type, and then updates the Construct methods to go back to the prior ConstructedFrom in order to avoid creating a chain.

… symbols This wasn't necessary when we had wrapped symbols, since it was the wrapper that had the annotation. Now that we are removing wrappers we need to implement it directly.

The compiler merged support for top-level symbol nullability in dotnet#39498, so we can now delete our own wrappers that were performing the same thing. This is a mostly mechanical change. For places where we were calling LookupSymbols or ClassifyConversion, those places previously called .WithoutNullability() since the compiler API would have thrown if we gave it our wrappers, but the APIs had otherwise no way to pass top-leve nullability. That was an accepted oversight at the time, and the belief is passing in the full symbol will generally be more correct.

In the deepest of ironies, the extension methods we had to wrap top-level nullability was implicitly hiding a potential null reference. Removing the extension methods is now flagging this, so this fixes it up. The handling isn't ideal: if we don't have a Task<T> we won't necessarily wrap, but that's not something really worth going out of our way to support -- most code doesn't.

This clarifies why we are stripping the nullability, and replaces it with the "not null" instead of "oblivious" which generates to the same code but is a bit more correct if something else tried to operate on the type symbol.

…ctly We weren't ensuring the the tuple elements had top-level nullability if we inferred the pattern was itself using nullable.

jasonmalinowski self-assigned this Nov 27, 2019

jasonmalinowski requested a review from ryzngard November 27, 2019 01:41

jasonmalinowski force-pushed the remove-nullable-wrappers branch from d5102d1 to fbed819 Compare November 27, 2019 01:57

CyrusNajmabadi approved these changes Nov 27, 2019

View reviewed changes

jasonmalinowski force-pushed the remove-nullable-wrappers branch 2 times, most recently from 8473c3a to d3e3d35 Compare November 27, 2019 22:11

This was referenced Nov 28, 2019

GetTypeInfo returns with nullability but the ITypeSymbol.NullableAnnotation is None #40051

Closed

TypeInfo.ConvertedType.NullableAnnotation is None when binding a 'var' keyword #40052

Closed

jasonmalinowski force-pushed the remove-nullable-wrappers branch 6 times, most recently from bc605e2 to 3af5fa3 Compare December 3, 2019 18:17

jasonmalinowski marked this pull request as ready for review December 3, 2019 18:18

jasonmalinowski requested a review from a team as a code owner December 3, 2019 18:18

jasonmalinowski mentioned this pull request Dec 3, 2019

CreateTupleTypeSymbol requires you to pass the element nullabilities explicitly #40105

Closed