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.

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
87 changes: 53 additions & 34 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,15 @@ 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;

/// <summary>
/// Nullable analysis data for methods, parameter default values, and attributes.
/// The key is a symbol for methods or parameters, and syntax for attributes,
/// and the value is the number of entries tracked during analysis of that key.
/// The data is collected during testing only.
/// </summary>
internal ConcurrentDictionary<object, int>? NullableAnalysisData;
333fred marked this conversation as resolved.
Show resolved Hide resolved

public override string Language
{
Expand Down Expand Up @@ -185,23 +193,6 @@ internal override CommonAnonymousTypeManager CommonAnonymousTypeManager
/// </summary>
internal bool FeatureStrictEnabled => Feature("strict") != null;

/// <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";
}
}

/// <summary>
/// True when the "peverify-compat" feature flag is set or the language version is below C# 7.2.
/// With this flag we will avoid certain patterns known not be compatible with PEVerify.
Expand All @@ -210,34 +201,62 @@ internal bool NullableSemanticAnalysisEnabled
/// </summary>
internal bool IsPeVerifyCompatEnabled => LanguageVersion < LanguageVersion.CSharp7_2 || Feature("peverify-compat") != null;

internal bool ShouldRunNullableWalker
/// <summary>
/// Returns true if nullable analysis is enabled 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 IsNullableAnalysisEnabled
{
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());
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
}

return _lazyShouldRunNullableWalker.Value();
}
}

#if DEBUG
/// <summary>
/// Returns false if nullable analysis is disabled for the entire compilation.
/// </summary>
/// <remarks>
/// This method is only used for debug builds. In debug builds, unless analysis is
/// explicitly disabled, analysis is run, even though the results may be ignored,
/// to increase the chance of catching nullable regressions
/// (e.g. https://github.com/dotnet/roslyn/issues/40136).
/// </remarks>
internal bool IsNullableAnalysisExplicitlyDisabled => 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

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,
333fred marked this conversation as resolved.
Show resolved Hide resolved
};
}

/// <summary>
/// The language version that was used to parse the syntax trees of this compilation.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,7 @@ private void AppendSymbolsWithNameAndArity(

