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

Jump to bottom

Cleanup of lightup layer after MS.CA upgrade #6762

Merged
merged 1 commit into from
Jul 13, 2023
Merged

Cleanup of lightup layer after MS.CA upgrade #6762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion ...Compiler/Lightup/ITypeSymbolExtensions.cs → ...s/Core/Analyzers/ITypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,5 @@ private static readonly Func<ITypeSymbol, NullableAnnotation> s_nullableAnnotati

public static NullableAnnotation NullableAnnotation(this ITypeSymbol typeSymbol)
=> s_nullableAnnotation(typeSymbol);

}
}
0 ...es/Compiler/Lightup/NullableAnnotation.cs → ...zers/Core/Analyzers/NullableAnnotation.cs
View file
Open in desktop
File renamed without changes.
0 ...ities/Compiler/Lightup/NullableContext.cs → ...alyzers/Core/Analyzers/NullableContext.cs
View file
Open in desktop
File renamed without changes.
0 ...iler/Lightup/NullableContextExtensions.cs → ...re/Analyzers/NullableContextExtensions.cs
View file
Open in desktop
File renamed without changes.
5 changes: 0 additions & 5 deletions src/Utilities/Compiler/Analyzer.Utilities.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,9 @@
<Compile Include="$(MSBuildThisFileDirectory)Extensions\WellKnownDiagnosticTagsExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Index.cs" />
<Compile Include="$(MSBuildThisFileDirectory)IsExternalInit.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\INegatedPatternOperationWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\IOperationWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\ITypeSymbolExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\IFunctionPointerInvocationOperationWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\LightupHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableAnnotation.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableContext.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableContextExtensions.cs" />
Comment on lines -70 to -75
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed files moved to PublicApiAnalyzers project, which is still on MS.CA 1.2.1

Copy link
Member

Choose a reason for hiding this comment

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

1.2.1? That is Visual Studio 2015? Do we still care about such an old version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sharwell?

Copy link
Member

Choose a reason for hiding this comment

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

There isn't any motivating need to update. It works fine and there is no indication that will change.

<Compile Include="$(MSBuildThisFileDirectory)Lightup\OperationKindEx.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\OperationWrapperHelper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Options\MSBuildItemOptionNames.cs" />
Expand Down
51 changes: 0 additions & 51 deletions src/Utilities/Compiler/Lightup/INegatedPatternOperationWrapper.cs
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions src/Utilities/Compiler/Lightup/OperationWrapperHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ internal static class OperationWrapperHelper
private static readonly Assembly s_codeAnalysisAssembly = typeof(SyntaxNode).GetTypeInfo().Assembly;

private static readonly ImmutableDictionary<Type, Type?> WrappedTypes = ImmutableDictionary.Create<Type, Type?>()
.Add(typeof(IFunctionPointerInvocationOperationWrapper), s_codeAnalysisAssembly.GetType(IFunctionPointerInvocationOperationWrapper.WrappedTypeName))
.Add(typeof(INegatedPatternOperationWrapper), s_codeAnalysisAssembly.GetType(INegatedPatternOperationWrapper.WrappedTypeName));
.Add(typeof(IFunctionPointerInvocationOperationWrapper), s_codeAnalysisAssembly.GetType(IFunctionPointerInvocationOperationWrapper.WrappedTypeName));

