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.

Sign up for GitHub

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

Add switch to skip nullable analysis #49876

Merged
merged 15 commits into from
Dec 14, 2020
Merged

Add switch to skip nullable analysis #49876

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
27ccf02
Add switch to skip nullable analysis
cston Dec 8, 2020
92c6bee
Test SemanticModel
cston Dec 9, 2020
20bad7b
PR feedback
cston Dec 10, 2020
dbddbcd
Update comment
cston Dec 10, 2020
74b390f
Fix build
cston Dec 10, 2020
9765a57
Fix tests
cston Dec 10, 2020
3c759ca
PR feedback
cston Dec 10, 2020
8e4fb81
PR feedback
cston Dec 11, 2020
3756c6a
Rename property
cston Dec 11, 2020
4a324a5
PR feedback
cston Dec 12, 2020
88e8a69
Drop results if run-nullable-analysis=always but no #nullable enable
cston Dec 12, 2020
259663d
Rename property
cston Dec 12, 2020
b38ef12
Fix tests
cston Dec 12, 2020
0c22385
Fix formatting
cston Dec 12, 2020
35ad85d
Update comments
cston Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 48 additions & 30 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ internal Conversions Conversions
/// Run the nullable walker during the flow analysis passes. True if the project-level nullable
/// context option is set, or if any file enables nullable or just the nullable warnings.
/// </summary>
private ThreeState _lazyShouldRunNullableWalker;
private ThreeState _lazyShouldRunNullableAnalysis;

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

