Add switch to skip nullable analysis #49876

cston · 2020-12-09T20:21:09Z

Add a switch that allows skipping nullable analysis completely for the compilation, or running nullable analysis on all methods in the compilation.

run-nullable-analysis=always  // analyze all methods
run-nullable-analysis=never   // skip all analysis
run-nullable-analysis=<other> // unrecognized value silently ignored

From a project file: <Features>run-nullable-analysis=never</Features>

From the command line: -features:run-nullable-analysis=never

jaredpar · 2020-12-09T21:26:36Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1089,6 +1092,13 @@ private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)
            Analyze(compilation, method, node, diagnostics, useConstructorExitWarnings, initialNullableState, getFinalNullableState, out finalNullableState);
        }

+#if DEBUG
+        private static bool ShouldRunAnalysisInDebug(CSharpCompilation compilation)


Think the method name is a bit off here to what it actually does. Think something closer to ShouldRunAnalysisAndIgnoreResults is closer.
#Resolved

jaredpar · 2020-12-09T21:27:08Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                    // Always run analysis in debug builds so that we can more reliably catch
+                    // nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136


This comment seems more applicable on the method now than the invocation of the method. #Resolved

jaredpar · 2020-12-09T21:28:26Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+#if DEBUG
+        private static bool ShouldRunAnalysisInDebug(CSharpCompilation compilation)
+        {
+            return compilation.Feature("nullableAnalysis") != "false";


Consider that in the future this switch is going to have to modes: force on and force off. Don't think "false" is a good decriptive value here. #Resolved

jaredpar · 2020-12-09T21:28:48Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1089,6 +1092,13 @@ private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)
            Analyze(compilation, method, node, diagnostics, useConstructorExitWarnings, initialNullableState, getFinalNullableState, out finalNullableState);
        }

+#if DEBUG


Part of the motivation for the feature flag was to disable in production but seems like you're making it debug only here. #Resolved

CSharpCompilation.ShouldRunNullableAnalysis now returns true when run-nullable-analysis=always so this property is really only necessary for code paths in DEBUG builds where that value is not set or not checked.

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

jaredpar · 2020-12-09T21:31:02Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+                ThreeState calculateResult()
+                {
+                    var feature = Feature("nullableAnalysis");
+                    if (feature == "false")


Consider having a central method that returns a ThreeState for the "nullableAnalysis" flag rather than having it interpreted in multiple places. #Resolved

AlekseyTs · 2020-12-09T23:40:09Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+                }
+                return _lazyShouldRunNullableWalker.Value();
+
+                ThreeState calculateResult()


calculateResult [](start = 27, length = 15)

I think it would be good to have a comment that spells the rules out. #Closed

AlekseyTs · 2020-12-09T23:47:08Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -399,7 +411,7 @@ protected override INamespaceSymbol CommonCreateErrorNamespaceSymbol(INamespaceS
            SyntaxAndDeclarationManager syntaxAndDeclarations,
            IReadOnlyDictionary<string, string> features,
            SemanticModelProvider? semanticModelProvider,
-            AsyncQueue<CompilationEvent>? eventQueue = null)


= null) [](start = 52, length = 8)

It is not clear what is the motivation for this change. #Closed

AlekseyTs · 2020-12-09T23:53:09Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -685,6 +697,9 @@ internal override Compilation WithEventQueue(AsyncQueue<CompilationEvent>? event
                eventQueue);
        }

+        // Nullable analysis data for methods, parameter default values, and attributes. Collected during testing only.
+        internal ConcurrentDictionary<object, int>? NullableAnalysisData;
+


Consider placing the field declaration at the beginning of the type declaration. I couldn't figure out why this place was chosen, it doesn't look like there is any relationship to surrounding code. #Closed

