Add switch to skip nullable analysis#49876
Conversation
| } | ||
|
|
||
| #if DEBUG | ||
| private static bool ShouldRunAnalysisInDebug(CSharpCompilation compilation) |
There was a problem hiding this comment.
Think the method name is a bit off here to what it actually does. Think something closer to ShouldRunAnalysisAndIgnoreResults is closer.
#Resolved
| // Always run analysis in debug builds so that we can more reliably catch | ||
| // nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136 |
There was a problem hiding this comment.
This comment seems more applicable on the method now than the invocation of the method. #Resolved
| #if DEBUG | ||
| private static bool ShouldRunAnalysisInDebug(CSharpCompilation compilation) | ||
| { | ||
| return compilation.Feature("nullableAnalysis") != "false"; |
There was a problem hiding this comment.
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
| Analyze(compilation, method, node, diagnostics, useConstructorExitWarnings, initialNullableState, getFinalNullableState, out finalNullableState); | ||
| } | ||
|
|
||
| #if DEBUG |
There was a problem hiding this comment.
Part of the motivation for the feature flag was to disable in production but seems like you're making it debug only here. #Resolved
There was a problem hiding this comment.
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)
| ThreeState calculateResult() | ||
| { | ||
| var feature = Feature("nullableAnalysis"); | ||
| if (feature == "false") |
There was a problem hiding this comment.
Consider having a central method that returns a ThreeState for the "nullableAnalysis" flag rather than having it interpreted in multiple places. #Resolved
| } | ||
| return _lazyShouldRunNullableWalker.Value(); | ||
|
|
||
| ThreeState calculateResult() |
There was a problem hiding this comment.
calculateResult [](start = 27, length = 15)
I think it would be good to have a comment that spells the rules out. #Closed
| SyntaxAndDeclarationManager syntaxAndDeclarations, | ||
| IReadOnlyDictionary<string, string> features, | ||
| SemanticModelProvider? semanticModelProvider, | ||
| AsyncQueue<CompilationEvent>? eventQueue = null) |
There was a problem hiding this comment.
= null) [](start = 52, length = 8)
It is not clear what is the motivation for this change. #Closed
|
|
||
| // Nullable analysis data for methods, parameter default values, and attributes. Collected during testing only. | ||
| internal ConcurrentDictionary<object, int>? NullableAnalysisData; | ||
|
|
There was a problem hiding this comment.
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
|
|
||
| ThreeState calculateResult() | ||
| { | ||
| var feature = Feature("nullableAnalysis"); |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| var actualAnalyzedKeys = GetNullableDataKeysAsStrings(comp.NullableAnalysisData); | ||
| AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys); |
There was a problem hiding this comment.
AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys) [](start = 16, length = 56)
Is this order sensitive? #Closed
There was a problem hiding this comment.
| } | ||
|
|
||
| var syntaxTree = comp.SyntaxTrees[0]; | ||
| var model = comp.GetSemanticModel(syntaxTree); |
There was a problem hiding this comment.
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
| var actualAnalyzedKeys = GetNullableDataKeysAsStrings(comp.NullableAnalysisData); | ||
| AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys); | ||
| } | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
Do we have coverage for the code path that calls NullableWalker.AnalyzeAndRewrite in MethodCompiler.cs? #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Ah, apologies, this was on a different line in iteration 2 vs 7.
In reply to: 540606199 [](ancestors = 540606199,540378224,539752287)
|
Done with review pass (commit 2) #Closed |
I've left this in for now, but it simply returns In reply to: 742140016 [](ancestors = 742140016) Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:191 in 92c6bee. [](commit_id = 92c6bee, deletion_comment = False) |
| /// </summary> | ||
| private bool? GetNullableAnalysisValue() | ||
| { | ||
| return Feature("run-nullable-analysis") switch |
There was a problem hiding this comment.
"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
| #if !DEBUG | ||
| // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results | ||
| #if DEBUG | ||
| if (!Compilation.ShouldRunNullableAnalysisAndIgnoreResults) |
There was a problem hiding this comment.
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
| var compilation = bodyBinder.Compilation; | ||
| var isSufficientLangVersion = compilation.LanguageVersion >= MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion(); | ||
| if (compilation.NullableSemanticAnalysisEnabled) | ||
| if (compilation.ShouldRunNullableAnalysis) |
There was a problem hiding this comment.
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
| // 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; |
There was a problem hiding this comment.
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
|
Done with review pass (commit 6) #Closed |
Youssef1313
left a comment
There was a problem hiding this comment.
Should the command-line option be documented in the -help option for csc?
I think we intentionally do not document the In reply to: 550065544 [](ancestors = 550065544) |
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". |
| // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results | ||
| if (!Compilation.IsNullableAnalysisEnabled | ||
| #if DEBUG | ||
| && Compilation.IsNullableAnalysisExplicitlyDisabled |
There was a problem hiding this comment.
Shouldn't this be ||? #ByDesign
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
| return !canSkipAnalysis; | ||
| if (canSkipAnalysis) | ||
| { | ||
| return !compilation.IsNullableAnalysisExplicitlyDisabled; |
There was a problem hiding this comment.
Feels odd to have this check here but no tinside of CanSkipAnalysis #ByDesign
There was a problem hiding this comment.
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)
| // If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results | ||
| if (!Compilation.IsNullableAnalysisEnabled | ||
| #if DEBUG | ||
| && Compilation.IsNullableAnalysisExplicitlyDisabled |
There was a problem hiding this comment.
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
| { | ||
| #if DEBUG | ||
| if (!Compilation.NullableSemanticAnalysisEnabled) | ||
| if (!Compilation.IsNullableAnalysisEnabled) |
There was a problem hiding this comment.
Why do we want to consider IsNullableAnalysisEnabled here? All of our other DEBUG checks focus only on making sure it's not explicitly disabled.
There was a problem hiding this comment.
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)
|
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 |
| /// 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 |
There was a problem hiding this comment.
diabled [](start = 73, length = 7)
Typo? #Resolved
| return rewrittenNode; | ||
| } | ||
|
|
||
| private static bool CanSkipAnalysis(CSharpCompilation compilation) |
There was a problem hiding this comment.
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.
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. |
| /// 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. |
There was a problem hiding this comment.
| /// 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 DEBUG | ||
| // https://github.com/dotnet/roslyn/issues/34993 Enable for all calls | ||
| if (compilation.NullableSemanticAnalysisEnabled) | ||
| if (compilation.IsNullableAnalysisEnabled) |
There was a problem hiding this comment.
| if (compilation.IsNullableAnalysisEnabled) | |
| if (compilation.IsNullableAnalysisEnabled || compilation.IsNullableAnalysisEnabledAlways) | |
| ``` #ByDesign |
There was a problem hiding this comment.
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)
|
|
||
| #if DEBUG | ||
| if (binder.Compilation.NullableSemanticAnalysisEnabled) | ||
| if (binder.Compilation.IsNullableAnalysisEnabled) |
There was a problem hiding this comment.
| 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. |
There was a problem hiding this comment.
Did you consider having a named property that reflected this logic like: IsNullableWalkerVerificationEnabled?
There was a problem hiding this comment.
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 a switch that allows skipping nullable analysis completely for the compilation, or running nullable analysis on all methods in the compilation.
From a project file:
<Features>run-nullable-analysis=never</Features>From the command line:
-features:run-nullable-analysis=never