/// <summary>
/// Gets the type that is wrapped by the given wrapper.
Expand Down
5 changes: 1 addition & 4 deletions ...owAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,13 @@
using System.Diagnostics;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Analyzer.Utilities.Lightup;
using Analyzer.Utilities.PooledObjects;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis
{
using NullableAnnotation = Analyzer.Utilities.Lightup.NullableAnnotation;

public partial class PointsToAnalysis : ForwardDataFlowAnalysis<PointsToAnalysisData, PointsToAnalysisContext, PointsToAnalysisResult, PointsToBlockAnalysisResult, PointsToAbstractValue>
{
/// <summary>
Expand Down Expand Up @@ -979,7 +976,7 @@ private static bool IsSpecialFactoryMethodReturningNonNullValue(IMethodSymbol me
{
if (!method.IsStatic ||
!method.Name.StartsWith("Create", StringComparison.Ordinal) ||
method.ReturnType.NullableAnnotation() == NullableAnnotation.Annotated)
method.ReturnType.NullableAnnotation == NullableAnnotation.Annotated)
{
return false;
}
Expand Down
61 changes: 22 additions & 39 deletions src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Linq;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Analyzer.Utilities.Lightup;
using Analyzer.Utilities.PooledObjects;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis;
Expand Down Expand Up @@ -1267,9 +1266,8 @@ private bool TryInferConversion(
IPatternOperation patternOperation = isPatternOperation.Pattern;
bool direct = true;

if (INegatedPatternOperationWrapper.IsInstance(patternOperation))
if (patternOperation is INegatedPatternOperation negatedPattern)
{
INegatedPatternOperationWrapper negatedPattern = INegatedPatternOperationWrapper.FromOperation(patternOperation);
patternOperation = negatedPattern.Pattern;
direct = false;
}
Expand Down Expand Up @@ -1582,48 +1580,33 @@ private void PerformPredicateAnalysisCore(IOperation operation, TAnalysisData ta
equals: FlowBranchConditionKind == ControlFlowConditionKind.WhenTrue, isReferenceEquality: false, targetAnalysisData: targetAnalysisData);
break;

default:
// TODO: Remove the below string based checks when we move to Microsoft.CodeAnalysis 3.7 or later.
// TODO: File a tracking bug.
Comment on lines -1586 to -1587
Copy link
Member

Choose a reason for hiding this comment

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

Ouch, this would have worked totally fine with lightup.

var kindStr = isPatternOperation.Pattern.Kind.ToString();
switch (kindStr)
case OperationKind.NegatedPattern:
var negatedPattern = (INegatedPatternOperation)isPatternOperation.Pattern;
if (negatedPattern.Pattern is IConstantPatternOperation negatedConstantPattern)
{
case "NegatedPattern":
if (isPatternOperation.Pattern.Children.FirstOrDefault() is IPatternOperation negatedPattern)
{
if (negatedPattern is IConstantPatternOperation negatedConstantPattern)
{
predicateValueKind = SetValueForEqualsOrNotEqualsComparisonOperator(isPatternOperation.Value, negatedConstantPattern.Value,
equals: FlowBranchConditionKind == ControlFlowConditionKind.WhenFalse, isReferenceEquality: false, targetAnalysisData: targetAnalysisData);
}

break;
}
else
{
goto default;
}
predicateValueKind = SetValueForEqualsOrNotEqualsComparisonOperator(isPatternOperation.Value, negatedConstantPattern.Value,
equals: FlowBranchConditionKind == ControlFlowConditionKind.WhenFalse, isReferenceEquality: false, targetAnalysisData: targetAnalysisData);
}

case "RelationalPattern":
// For the true branch, set the pattern operation value to NotNull.
if (FlowBranchConditionKind == ControlFlowConditionKind.WhenTrue)
{
predicateValueKind = SetValueForIsNullComparisonOperator(isPatternOperation.Value, equals: false, targetAnalysisData: targetAnalysisData);
}
break;

break;
case OperationKind.RelationalPattern:
// For the true branch, set the pattern operation value to NotNull.
if (FlowBranchConditionKind == ControlFlowConditionKind.WhenTrue)
{
predicateValueKind = SetValueForIsNullComparisonOperator(isPatternOperation.Value, equals: false, targetAnalysisData: targetAnalysisData);
}

case "BinaryPattern":
// These high level patterns should not be present in the lowered CFG: https://github.com/dotnet/roslyn/issues/47068
predicateValueKind = PredicateValueKind.Unknown;
break;
break;

default:
Debug.Fail($"Unknown pattern kind '{isPatternOperation.Pattern.Kind}'");
predicateValueKind = PredicateValueKind.Unknown;
break;
}
case OperationKind.BinaryPattern:
// These high level patterns should not be present in the lowered CFG: https://github.com/dotnet/roslyn/issues/47068
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the root cause for #6755. We will need to figure out performing flow analysis in presence of binary pattern operations. I am working on a separate PR for it.

Copy link
Member

Choose a reason for hiding this comment

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

It could be for a bunch of CA1508 too. I'd love to see the correct handling of this will be!

predicateValueKind = PredicateValueKind.Unknown;
break;

default:
Debug.Fail($"Unknown pattern kind '{isPatternOperation.Pattern.Kind}'");
predicateValueKind = PredicateValueKind.Unknown;
break;
}

Expand Down