cston · 2020-12-09T23:53:41Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+
+                ThreeState calculateResult()
+                {
+                    var feature = Feature("nullableAnalysis");


nullableAnalysis [](start = 43, length = 16)

It is confusing to have this new "nullable-analysis" switch (used for all calls to NullableWalker) in addition to the existing "run-nullable-analysis" switch (used for calls to NullableWalker from SemanticModel only).

I'll re-purpose "run-nullable-analysis" to cover all calls. And I'll change the expected values from { true, false } to { always, never } to clarify the use:

run-nullable-analysis // equivalent to no switch run-nullable-analysis=always // analyze all methods run-nullable-analysis=never // skip analysis of methods run-nullable-analysis=<other> // silently ignored; equivalent to no switch ``` #Resolved

AlekseyTs · 2020-12-09T23:59:04Z

    internal bool NullableSemanticAnalysisEnabled

Should we deprecate/remove this now? #Closed

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:191 in 92c6bee. [](commit_id = 92c6bee, deletion_comment = False)

AlekseyTs · 2020-12-10T00:00:02Z

        if (!Compilation.NullableSemanticAnalysisEnabled)

Should this condition be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:1922 in 92c6bee. [](commit_id = 92c6bee, deletion_comment = False)

AlekseyTs · 2020-12-10T00:14:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableContextTests.cs

+                }
+
+                var actualAnalyzedKeys = GetNullableDataKeysAsStrings(comp.NullableAnalysisData);
+                AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys);


AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys) [](start = 16, length = 56)

Is this order sensitive? #Closed

GetNullableDataKeysAsStrings() sorts the list.

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

AlekseyTs · 2020-12-10T00:24:54Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableContextTests.cs

+                }
+
+                var syntaxTree = comp.SyntaxTrees[0];
+                var model = comp.GetSemanticModel(syntaxTree);


var model = comp.GetSemanticModel(syntaxTree); [](start = 16, length = 46)

I think it is probably better to split SemanticModel tests into their own tests because otherwise it is hard to tell what modifies NullableAnalysisData. Also, it would be good to cover all various flavors of SemanticModel. #Closed

AlekseyTs · 2020-12-10T00:31:24Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableContextTests.cs

+                var actualAnalyzedKeys = GetNullableDataKeysAsStrings(comp.NullableAnalysisData);
+                AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys);
+            }
+        }


} [](start = 8, length = 1)

Do we have coverage for the code path that calls NullableWalker.AnalyzeAndRewrite in MethodCompiler.cs? #Closed

This test hits that code path.

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

How? This test doesn't appear to execute a mainline compile, but instead gets it's data from the semantic model. I wouldn't expect that to hit the MethodCompiler.

In reply to: 540378224 [](ancestors = 540378224,539752287)

Ah, apologies, this was on a different line in iteration 2 vs 7.

In reply to: 540606199 [](ancestors = 540606199,540378224,539752287)

AlekseyTs · 2020-12-10T00:32:26Z

Done with review pass (commit 2) #Closed

cston · 2020-12-10T04:30:23Z

    internal bool NullableSemanticAnalysisEnabled

I've left this in for now, but it simply returns ShouldRunNullableAnalysis.

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

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:191 in 92c6bee. [](commit_id = 92c6bee, deletion_comment = False)

