Enable nullable reference types for the repo #3021

mavasani · 2019-11-08T00:43:26Z

I have broken down the changes to each analyzer package in different commits.
I haven't enable nullable for test projects in this PR
I have added few suppressions or nullable disable at source file level for bunch of security analyzers. @dotpaul will address these in a follow-up PR. You can search for #pramga warning disable CS or comments with TODO(dotpaul): Enable nullable analysis.

NOTE: PR targets 2.9.x branch. Once it flows into master, I will add remaining annotations in new code in master.

…ntire solution

src/Utilities/Compiler/Extensions/ISymbolExtensions.cs

src/Microsoft.CodeAnalysis.Analyzers/CSharp/CSharpImmutableObjectMethodAnalyzer.cs

src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/DiagnosticAnalyzerAttributeAnalyzer.cs

...ft.CodeQuality.Analyzers/CSharp/QualityGuidelines/CSharpUseLiteralsWhereAppropriate.Fixer.cs

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotUseInsecureDeserializerWithoutBinderBase.cs

...CodeQuality.Analyzers/Core/ApiDesignGuidelines/CollectionsShouldImplementGenericInterface.cs

src/Microsoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/DoNotDirectlyAwaitATask.cs

....CodeQuality.Analyzers/Core/ApiDesignGuidelines/DoNotRaiseExceptionsInUnexpectedLocations.cs

...Microsoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/OverrideMethodsOnComparableTypes.cs

...icrosoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/TypeNamesShouldNotMatchNamespaces.cs

sharwell · 2019-11-08T01:40:51Z

src/Microsoft.CodeQuality.Analyzers/Core/QualityGuidelines/UseLiteralsWhereAppropriate.cs

@@ -61,6 +61,7 @@ public override void Initialize(AnalysisContext analysisContext)
                var lastField = fieldInitializer?.InitializedFields.LastOrDefault();
                var fieldInitializerValue = fieldInitializer?.Value;
                if (fieldInitializerValue == null ||
+                    lastField == null ||


💭 Could this one have been a bug? Maybe try to add a test to hit this case?

Avoiding adding unit tests in this PR. I will leave this comment unresolved and file a followup bug to track any such cases.

...Microsoft.NetCore.Analyzers/Core/Performance/UsePropertyInsteadOfCountMethodWhenAvailable.cs

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotReferSelfInSerializableClass.cs

src/Utilities/FlowAnalysis/SingleThreadedConcurrentDictionary.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/ThrownExceptionInfo.cs

src/Utilities/Compiler/Extensions/ITypeSymbolExtensions.cs

src/Utilities/Compiler/Options/CategorizedAnalyzerConfigOptions.cs

src/Utilities/Compiler/Options/SymbolNamesOption.cs

src/Tools/ReleaseNotesUtil/Program.cs

src/Tools/ReleaseNotesUtil/FixerExtensions.cs

dotpaul · 2019-11-08T02:29:20Z

                                    && !sslProtocolsSymbol.Equals(variableInitializerOperation.Value.Type))

Change this to:

if (variableInitializerOperation.Value == null
    || !sslProtocolsSymbol.Equals(variableInitializerOperation.Value.Type))

and then can get rid of the ? for valueOperation.

(I think that makes more sense anyway. Thanks nullable reference!)

Refers to: src/Microsoft.NetCore.Analyzers/Core/Security/SslProtocolsAnalyzer.cs:127 in 64aa452. [](commit_id = 64aa452, deletion_comment = False)

dotpaul

🚢 security analyzers look fine to me

sharwell

📝 Now at 212/322 files viewed

src/Microsoft.CodeAnalysis.Analyzers/CSharp/CSharpImmutableObjectMethodAnalyzer.cs

src/Microsoft.NetCore.Analyzers/CSharp/Runtime/CSharpUseOrdinalStringComparison.Fixer.cs

src/Utilities/Compiler/RoslynString.cs

src/Utilities/Compiler/Options/EditorConfigParser.cs

src/Utilities/Compiler/Extensions/ISymbolExtensions.cs

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotSerializeTypeWithPointerFields.cs

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotDisableHttpClientCRLCheck.cs

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotCallDangerousMethodsInDeserialization.cs

sharwell · 2019-11-15T15:57:47Z

src/Microsoft.NetCore.Analyzers/Core/Runtime/UseOrdinalStringComparison.Fixer.cs

        internal static SyntaxNode CreateOrdinalMemberAccess(SyntaxGenerator generator, SemanticModel model)
        {
-            INamedTypeSymbol stringComparisonType = model.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringComparison);
+            INamedTypeSymbol stringComparisonType = model.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringComparison)!;


💭 This is a fascinating case. We have several situations where the code fix relies on the analyzer behavior to ensure null will not be encountered. No request for a change here but eventually interested in seeing if the situation could be improved.

Yes, totally agree. We need a follow-up PR to clean this up across the repo.

src/Microsoft.CodeQuality.Analyzers/Core/QualityGuidelines/AbstractRemoveEmptyFinalizers.cs

