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

Code cleanup & modernization #31077

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Code cleanup & modernization #31077

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
10 changes: 4 additions & 6 deletions src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,7 @@ private static void AnalyzeNamedTypeSymbol(SymbolAnalysisContext context, INamed
{
var location = declaringSyntax.GetSyntax() switch
{
CSharpSyntax.ClassDeclarationSyntax s when s.BaseList?.Types.Count > 0
=> s.BaseList.Types[0].GetLocation(),
CSharpSyntax.ClassDeclarationSyntax { BaseList.Types.Count: > 0 } s => s.BaseList.Types[0].GetLocation(),
{ } otherSyntax => otherSyntax.GetLocation()
};

Expand Down Expand Up @@ -246,7 +245,8 @@ private static void AnalyzeMethodTypeSymbol(SymbolAnalysisContext context, IMeth
{
var location = declaringSyntax.GetSyntax() switch
{
CSharpSyntax.ParameterSyntax s when s.Type != null => s.Type.GetLocation(),
CSharpSyntax.ParameterSyntax { Type: not null } s => s.Type.GetLocation(),

{ } otherSyntax => otherSyntax.GetLocation()
};

Expand Down Expand Up @@ -283,9 +283,7 @@ private static void ReportDiagnostic(SymbolAnalysisContext context, SyntaxNode s
private static SyntaxNode NarrowDownSyntax(SyntaxNode syntax)
=> syntax switch
{
CSharpSyntax.InvocationExpressionSyntax s
when s.Expression is CSharpSyntax.MemberAccessExpressionSyntax memberAccessSyntax
=> memberAccessSyntax.Name,
CSharpSyntax.InvocationExpressionSyntax { Expression: CSharpSyntax.MemberAccessExpressionSyntax memberAccessSyntax } => memberAccessSyntax.Name,
CSharpSyntax.MemberAccessExpressionSyntax s => s.Name,
CSharpSyntax.ObjectCreationExpressionSyntax s => s.Type,
CSharpSyntax.PropertyDeclarationSyntax s => s.Type,
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore.Cosmos/Diagnostics/Internal/CosmosLoggerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ private static string ExecutingSqlQuery(EventDefinitionBase definition, EventDat
return d.GenerateMessage(
p.ContainerId,
p.LogSensitiveData ? p.PartitionKey : "?",
FormatParameters(p.Parameters, p.LogSensitiveData && p.Parameters.Count > 0),
FormatParameters(p.Parameters, p is { LogSensitiveData: true, Parameters.Count: > 0 }),
Environment.NewLine,
p.QuerySql);
}
Expand Down Expand Up @@ -183,7 +183,7 @@ private static string ExecutedReadNext(EventDefinitionBase definition, EventData
p.ActivityId,
p.ContainerId,
p.LogSensitiveData ? p.PartitionKey : "?",
FormatParameters(p.Parameters, p.LogSensitiveData && p.Parameters.Count > 0),
FormatParameters(p.Parameters, p is { LogSensitiveData: true, Parameters.Count: > 0 }),
Environment.NewLine,
p.QuerySql));
}
Expand Down
6 changes: 2 additions & 4 deletions src/EFCore.Cosmos/Metadata/Conventions/CosmosManyToManyJoinEntityTypeConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,8 @@ private void CreateSkipNavigationForeignKey(
private void ProcessJoinPartitionKey(IConventionSkipNavigation skipNavigation)
{
var inverseSkipNavigation = skipNavigation.Inverse;
if (skipNavigation.JoinEntityType != null
&& skipNavigation.IsCollection
&& inverseSkipNavigation != null
&& inverseSkipNavigation.IsCollection
if (skipNavigation is { JoinEntityType: not null, IsCollection: true }
&& inverseSkipNavigation is { IsCollection: true }
&& inverseSkipNavigation.JoinEntityType == skipNavigation.JoinEntityType)
{
var joinEntityType = skipNavigation.JoinEntityType;
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Cosmos/Metadata/Conventions/CosmosValueGenerationConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public virtual void ProcessEntityTypeAnnotationChanged(
if (propertyType == typeof(int))
{
var ownership = entityType.FindOwnership();
if (ownership != null
&& !ownership.IsUnique
if (ownership is { IsUnique: false }
&& !entityType.IsDocumentRoot())
{
var pk = property.FindContainingPrimaryKey();
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Cosmos/Metadata/Internal/CosmosPropertyExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ public static bool IsOrdinalKeyProperty(this IReadOnlyProperty property)
property.DeclaringEntityType.IsOwned(), $"Expected {property.DeclaringEntityType.DisplayName()} to be owned.");
Check.DebugAssert(property.GetJsonPropertyName().Length == 0, $"Expected {property.Name} to be non-persisted.");

return property.FindContainingPrimaryKey() is IReadOnlyKey key
&& key.Properties.Count > 1
return property.FindContainingPrimaryKey() is IReadOnlyKey { Properties.Count: > 1 }
&& !property.IsForeignKey()
&& property.ClrType == typeof(int)
&& (property.ValueGenerated & ValueGenerated.OnAdd) != 0;
Expand Down
6 changes: 3 additions & 3 deletions src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ protected override Expression VisitMember(MemberExpression memberExpression)
var navigationProjection = innerEntityProjection.BindMember(
memberExpression.Member, innerExpression.Type, clientEval: true, out var propertyBase);

if (!(propertyBase is INavigation navigation)
if (propertyBase is not INavigation navigation
|| !navigation.IsEmbedded())
{
return NullSafeUpdate(innerExpression);
Expand Down Expand Up @@ -509,7 +509,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
navigationProjection = innerEntityProjection.BindMember(
memberName, visitedSource.Type, clientEval: true, out var propertyBase);

if (!(propertyBase is INavigation projectedNavigation)
if (propertyBase is not INavigation projectedNavigation
|| !projectedNavigation.IsEmbedded())
{
return null;
Expand Down Expand Up @@ -567,7 +567,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp

case nameof(Queryable.Select)
when genericMethod == QueryableMethods.Select:
if (!(visitedSource is CollectionShaperExpression shaper))
if (visitedSource is not CollectionShaperExpression shaper)
{
return null;
}
Expand Down
10 changes: 3 additions & 7 deletions src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ protected CosmosQueryableMethodTranslatingExpressionVisitor(
/// </summary>
public override Expression Visit(Expression expression)
{
if (expression is MethodCallExpression methodCallExpression
&& methodCallExpression.Method.IsGenericMethod
if (expression is MethodCallExpression { Method.IsGenericMethod: true } methodCallExpression
&& methodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.FirstOrDefaultWithoutPredicate)
{
if (methodCallExpression.Arguments[0] is MethodCallExpression queryRootMethodCallExpression
Expand All @@ -90,8 +89,7 @@ public override Expression Visit(Expression expression)
{
var entityType = entityQueryRootExpression.EntityType;

if (queryRootMethodCallExpression.Arguments[1] is UnaryExpression unaryExpression
&& unaryExpression.Operand is LambdaExpression lambdaExpression)
if (queryRootMethodCallExpression.Arguments[1] is UnaryExpression { Operand: LambdaExpression lambdaExpression })
{
var queryProperties = new List<IProperty>();
var parameterNames = new List<string>();
Expand Down Expand Up @@ -636,9 +634,7 @@ protected override ShapedQueryExpression TranslateOfType(ShapedQueryExpression s
}

var selectExpression = (SelectExpression)source.QueryExpression;
if (!(translation is SqlConstantExpression sqlConstantExpression
&& sqlConstantExpression.Value is bool constantValue
&& constantValue))
if (translation is not SqlConstantExpression { Value: true })
{
selectExpression.ApplyPredicate(translation);
}
Expand Down
10 changes: 3 additions & 7 deletions ...edQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
projectionExpression = projection.Expression;
storeName = projection.Alias;
}
else if (projectionExpression is UnaryExpression convertExpression
&& convertExpression.NodeType == ExpressionType.Convert)
else if (projectionExpression is UnaryExpression { NodeType: ExpressionType.Convert } convertExpression)
{
// Unwrap EntityProjectionExpression when the root entity is not projected
projectionExpression = ((UnaryExpression)convertExpression.Operand).Operand;
Expand Down Expand Up @@ -155,9 +154,7 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
}
}

if (binaryExpression.Left is MemberExpression memberExpression
&& memberExpression.Member is FieldInfo fieldInfo
&& fieldInfo.IsInitOnly)
if (binaryExpression.Left is MemberExpression { Member: FieldInfo { IsInitOnly: true } } memberExpression)
{
return memberExpression.Assign(Visit(binaryExpression.Right));
}
Expand Down Expand Up @@ -418,8 +415,7 @@ private static void IncludeReference<TIncludingEntity, TIncludedEntity>(
if (relatedEntity != null)
{
fixup(includingEntity, relatedEntity);
if (inverseNavigation != null
&& !inverseNavigation.IsCollection)
if (inverseNavigation is { IsCollection: false })
{
inverseNavigation.SetIsLoadedWhenNoTracking(relatedEntity);
}
Expand Down
2 changes: 1 addition & 1 deletion .../Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ public void Reset()

private bool ShapeResult()
{
var hasNext = !(_item is null);
var hasNext = _item is not null;

_cosmosQueryContext.InitializeStateManager(_standAloneStateManager);

Expand Down
15 changes: 6 additions & 9 deletions src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#nullable disable

using System.Collections;
using System.Diagnostics.CodeAnalysis;
using Microsoft.EntityFrameworkCore.Cosmos.Internal;

namespace Microsoft.EntityFrameworkCore.Cosmos.Query.Internal;
Expand Down Expand Up @@ -354,8 +353,7 @@ protected override Expression VisitExtension(Expression extensionExpression)

if (result.NodeType == ExpressionType.Convert
&& result.Type == typeof(ValueBuffer)
&& result is UnaryExpression outerUnary
&& outerUnary.Operand.NodeType == ExpressionType.Convert
&& result is UnaryExpression { Operand.NodeType: ExpressionType.Convert } outerUnary
&& outerUnary.Operand.Type == typeof(object))
{
result = ((UnaryExpression)outerUnary.Operand).Operand;
Expand Down Expand Up @@ -751,7 +749,7 @@ protected override Expression VisitTypeBinary(TypeBinaryExpression typeBinaryExp

private Expression TryBindMember(Expression source, MemberIdentity member)
{
if (!(source is EntityReferenceExpression entityReferenceExpression))
if (source is not EntityReferenceExpression entityReferenceExpression)
{
return null;
}
Expand Down Expand Up @@ -814,7 +812,7 @@ private bool TryRewriteContainsEntity(Expression source, Expression item, out Ex
{
result = null;

if (!(item is EntityReferenceExpression itemEntityReference))
if (item is not EntityReferenceExpression itemEntityReference)
{
return false;
}
Expand Down Expand Up @@ -1026,7 +1024,7 @@ private static List<TProperty> ParameterListValueExtractor<TEntity, TProperty>(
}

private static bool IsNullSqlConstantExpression(Expression expression)
=> expression is SqlConstantExpression sqlConstant && sqlConstant.Value == null;
=> expression is SqlConstantExpression { Value: null };

private static bool TryEvaluateToConstant(Expression expression, out SqlConstantExpression sqlConstantExpression)
{
Expand Down Expand Up @@ -1061,7 +1059,7 @@ private static bool CanEvaluate(Expression expression)
private static bool TranslationFailed(Expression original, Expression translation, out SqlExpression castTranslation)
{
if (original != null
&& !(translation is SqlExpression))
&& translation is not SqlExpression)
{
castTranslation = null;
return true;
Expand Down Expand Up @@ -1106,8 +1104,7 @@ private sealed class SqlTypeMappingVerifyingExpressionVisitor : ExpressionVisito
{
protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is SqlExpression sqlExpression
&& sqlExpression.TypeMapping == null)
if (extensionExpression is SqlExpression { TypeMapping: null } sqlExpression)
{
throw new InvalidOperationException(CosmosStrings.NullTypeMappingInSqlTree(sqlExpression.Print()));
}
Expand Down
9 changes: 3 additions & 6 deletions src/EFCore.Cosmos/Query/Internal/CosmosStringMethodTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,8 @@ public CosmosStringMethodTranslator(ISqlExpressionFactory sqlExpressionFactory)

if (SubstringMethodInfoWithTwoArgs.Equals(method))
{
return arguments[0] is SqlConstantExpression constant
&& constant.Value is int intValue
&& intValue == 0
? TranslateSystemFunction("LEFT", method.ReturnType, instance, arguments[1])
return arguments[0] is SqlConstantExpression { Value: 0 }
? TranslateSystemFunction("LEFT", method.ReturnType, instance, arguments[1])
: TranslateSystemFunction("SUBSTRING", method.ReturnType, instance, arguments[0], arguments[1]);
}
}
Expand Down Expand Up @@ -237,8 +235,7 @@ public CosmosStringMethodTranslator(ISqlExpressionFactory sqlExpressionFactory)
{
var comparisonTypeArgument = arguments[^1];

if (comparisonTypeArgument is SqlConstantExpression constantComparisonTypeArgument
&& constantComparisonTypeArgument.Value is StringComparison comparisonTypeArgumentValue
if (comparisonTypeArgument is SqlConstantExpression { Value: StringComparison comparisonTypeArgumentValue }
&& (comparisonTypeArgumentValue == StringComparison.OrdinalIgnoreCase
|| comparisonTypeArgumentValue == StringComparison.Ordinal))
{
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Cosmos/Query/Internal/ExpressionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ public static class ExpressionExtensions
{
for (var i = 0; i < expressions.Length; i++)
{
if (expressions[i] is SqlExpression sql
&& sql.TypeMapping != null)
if (expressions[i] is SqlExpression { TypeMapping: not null } sql)
{
return sql.TypeMapping;
}
Expand Down
4 changes: 1 addition & 3 deletions src/EFCore.Cosmos/Query/Internal/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,7 @@ public virtual void ClearOrdering()
/// </summary>
public virtual void ApplyPredicate(SqlExpression expression)
{
if (expression is SqlConstantExpression sqlConstant
&& sqlConstant.Value is bool boolValue
&& boolValue)
if (expression is SqlConstantExpression { Value: true })
{
return;
}
Expand Down
4 changes: 1 addition & 3 deletions src/EFCore.Cosmos/Query/Internal/SqlConstantExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,7 @@ private void Print(
object? value,
ExpressionPrinter expressionPrinter)
{
if (value is IEnumerable enumerable
&& !(value is string)
&& !(value is byte[]))
if (value is IEnumerable enumerable and not (string or byte[]))
{
var first = true;
foreach (var item in enumerable)
Expand Down
1 change: 0 additions & 1 deletion src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Diagnostics.CodeAnalysis;
using Microsoft.EntityFrameworkCore.Cosmos.Internal;
using Microsoft.EntityFrameworkCore.Cosmos.Storage.Internal;
using Microsoft.EntityFrameworkCore.Internal;

namespace Microsoft.EntityFrameworkCore.Cosmos.Query.Internal;
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Cosmos/Storage/Internal/CosmosClientWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,8 +484,7 @@ private static void ProcessResponse(ResponseMessage response, IUpdateEntry entry
}

var jObjectProperty = entry.EntityType.FindProperty(StoreKeyConvention.JObjectPropertyName);
if (jObjectProperty != null
&& jObjectProperty.ValueGenerated == ValueGenerated.OnAddOrUpdate
if (jObjectProperty is { ValueGenerated: ValueGenerated.OnAddOrUpdate }
&& response.Content != null)
{
using var responseStream = response.Content;
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Cosmos/Storage/Internal/CosmosTypeMappingSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ public CosmosTypeMappingSource(TypeMappingSourceDependencies dependencies)
clrType, CreateArrayComparer(elementMapping, elementType), jsonValueReaderWriter: jsonValueReaderWriter);
}

if (clrType.IsGenericType
&& !clrType.IsGenericTypeDefinition)
if (clrType is { IsGenericType: true, IsGenericTypeDefinition: false })
{
var genericTypeDefinition = clrType.GetGenericTypeDefinition();
if (genericTypeDefinition == typeof(List<>)
Expand Down
5 changes: 2 additions & 3 deletions src/EFCore.Design/Design/Internal/CSharpHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ public virtual string UnknownLiteral(object? value)
}

var valueType = value.GetType();
if (valueType.IsGenericType && !valueType.IsGenericTypeDefinition)
if (valueType is { IsGenericType: true, IsGenericTypeDefinition: false })
{
var genericArguments = valueType.GetGenericArguments();
switch (value)
Expand Down Expand Up @@ -1519,8 +1519,7 @@ private static bool IsIdentifierPartCharacter(char ch)
if (ch < 'a')
{
return (ch < 'A'
? ch >= '0'
&& ch <= '9'
? ch is >= '0' and <= '9'
: ch <= 'Z')
|| ch == '_';
}
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Design/Design/Internal/DbContextOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public virtual void Optimize(string? outputDir, string? modelNamespace, string?
_reporter.WriteInformation(DesignStrings.CompiledModelGenerated($"options.UseModel({fullName}.Instance)"));

var cacheKeyFactory = context.GetService<IModelCacheKeyFactory>();
if (!(cacheKeyFactory is ModelCacheKeyFactory))
if (cacheKeyFactory is not ModelCacheKeyFactory)
{
_reporter.WriteWarning(DesignStrings.CompiledModelCustomCacheKeyFactory(cacheKeyFactory.GetType().ShortDisplayName()));
}
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Design/Extensions/ScaffoldingModelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ public static bool IsSimpleManyToManyJoinEntityType(this IEntityType entityType)
var primaryKey = entityType.FindPrimaryKey();
var properties = entityType.GetProperties().ToList();
var foreignKeys = entityType.GetForeignKeys().ToList();
if (primaryKey != null
&& primaryKey.Properties.Count > 1
if (primaryKey is { Properties.Count: > 1 }
&& foreignKeys.Count == 2
&& primaryKey.Properties.Count == properties.Count
&& foreignKeys[0].Properties.Count + foreignKeys[1].Properties.Count == properties.Count
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected virtual void GenerateEntityTypes(
}

foreach (var entityType in nonOwnedTypes.Where(
e => e.GetDeclaredNavigations().Any(n => !n.IsOnDependent && !n.ForeignKey.IsOwnership)))
e => e.GetDeclaredNavigations().Any(n => n is { IsOnDependent: false, ForeignKey.IsOwnership: false })))
Copy link
Contributor

Choose a reason for hiding this comment

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

Now see, this is why I hate most of these. It was perfectly terse and clear before.

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 do find the pattern here better - my brain automatically understands that we're checking two things on n, rather than two unrelated conditions (with n being repeated).

But I agree that this is an edge case, do you really hate most of the changes in this PR? IMHO it cuts down on so much crap and makes conditions so much easier to grok... If people hate it we can revert.

Copy link
Contributor

@bricelam bricelam Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It’s half and half for me. I’m fine doing the occasional bulk cleanup like this. I just put a lot of thought into how I express conditions that it’s sad to see them occasionally mangled in the name of simplification or new language features.

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 probably should have asked/cleared this with the team before doing it - my bad.

We should maybe briefly discuss on Friday - we can also decide to roll at least some of it back, and/or not have this corrected/suggested automatically, but rather decide on a one-by-one basis - though looking at this PR, my personal take is that 95% of these make total sense and do improve code quality.

Copy link
Member

Choose a reason for hiding this comment

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

This adds a few additional null checks if n and ForeignKey are reference types, could be a reason to not use pattern matching in some cases.

{
stringBuilder.AppendLine();

Expand Down
Loading