public override string Language
{
Expand Down Expand Up @@ -188,19 +191,7 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager
/// <summary>
/// True if we should enable nullable semantic analysis in this compilation.
/// </summary>
internal bool NullableSemanticAnalysisEnabled
{
get
{
var nullableAnalysisFlag = Feature("run-nullable-analysis");
if (nullableAnalysisFlag == "false")
{
return false;
}

return ShouldRunNullableWalker || nullableAnalysisFlag == "true";
}
}
internal bool NullableSemanticAnalysisEnabled => ShouldRunNullableAnalysis;

/// <summary>
/// True when the "peverify-compat" feature flag is set or the language version is below C# 7.2.
Expand All @@ -210,34 +201,61 @@ internal bool NullableSemanticAnalysisEnabled
/// </summary>
internal bool IsPeVerifyCompatEnabled => LanguageVersion < LanguageVersion.CSharp7_2 || Feature("peverify-compat") != null;

internal bool ShouldRunNullableWalker
/// <summary>
/// Returns true if nullable analysis should be run in this compilation.
/// </summary>
/// <return>
/// Returns true if analysis is explicitly enabled for all methods;
/// false if analysis is explicitly disabled; and otherwise returns true
/// if there are any nullable-enabled contexts in the compilation.
/// </return>
internal bool ShouldRunNullableAnalysis
{
get
{
if (!_lazyShouldRunNullableWalker.HasValue())
if (!_lazyShouldRunNullableAnalysis.HasValue())
{
_lazyShouldRunNullableAnalysis = (GetNullableAnalysisValue() ?? hasEnabledContexts()).ToThreeState();
}
return _lazyShouldRunNullableAnalysis.Value();

bool hasEnabledContexts()
{
if (Options.NullableContextOptions != NullableContextOptions.Disable)
{
_lazyShouldRunNullableWalker = ThreeState.True;
return true;
}

foreach (var syntaxTree in SyntaxTrees)
{
if (((CSharpSyntaxTree)syntaxTree).HasNullableEnables())
{
_lazyShouldRunNullableWalker = ThreeState.True;
return true;
}
}

_lazyShouldRunNullableWalker = ThreeState.False;
return SyntaxTrees.Any(tree => ((CSharpSyntaxTree)tree).HasNullableEnables());
}

return _lazyShouldRunNullableWalker.Value();
}
}

#if DEBUG
/// <summary>
/// Returns true if nullable analysis should be run and results ignored in cases where analysis
/// is not otherwise necessary. Used for testing in debug builds to increase the chance of catching
/// nullable regressions (e.g. https://github.com/dotnet/roslyn/issues/40136).
/// </summary>
/// <return>
/// Returns true unless analysis is explicitly disabled.
/// </return>
internal bool ShouldRunNullableAnalysisAndIgnoreResults => GetNullableAnalysisValue() != false;
#endif

/// <summary>
/// Returns Feature("run-nullable-analysis") as a bool? value:
/// true for "always"; false for "never"; and null otherwise.
/// </summary>
private bool? GetNullableAnalysisValue()
{
return Feature("run-nullable-analysis") switch
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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

{
"always" => true,
"never" => false,
_ => null,
};
}

/// <summary>
/// The language version that was used to parse the syntax trees of this compilation.
/// </summary>
Expand Down
18 changes: 13 additions & 5 deletions src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1730,7 +1730,10 @@ private BoundNode GetBoundLambdaOrQuery(CSharpSyntaxNode lambdaOrQuery)

// https://github.com/dotnet/roslyn/issues/35038: We need to do a rewrite here, and create a test that can hit this.
#if DEBUG
AnalyzeBoundNodeNullability(boundOuterExpression, incrementalBinder, diagnostics: new DiagnosticBag(), createSnapshots: false);
if (Compilation.ShouldRunNullableAnalysisAndIgnoreResults)
{
AnalyzeBoundNodeNullability(boundOuterExpression, incrementalBinder, diagnostics: new DiagnosticBag(), createSnapshots: false);
}
#endif

nodes = GuardedAddBoundTreeAndGetBoundNodeFromMap(lambdaOrQuery, boundOuterExpression);
Expand Down Expand Up @@ -1914,13 +1917,15 @@ private static Binder GetLambdaEnclosingBinder(int position, CSharpSyntaxNode st
/// </summary>
protected void EnsureNullabilityAnalysisPerformedIfNecessary()
{
// If we're in DEBUG mode, always enable the analysis, but throw away the results
#if !DEBUG
// If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results
#if DEBUG
if (!Compilation.ShouldRunNullableAnalysisAndIgnoreResults)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#else
if (!Compilation.NullableSemanticAnalysisEnabled)
#endif
{
return;
}
#endif

// If we have a snapshot manager, then we've already done
// all the work necessary and we should avoid taking an
Expand Down Expand Up @@ -2003,7 +2008,10 @@ void rewriteAndCache(DiagnosticBag diagnosticBag)
#if DEBUG
if (!Compilation.NullableSemanticAnalysisEnabled)
{
AnalyzeBoundNodeNullability(boundRoot, binder, diagnosticBag, createSnapshots: true);
if (Compilation.ShouldRunNullableAnalysisAndIgnoreResults)
{
AnalyzeBoundNodeNullability(boundRoot, binder, diagnosticBag, createSnapshots: true);
}
return;
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1689,7 +1689,7 @@ syntaxNode is ConstructorDeclarationSyntax constructorSyntax &&
ImmutableDictionary<Symbol, Symbol> remappedSymbols = null;
var compilation = bodyBinder.Compilation;
var isSufficientLangVersion = compilation.LanguageVersion >= MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion();
if (compilation.NullableSemanticAnalysisEnabled)
if (compilation.ShouldRunNullableAnalysis)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
methodBodyForSemanticModel = NullableWalker.AnalyzeAndRewrite(
compilation,
Expand Down
58 changes: 25 additions & 33 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,15 +1072,16 @@ internal static void AnalyzeIfNeeded(
bool getFinalNullableState,
out VariableState? finalNullableState)
{
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker)
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableAnalysis)
{
#if DEBUG
// Always run analysis in debug builds so that we can more reliably catch
// nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136
// Once we address https://github.com/dotnet/roslyn/issues/46579 we should also always pass `getFinalNullableState: true` in debug mode.
// We will likely always need to write a 'null' out for the out parameter in this code path, though, because
// we don't want to introduce behavior differences between debug and release builds
Analyze(compilation, method, node, new DiagnosticBag(), useConstructorExitWarnings: false, initialNullableState: null, getFinalNullableState: false, out _);
if (compilation.ShouldRunNullableAnalysisAndIgnoreResults)
{
// Once we address https://github.com/dotnet/roslyn/issues/46579 we should also always pass `getFinalNullableState: true` in debug mode.
// We will likely always need to write a 'null' out for the out parameter in this code path, though, because
// we don't want to introduce behavior differences between debug and release builds
Analyze(compilation, method, node, new DiagnosticBag(), useConstructorExitWarnings: false, initialNullableState: null, getFinalNullableState: false, out _);
}
#endif
finalNullableState = null;
return;
Expand Down Expand Up @@ -1293,11 +1294,9 @@ private static BoundNode Rewrite(ImmutableDictionary<BoundExpression, (Nullabili
internal static bool NeedsAnalysis(CSharpCompilation compilation)
{
#if DEBUG
// 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;
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#else
var canSkipAnalysis = compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker;
var canSkipAnalysis = compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableAnalysis;
return !canSkipAnalysis;
#endif
}
Expand All @@ -1309,15 +1308,18 @@ internal static void AnalyzeIfNeeded(
DiagnosticBag diagnostics)
{
var compilation = binder.Compilation;
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker)
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableAnalysis)
{
#if DEBUG
// Always run analysis in debug builds so that we can more reliably catch
// nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136
diagnostics = new DiagnosticBag();
#else
return;
if (compilation.ShouldRunNullableAnalysisAndIgnoreResults)
{
diagnostics = new DiagnosticBag();
}
else
#endif
{
return;
}
}

Analyze(
Expand Down Expand Up @@ -1436,26 +1438,16 @@ private static void Analyze(
{
ex.AddAnError(diagnostics);
}
}

#if DEBUG
#if REPORT_MAX
private static (Symbol symbol, int slots) s_maxSlots;
#endif
#endif
if (walker.compilation.NullableAnalysisData is { } state)
{
var key = (object?)symbol ?? walker.methodMainNode.Syntax;
state[key] = walker._variableSlot.Count;
}
}

private SharedWalkerState SaveSharedState()
{
#if DEBUG
#if REPORT_MAX
int slots = _variableSlot.Count;
if (slots > s_maxSlots.slots)
{
s_maxSlots = (_symbol, slots);
Debug.WriteLine("{0}: {1}", slots, _symbol);
}
#endif
#endif
return new SharedWalkerState(
_variableSlot.ToImmutableDictionary(),
ImmutableArray.Create(variableBySlot, start: 0, length: nextVariableSlot),
Expand Down
42 changes: 42 additions & 0 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9708,6 +9708,48 @@ public class Program
};
}

// See also NullableContextTests.NullableAnalysisFlags_01().
[Fact]
public void NullableAnalysisFlags()
{
string source =
@"class Program
{
#nullable enable
static object F1() => null;
#nullable disable
static object F2() => null;
}";

string filePath = Temp.CreateFile().WriteAllText(source).Path;
string fileName = Path.GetFileName(filePath);

string[] expectedWarningsAll = new[] { fileName + "(4,27): warning CS8603: Possible null reference return." };
string[] expectedWarningsNone = Array.Empty<string>();

AssertEx.Equal(expectedWarningsAll, compileAndRun(featureOpt: null));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=always"));
AssertEx.Equal(expectedWarningsNone, compileAndRun("/features:run-nullable-analysis=never"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=ALWAYS")); // unrecognized value (incorrect case) ignored
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=NEVER")); // unrecognized value (incorrect case) ignored
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=true")); // unrecognized value ignored
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=false")); // unrecognized value ignored
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=unknown")); // unrecognized value ignored

CleanupAllGeneratedFiles(filePath);

string[] compileAndRun(string featureOpt)
{
var args = new[] { "/target:library", "/preferreduilang:en", "/nologo", filePath };
if (featureOpt != null) args = args.Concat(featureOpt).ToArray();
var compiler = CreateCSharpCompiler(null, WorkingDirectory, args);
var outWriter = new StringWriter(CultureInfo.InvariantCulture);
int exitCode = compiler.Run(outWriter);
return outWriter.ToString().Split(new[] { '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries);
};
}

private static int OccurrenceCount(string source, string word)
{
var n = 0;
Expand Down
Loading