sharwell

Now up to 248/322.

src/Microsoft.NetCore.Analyzers/Core/Runtime/DoNotPassLiteralsAsLocalizedParameters.cs

sharwell · 2019-11-15T16:15:52Z

src/Microsoft.NetCore.Analyzers/Core/Runtime/DoNotPassLiteralsAsLocalizedParameters.cs

@@ -144,7 +144,7 @@ void AnalyzeArgument(IParameterSymbol parameter, IPropertySymbol containingPrope
                                }

                                // FxCop compat: Filter out xml string literals.
-                                var filteredStrings = stringLiteralValues.Where(literal => !LooksLikeXmlTag(literal));
+                                IEnumerable<string> filteredStrings = stringLiteralValues.Where(literal => literal != null && !LooksLikeXmlTag(literal))!;


💭 Was the lack of a null check here a bug in the analyzer? Any idea how we might have hit that case?

Not a big fan of the suppression here, but WhereNotNull would be an additional allocation. Perhaps we're on a code path where it doesn't matter anyway so I'll leave the suggestion here:

Suggested change

IEnumerable<string> filteredStrings = stringLiteralValues.Where(literal => literal != null && !LooksLikeXmlTag(literal))!;

IEnumerable<string> filteredStrings = stringLiteralValues.WhereNotNull().Where(literal => !LooksLikeXmlTag(literal));

Yep, it was a bug. I will keep this comment unresolved so we can track adding a regression test for this.

src/Microsoft.NetCore.Analyzers/Core/Runtime/SpecifyIFormatProvider.cs

sharwell · 2019-11-15T16:24:24Z

src/Microsoft.NetCore.Analyzers/Core/Security/SslProtocolsAnalyzer.cs

-                                    if (variableInitializerOperation.Value != null
-                                        && !sslProtocolsSymbol.Equals(variableInitializerOperation.Value.Type))
+                                    if (variableInitializerOperation.Value == null
+                                        || !sslProtocolsSymbol.Equals(variableInitializerOperation.Value.Type))


😕 Was this a bug? Should we have a test covering this condition?

Yes, will add this comment to set of items that needs a regression test.

src/Utilities/Compiler/WellKnownTypeProvider.cs

src/Utilities/Compiler/PooledObjects/ArrayBuilder.cs

src/Microsoft.NetCore.Analyzers/Core/Runtime/DisposeObjectsBeforeLosingScope.cs

src/Microsoft.NetCore.Analyzers/Core/Runtime/DisposableFieldsShouldBeDisposed.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/PredicatedAnalysisData.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/CacheBasedEquatable.cs

...ilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAbstractValue.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMap.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DictionaryAnalysisData.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AbstractLocation.cs

...ilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAbstractValue.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AddressSharedEntitiesProvider.cs

...ities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityFactory.cs

...ysis/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs

...lowAnalysis/FlowAnalysis/Analysis/ParameterValidationAnalysis/ParameterValidationAnalysis.cs

...alidationAnalysis/ParameterValidationAnalysis.ParameterValidationDataFlowOperationVisitor.cs

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/DisposeAnalysis/DisposeAnalysisHelper.cs

mavasani · 2019-11-16T00:46:55Z