AlekseyTs · 2020-12-10T13:54:04Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+        /// </summary>
+        private bool? GetNullableAnalysisValue()
+        {
+            return Feature("run-nullable-analysis") switch


"run-nullable-analysis" [](start = 27, length = 23)

It looks like at least one IDE test in CSharpCompletionCommandHandlerTests.vb takes advantage of this feature. Do we need to make changes to test utilities to account for new expected values? #Closed

AlekseyTs · 2020-12-10T14:23:41Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

-#if !DEBUG
+            // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results
+#if DEBUG
+            if (!Compilation.ShouldRunNullableAnalysisAndIgnoreResults)


ShouldRunNullableAnalysisAndIgnoreResults [](start = 29, length = 41)

The name of this property (the "AndIgnoreResults" part) feels confusing in context of this if. Do we return even though we don't want to ignore the result? I think we either need to come up with a "better" name, or slightly adjust the code. For example:

if (!Compilation.NullableSemanticAnalysisEnabled #if DEBUG && !Compilation.ShouldRunNullableAnalysisAndIgnoreResults #endif ) { return; }

Even though combining both conditions in DEBUG might feel redundant if implementation of properties is "inlined", I think the intent is expressed much clearer and is easier to understand. The logic will be more robust to future changes in implementation of these properties. Also, this matches the logic we have below around the same conditions. In fact, that logic can be simplified. We can remove the added if (Compilation.ShouldRunNullableAnalysisAndIgnoreResults) and assert that the condition is true instead. #Closed

AlekseyTs · 2020-12-10T14:39:54Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -1689,7 +1689,7 @@ internal static BoundBlock BindMethodBody(MethodSymbol method, TypeCompilationSt
                    ImmutableDictionary<Symbol, Symbol> remappedSymbols = null;
                    var compilation = bodyBinder.Compilation;
                    var isSufficientLangVersion = compilation.LanguageVersion >= MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion();
-                    if (compilation.NullableSemanticAnalysisEnabled)
+                    if (compilation.ShouldRunNullableAnalysis)


ShouldRunNullableAnalysis [](start = 36, length = 25)

It is not clear what motivated this change. Given the current implementation and comments for the properties, I cannot tell the difference between the two. If it is important to keep both properties, I think we need to clarify when each of them is supposed to be used. If NullableSemanticAnalysisEnabled is about SemanticModel and ShouldRunNullableAnalysis is not, then the change doesn't feel appropriate because the comment inside the if indicates that SemanticModel is involved here in some way. #Closed

AlekseyTs · 2020-12-10T14:48:12Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

-            // Always run analysis in debug builds so that we can more reliably catch
-            // nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136
-            return true;
+            return compilation.ShouldRunNullableAnalysisAndIgnoreResults;


return compilation.ShouldRunNullableAnalysisAndIgnoreResults; [](start = 12, length = 61)

For clarity and consistency with other use-sites, I think this condition should be combined with !canSkipAnalysis #Closed

AlekseyTs · 2020-12-10T14:55:52Z

Done with review pass (commit 6) #Closed

Youssef1313

Should the command-line option be documented in the -help option for csc?

roslyn/src/Compilers/CSharp/Portable/CSharpResources.resx

Line 4623 in 0845e86

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

AlekseyTs · 2020-12-11T16:22:00Z

Should the command-line option be documented in the -help option for csc?

I think we intentionally do not document the /features switch. It's been there for years.

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

AlekseyTs

LGTM (commit 9)

333fred · 2020-12-11T17:58:16Z

I think we intentionally do not document the /features switch. It's been there for years.

Yep. For reference Youssef, the switch is there to help us diagnose issues when customers report things. This flag is being added so that, if we suspect a customer-reported issue is being caused by nullable and they can't share more info with us, we can tell them "Go set this in your csproj and let us know if the problem goes away".

333fred

LGTM (commit 9)

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

jaredpar · 2020-12-11T19:42:37Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+            // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results
+            if (!Compilation.IsNullableAnalysisEnabled
+#if DEBUG
+                && Compilation.IsNullableAnalysisExplicitlyDisabled


Shouldn't this be ||? #ByDesign

We only want to skip analysis in debug builds if "run-nullable-analysis=false". In that case, IsNullableAnalysisEnabled returns false and IsNullableAnalysisExplicitlyDisabled returns true. The first check is actually redundant in this case.

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

I find the #if in the middle of an if condition really hard to grok. Can we break this up so it's not splitting conditions? #Resolved

jaredpar · 2020-12-11T19:45:24Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

-            return !canSkipAnalysis;
+            if (canSkipAnalysis)
+            {
+                return !compilation.IsNullableAnalysisExplicitlyDisabled;


Feels odd to have this check here but no tinside of CanSkipAnalysis #ByDesign

If we moved the IsNullableAnalysisExplicitlyDisabled check into CanSkipAnalysis(), it would be difficult to differentiate cases in the callers where we want to analyze but drop results. (See the other two uses of CanSkipAnalysis().)

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

jaredpar · 2020-12-11T23:23:44Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+            // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results
+            if (!Compilation.IsNullableAnalysisEnabled
+#if DEBUG
+                && Compilation.IsNullableAnalysisExplicitlyDisabled


I find the #if in the middle of an if condition really hard to grok. Can we break this up so it's not splitting conditions? #Resolved

jaredpar · 2020-12-11T23:27:06Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

@@ -2001,9 +2006,12 @@ BoundNode bind(CSharpSyntaxNode root, DiagnosticBag diagnosticBag, out Binder bi
            void rewriteAndCache(DiagnosticBag diagnosticBag)
            {
 #if DEBUG
-                if (!Compilation.NullableSemanticAnalysisEnabled)
+                if (!Compilation.IsNullableAnalysisEnabled)


Why do we want to consider IsNullableAnalysisEnabled here? All of our other DEBUG checks focus only on making sure it's not explicitly disabled.

I'm not sure why we skipped AnalyzeBoundNodeNullability() here previously if the project has not enabled nullability. That behavior is unchanged.

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

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

cston · 2020-12-12T06:54:46Z

Based on offline feedback from @jaredpar: if "run-nullable-analysis=always", all methods are analyzed but results are dropped for methods that would not otherwise be analyzed; and in DEBUG builds, we analyze all methods if "run-nullable-analysis" is not set (see 88e8a69).

As a result of the DEBUG change, a number of callers of NullableWalker that were DEBUG only are now included for all build configurations.

AlekseyTs · 2020-12-14T15:23:42Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

        /// <summary>
        /// Analyzes a set of bound nodes, recording updated nullability information. This method is only
-        /// used during debug runs when nullability is disabled to verify that correct semantic information
-        /// is being recorded for all bound nodes. The results are thrown away.
+        /// used when nullable is explicitly enabled for all methods but diabled otherwise to verify that


diabled [](start = 73, length = 7)

Typo? #Resolved

AlekseyTs · 2020-12-14T15:33:50Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1290,16 +1287,14 @@ private static BoundNode Rewrite(ImmutableDictionary<BoundExpression, (Nullabili
            return rewrittenNode;
        }

+        private static bool CanSkipAnalysis(CSharpCompilation compilation)


CanSkipAnalysis [](start = 28, length = 15)

I think the name of this method is somewhat confusing given that sometimes we don't skip analysis when this method returns true. Consider renaming to "NeedResultFromAnalysis"/"NeedResultOfAnalysis" and reversing the logic appropriately.

AlekseyTs

LGTM (commit 14)

jaredpar · 2020-12-14T16:47:29Z

Based on offline feedback from @jaredpar: if "run-nullable-analysis=always", all methods are analyzed but results are dropped for methods that would not otherwise be analyzed;

Wanted to elaborate a bit on this. The motivation for this is increased code coverage of the nullable walker that could help us prevent regressions like we saw in 16.8.

Today the only way to exercise the nullable walker on code is to have the project opt into nullable reference types. That means that we are only testing this code on a fraction of the teams that are dogfooding the compiler. It's not reasonable to go to all of them and say "please enable NRT everywhere". That's a big ask and not a reasonable one for lots of legacy code.

That also means we are shipping bugs we easily could have prevented. The two bugs we shipped around excessive anonymous types and lambda usage exist in the customers who actively dogfood the compiler today. However those teams don't have nullable reference types enabled in that code base. If they had we could have prevented these bugs from getting to customers.

By implementing this proposal we can close this gap. All we need to do is have the feature flag flipped on in our central build locations (visual studio and arcade). Then everyone will start exercising the nullable walker on all of their code and hence we will find these bugs before they get to customers.

Imagine for instance how we could leverage this flag during our VS insertion process. It would let us run the nullable walker over the entier VS code base. That's a massive increase in test coverage.

jaredpar · 2020-12-14T16:57:50Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+        /// Performs just the analysis step of getting nullability information for a semantic model.
+        /// This is only used when nullable analysis is explicitly enabled for all methods but nullable
+        /// is turned off on a compilation level, giving some extra verification of the nullable flow analysis. 


Suggested change

/// Performs just the analysis step of getting nullability information for a semantic model.

/// This is only used when nullable analysis is explicitly enabled for all methods but nullable

/// is turned off on a compilation level, giving some extra verification of the nullable flow analysis.

/// Performs the analysis step of getting nullability information for a semantic model but does

/// not actually use the results. This gives us extra verification of nullable flow analysis. It is only

/// used in contexts where nullable analysis is disabled in the program but requested through the

/// "run-nullable-analysis" feature flag or when the compiler is running in DEBUG

``` #Resolved

jaredpar · 2020-12-14T16:59:15Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1233,7 +1230,7 @@ private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)

 #if DEBUG
            // https://github.com/dotnet/roslyn/issues/34993 Enable for all calls
-            if (compilation.NullableSemanticAnalysisEnabled)
+            if (compilation.IsNullableAnalysisEnabled)


Suggested change

if (compilation.IsNullableAnalysisEnabled)

if (compilation.IsNullableAnalysisEnabled || compilation.IsNullableAnalysisEnabledAlways)

``` #ByDesign

I tried testing for both properties (here and below) in an earlier commit, but it looks like callers are depending on IsNullableAnalysisEnabled since many tests failed. If we need this DEBUG verification for IsNullableAnalysisEnabledAlways, let's add that later.

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

jaredpar · 2020-12-14T16:59:27Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1266,7 +1263,7 @@ private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)
            newSnapshots = newSnapshotBuilder.ToManagerAndFree();

 #if DEBUG
-            if (binder.Compilation.NullableSemanticAnalysisEnabled)
+            if (binder.Compilation.IsNullableAnalysisEnabled)


Suggested change

if (binder.Compilation.IsNullableAnalysisEnabled)

if (binder.Compilation.IsNullableAnalysisEnabled || binder.Compilation.IsNullableAnalysisEnabledAlways)

``` #ByDesign

jaredpar · 2020-12-14T17:01:26Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+        /// If this property returns true but IsNullableAnalysisEnabled returns false,
+        /// any nullable analysis should be enabled but results should be ignored.


Did you consider having a named property that reflected this logic like: IsNullableWalkerVerificationEnabled?

I did consider variants of these two properties but wasn't able to find variants that made it easier to understand the callers. I'm happy to revisit this later.

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

* upstream/master: (241 commits) Allow pattern matching `null` against pointer types when the pointer types contain nested type parameters (dotnet#49915) Remove document extension method and convert usages to use the text buffer extension method. VB: Strengthen implementation of `PropertySymbol.IsWritable` against NullReferenceException (dotnet#49962) Add switch to skip nullable analysis (dotnet#49876) Update dependencies from https://github.com/dotnet/roslyn build 20201211.16 (dotnet#49958) Treat record positional parameters as properties (dotnet#48329) [master] Update dependencies from dotnet/roslyn (dotnet#49395) VB: Ensure array access indexes undergo conversion to integer even when there is a mismatch with array rank. (dotnet#49907) Disable OOP when running as cloud environment client VS instance Rename workspace context method (and unify impls) to better represent the condition being checked Report non-Const locals used in an expression that must have a constant value. (dotnet#49912) Add support for more ServiceAudience values (dotnet#49914) Handle ref-containing structs returned by value from function-pointers (dotnet#49883) Fix error on out param of extern local function (dotnet#49860) Fix constructor exit warnings for generic NotNull (dotnet#49841) Loc updates Prefer more specific path map key (dotnet#49670) Rename `_availablelocalFunctionOrdinal` to `_availableLocalFunctionOrdinal` (dotnet#49901) Fix namespace so that external access wrapper type can be accessed from UT. XamlProjectService fixes (dotnet#49711) ...

Add switch to skip nullable analysis

27ccf02

cston requested a review from a team as a code owner December 9, 2020 20:21

Dotnet-GitSync-Bot added the Area-Compilers label Dec 9, 2020

cston added the New Language Feature - Nullable Reference Types Nullable Reference Types label Dec 9, 2020

cston added this to the 16.9 milestone Dec 9, 2020

jaredpar reviewed Dec 9, 2020

View reviewed changes

Test SemanticModel

92c6bee

AlekseyTs reviewed Dec 9, 2020

View reviewed changes

cston commented Dec 9, 2020

View reviewed changes

AlekseyTs reviewed Dec 10, 2020

View reviewed changes

cston added 4 commits December 9, 2020 20:43

PR feedback

20bad7b

Update comment

dbddbcd

Fix build

74b390f

Fix tests

9765a57

Youssef1313 mentioned this pull request Dec 10, 2020

Document run-nullable-analysis when it's shipped dotnet/docs#21939

Closed

AlekseyTs reviewed Dec 10, 2020

View reviewed changes

RikkiGibson self-assigned this Dec 10, 2020

Youssef1313 suggested changes Dec 11, 2020

View reviewed changes

Youssef1313 reviewed Dec 11, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Show resolved Hide resolved

Rename property

3756c6a

AlekseyTs approved these changes Dec 11, 2020

View reviewed changes

333fred approved these changes Dec 11, 2020

View reviewed changes

jaredpar reviewed Dec 11, 2020

View reviewed changes

PR feedback

4a324a5

jaredpar approved these changes Dec 12, 2020

View reviewed changes

Drop results if run-nullable-analysis=always but no #nullable enable

88e8a69

cston added 3 commits December 11, 2020 23:04

Rename property

259663d

Fix tests

b38ef12

Fix formatting

0c22385

AlekseyTs reviewed Dec 14, 2020

View reviewed changes

AlekseyTs approved these changes Dec 14, 2020

View reviewed changes

jaredpar reviewed Dec 14, 2020

View reviewed changes

Update comments

35ad85d

jaredpar approved these changes Dec 14, 2020

View reviewed changes

cston merged commit 9955e96 into dotnet:master Dec 14, 2020

ghost modified the milestones: 16.9, Next Dec 14, 2020

cston deleted the skip-nullable branch December 14, 2020 19:00

dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020

		// Always run analysis in debug builds so that we can more reliably catch
		// nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136

-        /// Performs just the analysis step of getting nullability information for a semantic model.
-        /// This is only used when nullable analysis is explicitly enabled for all methods but nullable
-        /// is turned off on a compilation level, giving some extra verification of the nullable flow analysis.
+        /// Performs the analysis step of getting nullability information for a semantic model but does
+        /// not actually use the results. This gives us extra verification of nullable flow analysis. It is only
+        /// used in contexts where nullable analysis is disabled in the program but requested through the
+        /// "run-nullable-analysis" feature flag or when the compiler is running in DEBUG
+``` #Resolved

	if (compilation.IsNullableAnalysisEnabled)
	if (compilation.IsNullableAnalysisEnabled \|\| compilation.IsNullableAnalysisEnabledAlways)
	``` #ByDesign

	if (binder.Compilation.IsNullableAnalysisEnabled)
	if (binder.Compilation.IsNullableAnalysisEnabled \|\| binder.Compilation.IsNullableAnalysisEnabledAlways)
	``` #ByDesign

		/// If this property returns true but IsNullableAnalysisEnabled returns false,
		/// any nullable analysis should be enabled but results should be ignored.

Add switch to skip nullable analysis #49876

Add switch to skip nullable analysis #49876

Conversation

cston commented Dec 9, 2020 • edited Loading

jaredpar Dec 9, 2020 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 9, 2020 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 9, 2020 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 9, 2020 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

jaredpar Dec 9, 2020 • edited by cston Loading

Choose a reason for hiding this comment

AlekseyTs Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

cston Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 9, 2020 • edited Loading

AlekseyTs commented Dec 10, 2020 • edited Loading

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 10, 2020 • edited Loading

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

AlekseyTs commented Dec 10, 2020 • edited Loading

cston commented Dec 10, 2020

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 10, 2020 • edited Loading

Youssef1313 left a comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 11, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

333fred commented Dec 11, 2020

333fred left a comment

Choose a reason for hiding this comment

jaredpar Dec 11, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Dec 11, 2020 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 11, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Dec 11, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Dec 12, 2020

AlekseyTs Dec 14, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jaredpar commented Dec 14, 2020

jaredpar Dec 14, 2020 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 14, 2020 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Dec 14, 2020 • edited by cston Loading

cston commented Dec 9, 2020 •

edited

Loading

jaredpar Dec 9, 2020 •

edited by cston

Loading

jaredpar Dec 9, 2020 •

edited by cston

Loading

jaredpar Dec 9, 2020 •

edited by cston

Loading

jaredpar Dec 9, 2020 •

edited by cston

Loading

cston Dec 10, 2020 •

edited

Loading

jaredpar Dec 9, 2020 •

edited by cston

Loading

AlekseyTs Dec 9, 2020 •

edited

Loading

AlekseyTs Dec 9, 2020 •

edited

Loading

AlekseyTs Dec 9, 2020 •

edited

Loading

cston Dec 9, 2020 •

edited

Loading

AlekseyTs commented Dec 9, 2020 •

edited

Loading

AlekseyTs commented Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs commented Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs Dec 10, 2020 •

edited

Loading

AlekseyTs commented Dec 10, 2020 •

edited

Loading

jaredpar Dec 11, 2020 •

edited by cston

Loading

jaredpar Dec 11, 2020 •

edited by cston

Loading

jaredpar Dec 11, 2020 •

edited by cston

Loading

jaredpar Dec 11, 2020 •

edited by cston

Loading

AlekseyTs Dec 14, 2020 •

edited by cston

Loading

jaredpar Dec 14, 2020 •

edited by cston

Loading

jaredpar Dec 14, 2020 •

edited by cston

Loading

jaredpar Dec 14, 2020 •

edited by cston

Loading

cston Dec 14, 2020 •

edited

Loading