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 1 commit
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
27 changes: 19 additions & 8 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 @@ -216,25 +216,33 @@ internal bool ShouldRunNullableWalker
{
if (!_lazyShouldRunNullableWalker.HasValue())
{
_lazyShouldRunNullableWalker = calculateResult();
}
return _lazyShouldRunNullableWalker.Value();

ThreeState calculateResult()
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

{
var feature = Feature("nullableAnalysis");
Copy link
Member Author

@cston cston Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (feature == "false")
Copy link
Member

@jaredpar jaredpar Dec 9, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

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

{
return ThreeState.False;
}

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

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

_lazyShouldRunNullableWalker = ThreeState.False;
return ThreeState.False;
}

return _lazyShouldRunNullableWalker.Value();
}
}

Expand Down Expand Up @@ -399,7 +407,7 @@ private CSharpCompilation(
SyntaxAndDeclarationManager syntaxAndDeclarations,
IReadOnlyDictionary<string, string> features,
SemanticModelProvider? semanticModelProvider,
AsyncQueue<CompilationEvent>? eventQueue = null)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

AsyncQueue<CompilationEvent>? eventQueue)
: base(assemblyName, references, features, isSubmission, semanticModelProvider, eventQueue)
{
WellKnownMemberSignatureComparer = new WellKnownMembersSignatureComparer(this);
Expand Down Expand Up @@ -685,6 +693,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;

Copy link
Contributor

@AlekseyTs AlekseyTs Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

#endregion

#region Submission
Expand Down
61 changes: 33 additions & 28 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 @@ -1075,12 +1075,15 @@ internal static void AnalyzeIfNeeded(
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker)
{
#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 (ShouldRunAnalysisInDebug(compilation))
{
// 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 _);
}
#endif
finalNullableState = null;
return;
Expand All @@ -1089,6 +1092,13 @@ internal static void AnalyzeIfNeeded(
Analyze(compilation, method, node, diagnostics, useConstructorExitWarnings, initialNullableState, getFinalNullableState, out finalNullableState);
}

#if DEBUG
Copy link
Member

@jaredpar jaredpar Dec 9, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@cston cston 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.

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)

private static bool ShouldRunAnalysisInDebug(CSharpCompilation compilation)
Copy link
Member

@jaredpar jaredpar Dec 9, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

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

{
return compilation.Feature("nullableAnalysis") != "false";
Copy link
Member

@jaredpar jaredpar Dec 9, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

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

}
#endif

internal static void Analyze(
CSharpCompilation compilation,
MethodSymbol method,
Expand Down Expand Up @@ -1295,7 +1305,7 @@ 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 ShouldRunAnalysisInDebug(compilation);
#else
var canSkipAnalysis = compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker;
return !canSkipAnalysis;
Expand All @@ -1312,12 +1322,17 @@ internal static void AnalyzeIfNeeded(
if (compilation.LanguageVersion < MessageID.IDS_FeatureNullableReferenceTypes.RequiredVersion() || !compilation.ShouldRunNullableWalker)
{
#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 (ShouldRunAnalysisInDebug(compilation))
{
// Always run analysis in debug builds so that we can more reliably catch
// nullable regressions e.g. https://github.com/dotnet/roslyn/issues/40136
Copy link
Member

@jaredpar jaredpar Dec 9, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

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

diagnostics = new DiagnosticBag();
}
else
#endif
{
return;
}
}

Analyze(
Expand Down Expand Up @@ -1436,26 +1451,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
46 changes: 46 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,52 @@ 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:nullableAnalysis"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=true"));
AssertEx.Equal(expectedWarningsNone, compileAndRun("/features:nullableAnalysis=false"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=TRUE")); // unrecognized value (incorrect case) treated as "true"
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=FALSE")); // unrecognized value (incorrect case) treated as "true"
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=unknown")); // unrecognized value treated as "true"
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=true"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:run-nullable-analysis=false"));
AssertEx.Equal(expectedWarningsNone, compileAndRun("/features:nullableAnalysis=false,run-nullable-analysis=false"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=false,run-nullable-analysis=true"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=true,run-nullable-analysis=false"));
AssertEx.Equal(expectedWarningsAll, compileAndRun("/features:nullableAnalysis=true,run-nullable-analysis=true"));

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
161 changes: 161 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@

#nullable disable

using System.Collections.Concurrent;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Semantics
Expand Down Expand Up @@ -149,5 +152,163 @@ void AssertEnabledForInheritence(NullableContext context, bool warningsEnabled,
Assert.True(contextInherited.AnnotationsInherited());
}
}

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

// https://github.com/dotnet/roslyn/issues/49746: Currently, if we analyze any members, we analyze all.
var expectedAnalyzedKeysAll = new[] { ".cctor", ".ctor", "F1", "F2" };

verify(parseOptions: TestOptions.Regular, expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", null), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "true"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "false"));
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "TRUE"), expectedAnalyzedKeysAll); // unrecognized value (incorrect case) treated as "true"
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "FALSE"), expectedAnalyzedKeysAll); // unrecognized value (incorrect case) treated as "true"
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "unknown"), expectedAnalyzedKeysAll); // unrecognized value treated as "true"
verify(parseOptions: TestOptions.Regular.WithFeature("run-nullable-analysis", "true"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("run-nullable-analysis", "false"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "false").WithFeature("run-nullable-analysis", "false"));
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "false").WithFeature("run-nullable-analysis", "true"), "F1", "F2");
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "true").WithFeature("run-nullable-analysis", "false"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "true").WithFeature("run-nullable-analysis", "true"), expectedAnalyzedKeysAll);

