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

Address some PROTOTYPE comments #29607

Merged
merged 4 commits into from
Aug 31, 2018
Merged

Address some PROTOTYPE comments #29607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19382af
Update PROTOTYPE comments
cston Aug 30, 2018
52688af
More
cston Aug 31, 2018
a34125c
PR feedback
cston Aug 31, 2018
d477d66
Update comment
cston Aug 31, 2018
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
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,7 +1405,6 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA
}
else
{
// PROTOTYPE(NullableReferenceTypes): this TypeSymbolWithAnnotations has bad annotation and context
newArguments[i] = ((OutVariablePendingInference)argument).SetInferredType(TypeSymbolWithAnnotations.Create(candidateType), null);
}
}
Expand All @@ -1417,7 +1416,6 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA
}
else
{
// PROTOTYPE(NullableReferenceTypes): this TypeSymbolWithAnnotations has bad annotation and context
newArguments[i] = ((BoundDiscardExpression)argument).SetInferredType(TypeSymbolWithAnnotations.Create(candidateType));
}
}
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1354,7 +1354,6 @@ private BoundExpression InferTypeForDiscardAssignment(BoundDiscardExpression op1
diagnostics.Add(ErrorCode.ERR_VoidAssignment, op1.Syntax.Location);
}

// PROTOTYPE(NullableReferenceTypes): this TypeSymbolWithAnnotations has bad annotation and context
return op1.SetInferredType(TypeSymbolWithAnnotations.Create(inferredType));
}

Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,9 +1366,9 @@ private static bool HasIdentityConversionInternal(TypeSymbol type1, TypeSymbol t
Debug.Assert((object)type1 != null);
Debug.Assert((object)type2 != null);

// PROTOTYPE(NullableReferenceTypes): If two types differ only by nullability and
// https://github.com/dotnet/roslyn/issues/27961: If two types differ only by nullability and
// one has IsNullable unset and the other doesn't, which do we choose in inference?
// See test TypeInference_04.
// See NullableReferenceTypesTests.TypeInference_04.
var compareKind = includeNullability ?
TypeCompareKind.AllIgnoreOptions | TypeCompareKind.CompareNullableModifiersForReferenceTypes | TypeCompareKind.UnknownNullableModifierMatchesAny :
TypeCompareKind.AllIgnoreOptions;
Expand Down
15 changes: 4 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2474,7 +2474,7 @@ private static TypeSymbolWithAnnotations Fix(
// Make a copy; don't modify the collection as we're iterating it.
foreach (var candidate in initialCandidates)
{
if (!bound.Equals(candidate, TypeCompareKind.AllAspects))
if (!bound.Equals(candidate, TypeCompareKind.CompareNullableModifiersForReferenceTypes))
{
if (!ImplicitConversionExists(bound, candidate, ref useSiteDiagnostics, conversions, includeNullability))
{
Expand All @@ -2498,7 +2498,7 @@ private static TypeSymbolWithAnnotations Fix(
{
foreach (var candidate in initialCandidates)
{
if (!bound.Equals(candidate, TypeCompareKind.AllAspects))
if (!bound.Equals(candidate, TypeCompareKind.CompareNullableModifiersForReferenceTypes))
{
if (!ImplicitConversionExists(candidate, bound, ref useSiteDiagnostics, conversions, includeNullability))
{
Expand All @@ -2524,7 +2524,7 @@ private static TypeSymbolWithAnnotations Fix(
{
foreach (var candidate2 in initialCandidates)
{
if (!candidate.Equals(candidate2, TypeCompareKind.AllAspects) &&
if (!candidate.Equals(candidate2, TypeCompareKind.CompareNullableModifiersForReferenceTypes) &&
!ImplicitConversionExists(candidate2, candidate, ref useSiteDiagnostics, conversions, includeNullability))
{
goto OuterBreak;
Expand All @@ -2546,7 +2546,7 @@ private static TypeSymbolWithAnnotations Fix(
Debug.Assert(!(best.IsObjectType() && candidate.IsDynamic()));
Debug.Assert(!(best.IsDynamic() && candidate.IsObjectType()));

best = Merge(best, candidate, conversions.CorLibrary);
best = MergeNullability(MergeTupleNames(MergeDynamic(best, candidate, conversions.CorLibrary), candidate), candidate);
}
else
{
Expand Down Expand Up @@ -2580,12 +2580,6 @@ internal static TypeSymbol Merge(TypeSymbol first, TypeSymbol second, AssemblySy
return MergeNullability(MergeTupleNames(MergeDynamic(firstWithAnnotations, secondWithAnnotations, corLibrary), secondWithAnnotations), secondWithAnnotations).TypeSymbol;
}

// PROTOTYPE(NullableReferenceTypes): Remove this overload.
internal static TypeSymbolWithAnnotations Merge(TypeSymbolWithAnnotations first, TypeSymbolWithAnnotations second, AssemblySymbol corLibrary)
{
return MergeNullability(MergeTupleNames(MergeDynamic(first, second, corLibrary), second), second);
}

/// <summary>
/// Returns first or a modified version of first with merged dynamic flags from both types.
/// </summary>
Expand Down Expand Up @@ -2678,7 +2672,6 @@ private static bool ImplicitConversionExists(TypeSymbolWithAnnotations sourceWit
return false;
}

// PROTOTYPE(NullableReferenceTypes): Pass includeNullability to ClassifyImplicitConversionFromType.
return conversions.ClassifyImplicitConversionFromType(source, destination, ref useSiteDiagnostics).Exists;
}

Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1734,7 +1734,6 @@
</Node>

<!-- Node only used during nullability flow analysis to represent an expression with an updated nullability -->
<!-- PROTOTYPE(NullableReferenceTypes): Derive from BoundValuePlaceholderBase. -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot derive from BoundValuePlaceholderBase since that type requires non-null Type.

<Node Name="BoundExpressionWithNullability" Base="BoundExpression">
<Field Name="Expression" Type="BoundExpression" Null="disallow"/>
<Field Name="Type" Type="TypeSymbol" Override="true" Null="allow"/> <!-- We use null Type for placeholders representing out vars -->
Expand Down
40 changes: 20 additions & 20 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 @@ -28,8 +28,8 @@ internal sealed partial class NullableWalker : DataFlowPassBase<NullableWalker.L
/// </summary>
internal sealed class VariableState
{
// PROTOTYPE(NullableReferenceTypes): Reference the collections directly from the original
// NullableWalker ratherthan coping the collections. (Items are added to the collections
// Consider referencing the collections directly from the original NullableWalker
// rather than coping the collections. (Items are added to the collections
// but never replaced so the collections are lazily populated but otherwise immutable.)
internal readonly ImmutableDictionary<VariableIdentifier, int> VariableSlot;
internal readonly ImmutableArray<VariableIdentifier> VariableBySlot;
Expand Down Expand Up @@ -1013,7 +1013,7 @@ public override BoundNode VisitLocalDeclaration(BoundLocalDeclaration node)
protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node)
{
Debug.Assert(!IsConditionalState);
_resultType = _invalidType; // PROTOTYPE(NullableReferenceTypes): Move to `Visit` method?
_resultType = _invalidType;
var result = base.VisitExpressionWithoutStackGuard(node);
#if DEBUG
// Verify Visit method set _result.
Expand Down Expand Up @@ -1376,7 +1376,7 @@ private TypeSymbolWithAnnotations InferResultNullability(BinaryOperatorKind oper
// PROTOTYPE(NullableReferenceTypes): Conversions: Lifted operator
return TypeSymbolWithAnnotations.Create(resultType);
}
// PROTOTYPE(NullableReferenceTypes): Update method based on operand types.
// Update method based on operand types: see https://github.com/dotnet/roslyn/issues/29605.
if ((object)methodOpt != null && methodOpt.ParameterCount == 2)
{
return methodOpt.ReturnType;
Expand Down Expand Up @@ -1418,7 +1418,7 @@ private TypeSymbolWithAnnotations InferResultNullability(BinaryOperatorKind oper
protected override void AfterLeftChildHasBeenVisited(BoundBinaryOperator binary)
{
Debug.Assert(!IsConditionalState);
//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
TypeSymbolWithAnnotations leftType = _resultType;
bool warnOnNullReferenceArgument = (binary.OperatorKind.IsUserDefined() && (object)binary.MethodOpt != null && binary.MethodOpt.ParameterCount == 2);
Expand Down Expand Up @@ -1897,7 +1897,7 @@ public override BoundNode VisitCall(BoundCall node)
{
VisitRvalue(receiverOpt);
CheckPossibleNullReceiver(receiverOpt);
// PROTOTYPE(NullableReferenceTypes): Update method based on inferred receiver type.
// Update method based on inferred receiver type: see https://github.com/dotnet/roslyn/issues/29605.
}

// PROTOTYPE(NullableReferenceTypes): Can we handle some error cases?
Expand All @@ -1919,7 +1919,7 @@ public override BoundNode VisitCall(BoundCall node)
ReplayReadsAndWrites(localFunc, node.Syntax, writes: true);
}

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
_resultType = method.ReturnType;
}
Expand Down Expand Up @@ -3041,7 +3041,7 @@ private TypeSymbolWithAnnotations ApplyConversion(
useLegacyWarnings,
assignmentKind);

// PROTOTYPE(NullableReferenceTypes): Update method based on operandType
// Update method based on operandType: see https://github.com/dotnet/roslyn/issues/29605.
// (see NullableReferenceTypesTests.ImplicitConversions_07).
var methodOpt = conversion.Method;
Debug.Assert((object)methodOpt != null);
Expand Down Expand Up @@ -3215,7 +3215,7 @@ public override BoundNode VisitMethodGroup(BoundMethodGroup node)
CheckPossibleNullReceiver(receiverOpt);
}

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
_resultType = default;
}
Expand Down Expand Up @@ -3326,9 +3326,9 @@ private static bool UseLegacyWarnings(BoundExpression expr)

public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstructionAssignmentOperator node)
{
// PROTOTYPE(NullableReferenceTypes): Assign each of the deconstructed values.
// https://github.com/dotnet/roslyn/issues/29618: Assign each of the deconstructed values,
// and handle deconstruction conversion for node.Right.
VisitLvalue(node.Left);
// PROTOTYPE(NullableReferenceTypes): Handle deconstruction conversion node.Right.
VisitRvalue(node.Right.Operand);
SetResult(node);
return null;
Expand Down Expand Up @@ -3439,7 +3439,7 @@ public override BoundNode VisitCompoundAssignmentOperator(BoundCompoundAssignmen
TypeSymbolWithAnnotations resultType;
Debug.Assert(!IsConditionalState);

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
TypeSymbolWithAnnotations leftOnRightType = GetAdjustedResult(leftType, MakeSlot(node.Left));

Expand Down Expand Up @@ -3602,7 +3602,7 @@ private void VisitMemberAccess(BoundExpression receiverOpt, Symbol member, bool
{
Debug.Assert(!IsConditionalState);

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
VisitRvalue(receiverOpt);

Expand Down Expand Up @@ -3719,7 +3719,7 @@ public override BoundNode VisitUnaryOperator(BoundUnaryOperator node)
var result = base.VisitUnaryOperator(node);
TypeSymbolWithAnnotations resultType = default;

// PROTOTYPE(NullableReferenceTypes): Update method based on inferred operand type.
// Update method based on inferred operand type: see https://github.com/dotnet/roslyn/issues/29605.
if (node.OperatorKind.IsUserDefined())
{
if (node.OperatorKind.IsLifted())
Expand Down Expand Up @@ -3779,7 +3779,7 @@ private TypeSymbolWithAnnotations InferResultNullability(BoundUserDefinedConditi
// PROTOTYPE(NullableReferenceTypes): Conversions: Lifted operator
return TypeSymbolWithAnnotations.Create(node.Type);
}
// PROTOTYPE(NullableReferenceTypes): Update method based on inferred operand types.
// Update method based on inferred operand types: see https://github.com/dotnet/roslyn/issues/29605.
if ((object)node.LogicalOperator != null && node.LogicalOperator.ParameterCount == 2)
{
return node.LogicalOperator.ReturnType;
Expand All @@ -3793,7 +3793,7 @@ private TypeSymbolWithAnnotations InferResultNullability(BoundUserDefinedConditi
protected override void AfterLeftChildOfBinaryLogicalOperatorHasBeenVisited(BoundExpression node, BoundExpression right, bool isAnd, bool isBool, ref LocalState leftTrue, ref LocalState leftFalse)
{
Debug.Assert(!IsConditionalState);
//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
TypeSymbolWithAnnotations leftType = _resultType;
// PROTOTYPE(NullableReferenceTypes): Update operator methods based on inferred operand types.
Expand Down Expand Up @@ -3872,7 +3872,7 @@ public override BoundNode VisitAwaitExpression(BoundAwaitExpression node)
}
else
{
// PROTOTYPE(NullableReferenceTypes): Update method based on inferred receiver type.
// Update method based on inferred receiver type: see https://github.com/dotnet/roslyn/issues/29605.
_resultType = node.GetResult.ReturnType;
}
return result;
Expand Down Expand Up @@ -3939,7 +3939,7 @@ public override BoundNode VisitAsOperator(BoundAsOperator node)
{
var result = base.VisitAsOperator(node);

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
bool? isNullable = null;
if (!node.Type.IsValueType)
Expand Down Expand Up @@ -3982,7 +3982,7 @@ public override BoundNode VisitSuppressNullableWarningExpression(BoundSuppressNu
{
base.VisitSuppressNullableWarningExpression(node);

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
_resultType = _resultType.IsNull ? default : _resultType.WithTopLevelNonNullabilityForReferenceTypes();
}
Expand Down Expand Up @@ -4018,7 +4018,7 @@ public override BoundNode VisitLiteral(BoundLiteral node)
var result = base.VisitLiteral(node);

Debug.Assert(!IsConditionalState);
//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
//if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
{
var constant = node.ConstantValue;

Expand Down
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,8 @@ internal override TypeSymbol SetUnknownNullabilityForReferenceTypes()
/// parameters in the type.</param>
public NamedTypeSymbol Construct(params TypeSymbol[] typeArguments)
{
return ConstructWithoutModifiers(typeArguments.AsImmutableOrNull(), false, nonNullTypesContext: null);
// PROTOTYPE(NullableReferenceTypes): We should fix the callers to pass an explicit context.
return ConstructWithoutModifiers(typeArguments.AsImmutableOrNull(), false, NonNullTypesFalseContext.Instance);
}

/// <summary>
Expand All @@ -865,7 +866,7 @@ public NamedTypeSymbol Construct(INonNullTypesContext nonNullTypesContext, param
public NamedTypeSymbol Construct(ImmutableArray<TypeSymbol> typeArguments, INonNullTypesContext nonNullTypesContext = null)
{
// PROTOTYPE(NullableReferenceTypes): We should fix the callers to pass an explicit context.
return ConstructWithoutModifiers(typeArguments, false, nonNullTypesContext);
return ConstructWithoutModifiers(typeArguments, false, nonNullTypesContext ?? NonNullTypesFalseContext.Instance);
}

/// <summary>
Expand All @@ -875,7 +876,7 @@ public NamedTypeSymbol Construct(ImmutableArray<TypeSymbol> typeArguments, INonN
public NamedTypeSymbol Construct(IEnumerable<TypeSymbol> typeArguments)
{
// PROTOTYPE(NullableReferenceTypes): We should fix the callers to pass an explicit context.
return ConstructWithoutModifiers(typeArguments.AsImmutableOrNull(), false, nonNullTypesContext: null);
return ConstructWithoutModifiers(typeArguments.AsImmutableOrNull(), false, NonNullTypesFalseContext.Instance);
}

/// <summary>
Expand Down
Loading