Ah, I accidentally fetched in sources from master :(. I will clean this up and update the PR over the weekend.

…ypes

mavasani · 2019-11-16T02:25:57Z

The branch is now back to sane state. @sharwell Please let me know if you have any more feedback before I merge the PR. I will file a separate work item for adding tests for unresolved comments.

mavasani · 2019-11-18T18:10:54Z

@sharwell Any concerns in me merging the PR?

@dotpaul I presume you want me to wait for #3050 to go in before this PR is merged, so that 2.9.8 release is unaffected by this change?

dotpaul · 2019-11-18T18:48:33Z

@dotpaul I presume you want me to wait for #3050 to go in before this PR is merged, so that 2.9.8 release is unaffected by this change?

Yes please.

sharwell · 2019-11-22T15:44:13Z

src/Microsoft.NetCore.Analyzers/CSharp/Runtime/CSharpUseOrdinalStringComparison.Fixer.cs

            {
                SemanticModel model = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
-                if (model.GetSymbolInfo((IdentifierNameSyntax)identifier, cancellationToken).Symbol is IMethodSymbol methodSymbol && CanAddStringComparison(methodSymbol, model))
+                if (model.GetSymbolInfo((IdentifierNameSyntax)identifier!, cancellationToken).Symbol is IMethodSymbol methodSymbol && CanAddStringComparison(methodSymbol, model))


📝 This suppression is not supposed to be needed. If it's not compiling for you with this removed, a compiler bug should be filed.

mavasani · 2019-11-25T20:38:20Z

@dotnet-bot retest this please

…ypes

mavasani added 21 commits November 7, 2019 09:42

Enable nullable on repo

e29e3a1

Annotate Utilities with nullable annotation

a5b448a

Restrict nullable enable to only shipping projects for now.

b424db6

Nullable annotations in Metrics project

6398f5f

Nullable annotations for Roslyn.Diagnostics.Analyzers

05bdcfa

Nullable annotations for Microsoft.CodeAnalysis.Analyzers

abecfb2

Nullable annotations for BannedApiAnalyzers

73aaa1b

Nullable annotations for PublicApiAnalyzers

bb439cb

Disable nullable for MetaCompilation.Analyzers

6f36f66

Nullable annotations for PerformanceSensitiveAnalyzers

e1afae9

Nullable annotation for internal flow analysis utilties

669b773

Nullable annotations for Microsoft.CodeQuality.Analyzers

1c344c2

Some more nullable annotations for utilities

a2531f3

Additional nullable annotations to a code quality analyzer

9090966

Add nullable annotations for Microsoft.NetCore.Analyzers

2404ec0

Add nullable annotations for ReleaseNotesUtil

28efa04

Add nullable annotations for GenerateAnalyzerRulesets

de1b692

Fix a unit test failure from incorrect refactoring

3591255

Disable nullable in TestReferenceAssembly

404f9cc

Add nullable annotations for Microsoft.NetFramework.Analyzers

c72761d

Add some more missing nullable annotations found after building the e…

64aa452

…ntire solution

mavasani requested review from genlu, sharwell, dotpaul and LLLXXXCCC November 8, 2019 00:43

sharwell reviewed Nov 8, 2019

View reviewed changes

src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/DiagnosticAnalyzerAttributeAnalyzer.cs Outdated Show resolved Hide resolved

...ft.CodeQuality.Analyzers/CSharp/QualityGuidelines/CSharpUseLiteralsWhereAppropriate.Fixer.cs Outdated Show resolved Hide resolved

dotpaul reviewed Nov 8, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Security/DoNotUseInsecureDeserializerWithoutBinderBase.cs Show resolved Hide resolved

sharwell reviewed Nov 8, 2019

View reviewed changes

mavasani added 4 commits November 8, 2019 06:13

Address feedback for symbol extensions

614dde0

Address feedback

b5b948d

Minor fix

4fdc7cc

Fix unit tests

a645fc3

dotpaul approved these changes Nov 8, 2019

View reviewed changes

sharwell reviewed Nov 15, 2019

View reviewed changes

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DictionaryAnalysisData.cs Outdated Show resolved Hide resolved

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DictionaryAnalysisData.cs Outdated Show resolved Hide resolved

sharwell reviewed Nov 15, 2019

View reviewed changes

mavasani requested a review from a team as a code owner November 16, 2019 00:45

mavasani added 3 commits November 15, 2019 18:16

Address feedback from Sam

9c1ee5b

Merge remote-tracking branch 'upstream/2.9.x' into NullableReferenceT…

0339d37

…ypes

Some more fixes

533b93e

mavasani force-pushed the NullableReferenceTypes branch from 15e8079 to 533b93e Compare November 16, 2019 02:24

sharwell approved these changes Nov 22, 2019

View reviewed changes

mavasani added 2 commits November 25, 2019 12:52

Merge remote-tracking branch 'upstream/2.9.x' into NullableReferenceT…

099e504

…ypes

Add nullable annotations after merging latest

cbe55d1

mavasani merged commit 0a8dd34 into dotnet:2.9.x Nov 26, 2019

mavasani deleted the NullableReferenceTypes branch November 26, 2019 13:15

This was referenced Nov 26, 2019

Follow-up work items from #3021 that enables Nullable reference types in the repo #3068

Open

Fix TODO comments to enable nullable reference types in security analyzers #3069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable nullable reference types for the repo #3021

Enable nullable reference types for the repo #3021

mavasani commented Nov 8, 2019

sharwell Nov 8, 2019

mavasani Nov 8, 2019

dotpaul commented Nov 8, 2019

dotpaul left a comment

sharwell left a comment

sharwell Nov 15, 2019

mavasani Nov 15, 2019

sharwell left a comment

sharwell Nov 15, 2019

mavasani Nov 15, 2019

sharwell Nov 15, 2019

mavasani Nov 15, 2019

mavasani commented Nov 16, 2019

mavasani commented Nov 16, 2019

mavasani commented Nov 18, 2019

dotpaul commented Nov 18, 2019

sharwell Nov 22, 2019

mavasani commented Nov 25, 2019

	IEnumerable<string> filteredStrings = stringLiteralValues.Where(literal => literal != null && !LooksLikeXmlTag(literal))!;
	IEnumerable<string> filteredStrings = stringLiteralValues.WhereNotNull().Where(literal => !LooksLikeXmlTag(literal));

Enable nullable reference types for the repo #3021

Enable nullable reference types for the repo #3021

Conversation

mavasani commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotpaul commented Nov 8, 2019

dotpaul left a comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Nov 16, 2019

mavasani commented Nov 16, 2019

mavasani commented Nov 18, 2019

dotpaul commented Nov 18, 2019

Choose a reason for hiding this comment

mavasani commented Nov 25, 2019