private Symbol RemapSymbolIfNecessary(Symbol symbol)
{
if (!Compilation.NullableSemanticAnalysisEnabled) return symbol;
if (!Compilation.IsNullableAnalysisEnabled) return symbol;

switch (symbol)
{
Expand Down
45 changes: 27 additions & 18 deletions src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,14 @@ internal override MemberSemanticModel GetMemberModel(SyntaxNode node)
protected virtual NullableWalker.SnapshotManager GetSnapshotManager()
{
EnsureNullabilityAnalysisPerformedIfNecessary();
Debug.Assert(_lazySnapshotManager is object || !Compilation.NullableSemanticAnalysisEnabled);
Debug.Assert(_lazySnapshotManager is object || !Compilation.IsNullableAnalysisEnabled);
return _lazySnapshotManager;
}

internal ImmutableDictionary<Symbol, Symbol> GetRemappedSymbols()
{
EnsureNullabilityAnalysisPerformedIfNecessary();
Debug.Assert(_lazyRemappedSymbols is object || this is AttributeSemanticModel || !Compilation.NullableSemanticAnalysisEnabled);
Debug.Assert(_lazyRemappedSymbols is object || this is AttributeSemanticModel || !Compilation.IsNullableAnalysisEnabled);
return _lazyRemappedSymbols;
}

Expand Down Expand Up @@ -197,7 +197,7 @@ internal override BoundExpression GetSpeculativelyBoundExpression(int position,
throw new ArgumentNullException(nameof(expression));
}

if (!Compilation.NullableSemanticAnalysisEnabled || bindingOption != SpeculativeBindingOption.BindAsExpression)
if (!Compilation.IsNullableAnalysisEnabled || bindingOption != SpeculativeBindingOption.BindAsExpression)
{
return GetSpeculativelyBoundExpressionWithoutNullability(position, expression, bindingOption, out binder, out crefSymbols);
}
Expand Down Expand Up @@ -701,7 +701,7 @@ private LocalFunctionSymbol GetDeclaredLocalFunction(LocalFunctionStatementSynta

private T GetRemappedSymbol<T>(T originalSymbol) where T : Symbol
{
if (!Compilation.NullableSemanticAnalysisEnabled) return originalSymbol;
if (!Compilation.IsNullableAnalysisEnabled) return originalSymbol;

EnsureNullabilityAnalysisPerformedIfNecessary();
if (_lazyRemappedSymbols is null) return originalSymbol;
Expand Down Expand Up @@ -1512,11 +1512,11 @@ protected void GuardedAddBoundTreeForStandaloneSyntax(SyntaxNode syntax, BoundNo
NodeMapBuilder.AddToMap(bound, _guardedNodeMap, SyntaxTree, syntax);
}

Debug.Assert((manager is null && (!Compilation.NullableSemanticAnalysisEnabled || syntax != Root || syntax is TypeSyntax ||
Debug.Assert((manager is null && (!Compilation.IsNullableAnalysisEnabled || syntax != Root || syntax is TypeSyntax ||
// Supporting attributes is tracked by
// https://github.com/dotnet/roslyn/issues/36066
this is AttributeSemanticModel)) ||
(manager is object && remappedSymbols is object && syntax == Root && Compilation.NullableSemanticAnalysisEnabled && _lazySnapshotManager is null));
(manager is object && remappedSymbols is object && syntax == Root && Compilation.IsNullableAnalysisEnabled && _lazySnapshotManager is null));
if (manager is object)
{
_lazySnapshotManager = manager;
Expand Down 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.IsNullableAnalysisExplicitlyDisabled)
{
AnalyzeBoundNodeNullability(boundOuterExpression, incrementalBinder, diagnostics: new DiagnosticBag(), createSnapshots: false);
}
#endif

nodes = GuardedAddBoundTreeAndGetBoundNodeFromMap(lambdaOrQuery, boundOuterExpression);
Expand Down Expand Up @@ -1914,13 +1917,16 @@ 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 (!Compilation.NullableSemanticAnalysisEnabled)
if (!Compilation.IsNullableAnalysisEnabled)
{
return;
}
// If we're in DEBUG mode, enable the analysis unless explicitly disabled, but throw away the results
#if DEBUG
if (Compilation.IsNullableAnalysisExplicitlyDisabled)
#endif
{
return;
}
}

// If we have a snapshot manager, then we've already done
// all the work necessary and we should avoid taking an
Expand All @@ -1941,7 +1947,7 @@ protected void EnsureNullabilityAnalysisPerformedIfNecessary()
// first BoundNode corresponds to the underlying EqualsValueSyntax of the initializer)
if (_guardedNodeMap.Count > 0)
{
Debug.Assert(!Compilation.NullableSemanticAnalysisEnabled ||
Debug.Assert(!Compilation.IsNullableAnalysisEnabled ||
_guardedNodeMap.ContainsKey(bindableRoot) ||
_guardedNodeMap.ContainsKey(bind(bindableRoot, getDiagnosticBag(), out _).Syntax));
return;
Expand Down Expand Up @@ -2001,9 +2007,12 @@ BoundNode bind(CSharpSyntaxNode root, DiagnosticBag diagnosticBag, out Binder bi
void rewriteAndCache(DiagnosticBag diagnosticBag)
{
#if DEBUG
if (!Compilation.NullableSemanticAnalysisEnabled)
if (!Compilation.IsNullableAnalysisEnabled)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

{
AnalyzeBoundNodeNullability(boundRoot, binder, diagnosticBag, createSnapshots: true);
if (!Compilation.IsNullableAnalysisExplicitlyDisabled)
{
AnalyzeBoundNodeNullability(boundRoot, binder, diagnosticBag, createSnapshots: true);
}
return;
}
#endif
Expand All @@ -2014,7 +2023,7 @@ void rewriteAndCache(DiagnosticBag diagnosticBag)

void cache(CSharpSyntaxNode bindableRoot, BoundNode boundRoot, NullableWalker.SnapshotManager snapshotManager, ImmutableDictionary<Symbol, Symbol> remappedSymbols)
{
Debug.Assert(Compilation.NullableSemanticAnalysisEnabled);
Debug.Assert(Compilation.IsNullableAnalysisEnabled);
GuardedAddBoundTreeForStandaloneSyntax(bindableRoot, boundRoot, snapshotManager, remappedSymbols);
}

Expand All @@ -2023,7 +2032,7 @@ DiagnosticBag getDiagnosticBag()
// In DEBUG without nullable analysis enabled, we want to use a temp diagnosticbag
// that can't produce any observable side effects
#if DEBUG
if (!Compilation.NullableSemanticAnalysisEnabled)
if (!Compilation.IsNullableAnalysisEnabled)
{
return new DiagnosticBag();
}
Expand Down Expand Up @@ -2323,7 +2332,7 @@ internal override Symbol RemapSymbolIfNecessaryCore(Symbol symbol)
Debug.Assert(symbol is LocalSymbol ||
symbol is ParameterSymbol ||
symbol is MethodSymbol { MethodKind: MethodKind.LambdaMethod });
Debug.Assert(Compilation.NullableSemanticAnalysisEnabled);
Debug.Assert(Compilation.IsNullableAnalysisEnabled);
EnsureNullabilityAnalysisPerformedIfNecessary();

if (_lazyRemappedSymbols is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ internal override BoundExpression GetSpeculativelyBoundExpression(int position,

internal AttributeSemanticModel CreateSpeculativeAttributeSemanticModel(int position, AttributeSyntax attribute, Binder binder, AliasSymbol aliasOpt, NamedTypeSymbol attributeType)
{
var memberModel = Compilation.NullableSemanticAnalysisEnabled ? GetMemberModel(position) : null;
var memberModel = Compilation.IsNullableAnalysisEnabled ? GetMemberModel(position) : null;
return AttributeSemanticModel.CreateSpeculative(this, attribute, attributeType, aliasOpt, binder, memberModel?.GetRemappedSymbols(), position);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
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.IsNullableAnalysisEnabled)
{
methodBodyForSemanticModel = NullableWalker.AnalyzeAndRewrite(
compilation,
Expand Down
Loading