-
Notifications
You must be signed in to change notification settings - Fork 523
Query: Fixes LINQ.Contains to handle Memory Span implicit conversion introduced in C#13/DotNet9 #5477
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
base: master
Are you sure you want to change the base?
Conversation
| if (type == typeof(Enumerable) || type == typeof(Queryable) || type == typeof(CosmosLinq)) | ||
|
|
||
| // Aside from the known types, we also need to avoid partial eval for op_implicit methods, which are the implicit conversions of enum to memoryextension span types (introduced in c#13) | ||
| if (type == typeof(Enumerable) || type == typeof(Queryable) || type == typeof(CosmosLinq) || methodCallExpression.Method.Name == "op_Implicit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the check tighter so that we only let through those on_implicit calls that we support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a check that's broader than what we support, please add negative coverage for those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also guard consts (for downstream success).
| && methodCallExpression.Method.Name == "op_Implicit" | ||
| && methodCallExpression.Method.DeclaringType is { IsGenericType: true } implicitCastDeclaringType | ||
| && implicitCastDeclaringType.GetGenericTypeDefinition() is var genericTypeDefinition | ||
| && (genericTypeDefinition == typeof(Span<>) || genericTypeDefinition == typeof(ReadOnlySpan<>))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug Assert inside if assuming this check is done at top level.
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| [TestClass] | ||
| public class Program |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename, turn this into a test project (if not already).
| @@ -0,0 +1,24 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the project, class namespaces to something suitable that indicate these are .NET C#14 specific tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft.Azure.Cosmos.Tests.C14
| } | ||
|
|
||
| ArrayContainsVisitor visitor = new ArrayContainsVisitor(); | ||
| return visitor.VisitIN(searchExpression, (ConstantExpression)searchList, context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| await container.CreateItemAsync(todoItem); | ||
| Console.WriteLine($"Created item: {todoItem.id}"); | ||
|
|
||
| string[] someStringArray = ["Learn Cosmos DB"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <EmulatorFlavor>master</EmulatorFlavor> | ||
| <DisableCopyEmulator>True</DisableCopyEmulator> | ||
| <LangVersion>$(LangVersion)</LangVersion> | ||
| <LangVersion>preview</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <Platform>AnyCPU</Platform> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <TargetFramework>net9.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
| List<LinqTestInput> inputs = new List<LinqTestInput> | ||
| { | ||
| { | ||
| //// Memory Span Conversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
| Type type = methodCallExpression.Method.DeclaringType; | ||
| if (type == typeof(Enumerable) || type == typeof(Queryable) || type == typeof(CosmosLinq)) | ||
|
|
||
| // Aside from the known types, we also need to avoid partial eval for op_implicit methods, which are the implicit conversions of enum to memoryextension span types (introduced in c#13) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Template
Description
Please include a summary of the change and which issue is fixed. Include samples if adding new API, and include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber