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

Track state separately in NullableWalker for nested and containing methods #50417

Merged
merged 16 commits into from
Jan 23, 2021
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ private BoundLambda SuppressIfNeeded(BoundLambda lambda)
public bool HasExplicitlyTypedParameterList { get { return Data.HasExplicitlyTypedParameterList; } }
public int ParameterCount { get { return Data.ParameterCount; } }
public TypeWithAnnotations InferReturnType(ConversionsBase conversions, NamedTypeSymbol delegateType, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
=> BindForReturnTypeInference(delegateType).GetInferredReturnType(conversions, _nullableState?.Clone(), ref useSiteDiagnostics);
=> BindForReturnTypeInference(delegateType).GetInferredReturnType(conversions, _nullableState, ref useSiteDiagnostics);

public RefKind RefKind(int index) { return Data.RefKind(index); }
public void GenerateAnonymousFunctionConversionError(DiagnosticBag diagnostics, TypeSymbol targetType) { Data.GenerateAnonymousFunctionConversionError(diagnostics, targetType); }
Expand Down
14 changes: 11 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1634,6 +1634,14 @@ void updatePendingBranchState(ref TLocalState stateToUpdate, ref TLocalState sta

protected Optional<TLocalState> NonMonotonicState;

/// <summary>
/// Join state from other try block, potentially in a nested method.
/// </summary>
protected virtual void JoinTryBlockState(ref TLocalState self, ref TLocalState other)
{
Join(ref self, ref other);
}

private void VisitTryBlockWithAnyTransferFunction(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState)
{
if (_nonMonotonicTransfer)
Expand All @@ -1646,7 +1654,7 @@ private void VisitTryBlockWithAnyTransferFunction(BoundStatement tryBlock, Bound
if (oldTryState.HasValue)
{
var oldTryStateValue = oldTryState.Value;
Join(ref oldTryStateValue, ref tempTryStateValue);
JoinTryBlockState(ref oldTryStateValue, ref tempTryStateValue);
oldTryState = oldTryStateValue;
}

Expand Down Expand Up @@ -1675,7 +1683,7 @@ private void VisitCatchBlockWithAnyTransferFunction(BoundCatchBlock catchBlock,
if (oldTryState.HasValue)
{
var oldTryStateValue = oldTryState.Value;
Join(ref oldTryStateValue, ref tempTryStateValue);
JoinTryBlockState(ref oldTryStateValue, ref tempTryStateValue);
oldTryState = oldTryStateValue;
}

Expand Down Expand Up @@ -1720,7 +1728,7 @@ private void VisitFinallyBlockWithAnyTransferFunction(BoundStatement finallyBloc
if (oldTryState.HasValue)
{
var oldTryStateValue = oldTryState.Value;
Join(ref oldTryStateValue, ref tempTryStateValue);
JoinTryBlockState(ref oldTryStateValue, ref tempTryStateValue);
oldTryState = oldTryStateValue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public AbstractLocalFunctionState(TLocalState stateFromBottom, TLocalState state
public bool Visited = false;
}

protected abstract TLocalFunctionState CreateLocalFunctionState();
protected abstract TLocalFunctionState CreateLocalFunctionState(LocalFunctionSymbol symbol);

private SmallDictionary<LocalFunctionSymbol, TLocalFunctionState>? _localFuncVarUsages = null;

Expand All @@ -50,7 +50,7 @@ protected TLocalFunctionState GetOrCreateLocalFuncUsages(LocalFunctionSymbol loc

if (!_localFuncVarUsages.TryGetValue(localFunc, out TLocalFunctionState? usages))
{
usages = CreateLocalFunctionState();
usages = CreateLocalFunctionState(localFunc);
_localFuncVarUsages[localFunc] = usages;
}
return usages;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public LocalFunctionState(LocalState unreachableState)
{ }
}

protected override LocalFunctionState CreateLocalFunctionState() => new LocalFunctionState(UnreachableState());
protected override LocalFunctionState CreateLocalFunctionState(LocalFunctionSymbol symbol) => new LocalFunctionState(UnreachableState());

protected override bool Meet(ref LocalState self, ref LocalState other)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ public LocalFunctionState(LocalState stateFromBottom, LocalState stateFromTop)
{ }
}

protected override LocalFunctionState CreateLocalFunctionState()
protected override LocalFunctionState CreateLocalFunctionState(LocalFunctionSymbol symbol)
=> CreateLocalFunctionState();

private LocalFunctionState CreateLocalFunctionState()
=> new LocalFunctionState(
// The bottom state should assume all variables, even new ones, are assigned
new LocalState(BitVector.AllSet(nextVariableSlot), normalizeToBottom: true),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand Down Expand Up @@ -87,7 +85,7 @@ public bool Equals(VariableIdentifier other)
return Symbol.Equals(other.Symbol, TypeCompareKind.AllIgnoreOptions);
}

public override bool Equals(object obj)
public override bool Equals(object? obj)
{
throw ExceptionUtilities.Unreachable;
}
Expand Down
71 changes: 71 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define REFERENCE_STATE
#endif

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand All @@ -38,6 +39,29 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass<
DefiniteAssignmentPass.LocalState,
DefiniteAssignmentPass.LocalFunctionState>
{
/// <summary>
/// A mapping from local variables to the index of their slot in a flow analysis local state.
/// </summary>
private readonly PooledDictionary<VariableIdentifier, int> _variableSlot = PooledDictionary<VariableIdentifier, int>.GetInstance();
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 20, 2021

Choose a reason for hiding this comment

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

_variableSlot [](start = 67, length = 13)

It looks naming of new fields is inconsistent around the use of "_". #Closed

Copy link
Member Author

@cston cston Jan 21, 2021

Choose a reason for hiding this comment

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

These fields were moved from the base class, with the names unchanged. The names having a leading underscore for private fields, and no underscore for protected fields.


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


/// <summary>
/// A mapping from the local variable slot to the symbol for the local variable itself. This
/// is used in the implementation of region analysis (support for extract method) to compute
/// the set of variables "always assigned" in a region of code.
///
/// The first slot, slot 0, is reserved for indicating reachability, so the first tracked variable will
/// be given slot 1. When referring to VariableIdentifier.ContainingSlot, slot 0 indicates
/// that the variable in VariableIdentifier.Symbol is a root, i.e. not nested within another
/// tracked variable. Slots &lt; 0 are illegal.
Copy link
Member

@333fred 333fred Jan 15, 2021

Choose a reason for hiding this comment

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

< [](start = 36, length = 4)

Consider just saying "less than" in english so I don't have to remember that &lt; is how you write < in xml :) #Closed

/// </summary>
protected VariableIdentifier[] variableBySlot = new VariableIdentifier[1];

/// <summary>
/// Variable slots are allocated to local variables sequentially and never reused. This is
/// the index of the next slot number to use.
/// </summary>
protected int nextVariableSlot = 1;

/// <summary>
/// Some variables that should be considered initially assigned. Used for region analysis.
/// </summary>
Expand Down Expand Up @@ -192,6 +216,7 @@ internal DefiniteAssignmentPass(

protected override void Free()
{
_variableSlot.Free();
_usedVariables.Free();
_readParameters?.Free();
_usedLocalFunctions.Free();
Expand All @@ -204,6 +229,52 @@ protected override void Free()
base.Free();
}

protected override bool TryGetVariable(VariableIdentifier identifier, out int slot)
{
return _variableSlot.TryGetValue(identifier, out slot);
}

protected override int AddVariable(VariableIdentifier identifier)
{
int slot = nextVariableSlot++;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 21, 2021

Choose a reason for hiding this comment

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

nextVariableSlot [](start = 23, length = 16)

Would it make sense to get rid of this field altogether? Can we simply look at _variableSlot to figure out the next available slot? #Closed

_variableSlot.Add(identifier, slot);
if (slot >= variableBySlot.Length)
{
Array.Resize(ref this.variableBySlot, slot * 2);
Copy link
Member

@333fred 333fred Jan 15, 2021

Choose a reason for hiding this comment

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

I know this stuff is just copied, but do we want to consider just making variableBySlot an ArrayBuilder, which handles this already? #Closed

}

variableBySlot[slot] = identifier;
return slot;
}

protected Symbol GetNonMemberSymbol(int slot)
{
VariableIdentifier variableId = variableBySlot[slot];
while (variableId.ContainingSlot > 0)
{
Debug.Assert(variableId.Symbol.Kind == SymbolKind.Field || variableId.Symbol.Kind == SymbolKind.Property || variableId.Symbol.Kind == SymbolKind.Event,
"inconsistent property symbol owner");
variableId = variableBySlot[variableId.ContainingSlot];
}
return variableId.Symbol;
}

private int RootSlot(int slot)
{
while (true)
{
int containingSlot = variableBySlot[slot].ContainingSlot;
if (containingSlot == 0)
{
return slot;
}
else
{
slot = containingSlot;
}
}
}

#if DEBUG
protected override void VisitRvalue(BoundExpression node, bool isKnownToBeAnLvalue = false)
{
Expand Down
97 changes: 8 additions & 89 deletions src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand All @@ -27,31 +26,6 @@ internal interface ILocalDataFlowState : ILocalState
bool NormalizeToBottom { get; }
}

/// <summary>
/// A mapping from local variables to the index of their slot in a flow analysis local state.
/// </summary>
protected PooledDictionary<VariableIdentifier, int> _variableSlot = PooledDictionary<VariableIdentifier, int>.GetInstance();

/// <summary>
/// A mapping from the local variable slot to the symbol for the local variable itself. This
/// is used in the implementation of region analysis (support for extract method) to compute
/// the set of variables "always assigned" in a region of code.
///
/// The first slot, slot 0, is reserved for indicating reachability, so the first tracked variable will
/// be given slot 1. When referring to <see cref="VariableIdentifier.ContainingSlot"/>, slot 0 indicates
/// that the variable in <see cref="VariableIdentifier.Symbol"/> is a root, i.e. not nested within another
/// tracked variable. Slots &lt; 0 are illegal.
/// </summary>
protected VariableIdentifier[] variableBySlot = new VariableIdentifier[1];

/// <summary>
/// Variable slots are allocated to local variables sequentially and never reused. This is
/// the index of the next slot number to use.
/// </summary>
protected int nextVariableSlot = 1;

private readonly int _maxSlotDepth;

/// <summary>
/// A cache for remember which structs are empty.
/// </summary>
Expand All @@ -62,12 +36,10 @@ protected LocalDataFlowPass(
Symbol? member,
BoundNode node,
EmptyStructTypeCache emptyStructs,
bool trackUnassignments,
int maxSlotDepth = 0)
bool trackUnassignments)
: base(compilation, member, node, nonMonotonicTransferFunction: trackUnassignments)
{
Debug.Assert(emptyStructs != null);
_maxSlotDepth = maxSlotDepth;
_emptyStructTypeCache = emptyStructs;
}

Expand All @@ -85,11 +57,9 @@ protected LocalDataFlowPass(
_emptyStructTypeCache = emptyStructs;
}

protected override void Free()
{
_variableSlot.Free();
base.Free();
}
protected abstract bool TryGetVariable(VariableIdentifier identifier, out int slot);

protected abstract int AddVariable(VariableIdentifier identifier);

/// <summary>
/// Locals are given slots when their declarations are encountered. We only need give slots
Expand All @@ -107,7 +77,7 @@ protected int VariableSlot(Symbol symbol, int containingSlot = 0)
containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, forceContainingSlotsToExist: false);

int slot;
return (_variableSlot.TryGetValue(new VariableIdentifier(symbol, containingSlot), out slot)) ? slot : -1;
return TryGetVariable(new VariableIdentifier(symbol, containingSlot), out slot) ? slot : -1;
}

protected virtual bool IsEmptyStructType(TypeSymbol type)
Expand Down Expand Up @@ -137,7 +107,7 @@ protected virtual int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, boo
int slot;

// Since analysis may proceed in multiple passes, it is possible the slot is already assigned.
if (!_variableSlot.TryGetValue(identifier, out slot))
if (!TryGetVariable(identifier, out slot))
{
if (!createIfMissing)
{
Expand All @@ -150,19 +120,7 @@ protected virtual int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, boo
return -1;
}

if (_maxSlotDepth > 0 && GetSlotDepth(containingSlot) >= _maxSlotDepth)
{
return -1;
}

slot = nextVariableSlot++;
_variableSlot.Add(identifier, slot);
if (slot >= variableBySlot.Length)
{
Array.Resize(ref this.variableBySlot, slot * 2);
}

variableBySlot[slot] = identifier;
slot = AddVariable(identifier);
}

if (IsConditionalState)
Expand All @@ -178,17 +136,6 @@ protected virtual int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, boo
return slot;
}

private int GetSlotDepth(int slot)
{
int depth = 0;
while (slot > 0)
{
depth++;
slot = variableBySlot[slot].ContainingSlot;
}
return depth;
}

/// <summary>
/// Sets the starting state for any newly declared variables in the LocalDataFlowPass.
/// </summary>
Expand Down Expand Up @@ -227,7 +174,7 @@ private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot,
}
else
{
if (!_variableSlot.TryGetValue(new VariableIdentifier(restField, containingSlot), out containingSlot))
if (!TryGetVariable(new VariableIdentifier(restField, containingSlot), out containingSlot))
{
return -1;
}
Expand All @@ -242,18 +189,6 @@ private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot,

protected abstract bool TryGetReceiverAndMember(BoundExpression expr, out BoundExpression? receiver, [NotNullWhen(true)] out Symbol? member);

protected Symbol GetNonMemberSymbol(int slot)
{
VariableIdentifier variableId = variableBySlot[slot];
while (variableId.ContainingSlot > 0)
{
Debug.Assert(variableId.Symbol.Kind == SymbolKind.Field || variableId.Symbol.Kind == SymbolKind.Property || variableId.Symbol.Kind == SymbolKind.Event,
"inconsistent property symbol owner");
variableId = variableBySlot[variableId.ContainingSlot];
}
return variableId.Symbol;
}

/// <summary>
/// Return the slot for a variable, or -1 if it is not tracked (because, for example, it is an empty struct).
/// </summary>
Expand Down Expand Up @@ -310,22 +245,6 @@ protected int MakeMemberSlot(BoundExpression? receiverOpt, Symbol member)
return GetOrCreateSlot(member, containingSlot);
}

protected int RootSlot(int slot)
{
while (true)
{
ref var varInfo = ref variableBySlot[slot];
if (varInfo.ContainingSlot == 0)
{
return slot;
}
else
{
slot = varInfo.ContainingSlot;
}
}
}

protected static bool HasInitializer(Symbol field) => field switch
{
SourceMemberFieldSymbol f => f.HasInitializer,
Expand Down
Loading