void verify(CSharpParseOptions parseOptions, params string[] expectedAnalyzedKeys)
{
var comp = CreateCompilation(source, parseOptions: parseOptions);
comp.NullableAnalysisData = new ConcurrentDictionary<object, int>();
if (expectedAnalyzedKeys.Length > 0)
{
comp.VerifyDiagnostics(
// (7,27): warning CS8603: Possible null reference return.
// static object F1() => C1;
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "C1").WithLocation(7, 27));
}
else
{
comp.VerifyDiagnostics();
}

var actualAnalyzedKeys = GetNullableDataKeysAsStrings(comp.NullableAnalysisData);
AssertEx.Equal(expectedAnalyzedKeys, actualAnalyzedKeys);
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.

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

Is this order sensitive? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

GetNullableDataKeysAsStrings() sorts the list.

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

}
}

[Fact]
public void NullableAnalysisFlags_02()
{
var source =
@"#nullable enable
class Program
{
const object? C1 = null;
const object? C2 = null;
#nullable enable
static void F1(object obj = C1) { }
#nullable disable
static void F2(object obj = C2) { }
}";

// https://github.com/dotnet/roslyn/issues/49746: Currently, if we analyze any members, we analyze all.
var expectedAnalyzedKeysAll = new[] { ".cctor", ".ctor", "= C1", "= C2", "F1", "F2" };

verify(parseOptions: TestOptions.Regular, expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", null), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "true"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "false"));

void verify(CSharpParseOptions parseOptions, params string[] expectedAnalyzedKeys)
{
var comp = CreateCompilation(source, parseOptions: parseOptions);
comp.NullableAnalysisData = new ConcurrentDictionary<object, int>();
if (expectedAnalyzedKeys.Length > 0)
{
comp.VerifyDiagnostics(
// (7,33): warning CS8625: Cannot convert null literal to non-nullable reference type.
// static void F1(object obj = C1) { }
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "C1").WithLocation(7, 33));
}
else
{
comp.VerifyDiagnostics();
}

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

[Fact]
public void NullableAnalysisFlags_03()
{
var sourceA =
@"#nullable enable
public class A : System.Attribute
{
public A(object obj) { }
public const object? C1 = null;
public const object? C2 = null;
}";
var refA = CreateCompilation(sourceA).EmitToImageReference();

var sourceB =
@"#nullable enable
[A(A.C1)]
struct B1
{
}
#nullable disable
[A(A.C2)]
struct B2
{
}";

// https://github.com/dotnet/roslyn/issues/49746: Currently, if we analyze any members, we analyze all.
var expectedAnalyzedKeysAll = new[] { ".cctor", ".cctor", "A(A.C1)", "A(A.C2)" };

verify(parseOptions: TestOptions.Regular, expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", null), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "true"), expectedAnalyzedKeysAll);
verify(parseOptions: TestOptions.Regular.WithFeature("nullableAnalysis", "false"));

void verify(CSharpParseOptions parseOptions, params string[] expectedAnalyzedKeys)
{
var comp = CreateCompilation(sourceB, references: new[] { refA }, parseOptions: parseOptions);
comp.NullableAnalysisData = new ConcurrentDictionary<object, int>();
if (expectedAnalyzedKeys.Length > 0)
{
comp.VerifyDiagnostics(
// (2,4): warning CS8625: Cannot convert null literal to non-nullable reference type.
// [A(A.C1)]
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "A.C1").WithLocation(2, 4));
}
else
{
comp.VerifyDiagnostics();
}

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

private static string[] GetNullableDataKeysAsStrings(ConcurrentDictionary<object, int> nullableData) =>
nullableData.Keys.Select(key => GetNullableDataKeyAsString(key)).OrderBy(key => key).ToArray();

private static string GetNullableDataKeyAsString(object key) =>
key is Symbol symbol ? symbol.MetadataName : ((SyntaxNode)key).ToString();
}
}
Loading