Skip to content

Update dependencies and transform Span-based overloads to Enumerable in funcletizer #35339

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

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

roji
Copy link
Member

@roji roji commented Dec 17, 2024

This PR is a copy of #35300, but with code to transform the problematic Span-based overloads to their Enumerable-based counterparts in the funcletizer (review that separate commit only). This is an alternative approach to #35279.

/cc @ChrisJollyAU

dotnet-maestro bot and others added 3 commits December 9, 2024 13:03
…206.6

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Helix.Sdk
 From Version 10.0.0-beta.24578.2 -> To Version 10.0.0-beta.24606.6
…213.2

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Helix.Sdk
 From Version 10.0.0-beta.24578.2 -> To Version 10.0.0-beta.24613.2
@roji roji requested review from a team, AndriySvyryd and maumar as code owners December 17, 2024 09:54
@roji roji changed the title Ref struct funcletizer Update depenendencies and transform Span-based overloads to Enumerable in funcletizer Dec 17, 2024
@roji roji changed the title Update depenendencies and transform Span-based overloads to Enumerable in funcletizer Update dependencies and transform Span-based overloads to Enumerable in funcletizer Dec 17, 2024
@ChrisJollyAU
Copy link
Contributor

@roji Not sure what's failing in the CI. Everything is passing locally.

I had one question from earlier today but it appears it doesn't seem to be a factor. The questions was since both Enumerable and Queryable had the same sort of functions (Contains/SequenceEqual etc) whether there was needed to rewrite some to Enumerable and others to Queryable (especially with Contains a big part goes through the Queryable visitor RelationalQueryableMethodTranslatingExpressionVisitor). Turns out this doesn't seem to be a factor

@roji
Copy link
Member Author

roji commented Dec 17, 2024

The questions was since both Enumerable and Queryable had the same sort of functions (Contains/SequenceEqual etc) whether there was needed to rewrite some to Enumerable and others to Queryable (especially with Contains a big part goes through the Queryable visitor RelationalQueryableMethodTranslatingExpressionVisitor).

AFAICT, the new Span-based overloads are only resolved when the parameters in question is e.g. an array (something that's implicitly convertible to Span). For the Queryable Contains/SequenceEquals, these generally accept IQueryable as their parameters, and I can't think of a case where something could be both an IQueryable and a Span (arrays are both IEnumerable and Span, but arrays are not IQueryable). In general, you either compose queryable operators over an EF DbSet (which is a class, definitely not convertible to Span) or over another IQueryable... Makes sense?

The other errors have to do with the build system and the switch to net10.0 - nothing that seems related to the problems above. We'll figure that part out...

@AndriySvyryd AndriySvyryd requested a review from Copilot December 17, 2024 21:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (8)
  • eng/Version.Details.xml: Language not supported
  • eng/Versions.props: Language not supported
  • eng/common/cross/build-rootfs.sh: Language not supported
  • eng/common/cross/toolchain.cmake: Language not supported
  • global.json: Language not supported
  • src/EFCore.Tools/EFCore.Tools.nuspec: Language not supported
  • test/Directory.Build.props: Language not supported
  • src/EFCore/Metadata/Internal/MemberClassifier.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

eng/common/core-templates/steps/source-build.yml:81

  • The word 'PortabelBuild' is misspelled. It should be 'PortableBuild'.
portableBuildArgs='/p:PortabelBuild=${{ parameters.platform.portableBuild }}'

@AndriySvyryd
Copy link
Member

/azd run all

@roji
Copy link
Member Author

roji commented Dec 18, 2024

Thanks for looking at this @AndriySvyryd... There's still some Helix failure apparently, if you plan to look into it, feel free to merge when green...

@ChrisJollyAU
Copy link
Contributor

AFAICT, the new Span-based overloads are only resolved when the parameters in question is e.g. an array (something that's implicitly convertible to Span). For the Queryable Contains/SequenceEquals, these generally accept IQueryable as their parameters, and I can't think of a case where something could be both an IQueryable and a Span (arrays are both IEnumerable and Span, but arrays are not IQueryable). In general, you either compose queryable operators over an EF DbSet (which is a class, definitely not convertible to Span) or over another IQueryable... Makes sense?

But you do normalize some Enumerable to a Queryable in QueryableMethodNormalizingExpressionVisitor

if (method.DeclaringType == typeof(Enumerable))
{
    visitedExpression = TryConvertEnumerableToQueryable(methodCallExpression);
}

This was obviously not being done on my version so that was why I had to basically copy the Contains and Primitive Collection stuff to the non queryable translator and catch the Contains there. It's also why in the original issue (#35100) it was going the basic route and doing a WHERE ... IN and inlining the values rather than parameterizing and doing the OPENJSON. Could probably do the same normalizing for the Span on that PR but not really needed.

Otherwise for the sake of completeness this just needs to handle all the MemoryExtensions functions

@roji
Copy link
Member Author

roji commented Dec 18, 2024

@ChrisJollyAU to be sure I understand, are you saying that there's still a problem here that this PR doesn't handle with its approach (or were you more discussing the other approach etc.)? If so, could you provide more details on those scenarios?

Otherwise for the sake of completeness this just needs to handle all the MemoryExtensions functions

Possibly, but I think it's OK to first concentrate on methods which "take over" standard enumerable methods which are already supported by EF (i.e. Contains/SequenceEquals). AFAICT most/all of the other methods are operations which don't have a corresponding Enumerable operator, so we can't use the approach in this PR to transform them to something else. Also, AFAICT most/all of these aren't currently supported by any EF provider - we can always add them later, possibly using your approach (i.e. mark them as non-evaluatable), though the same problems would apply for these... We could also have internal methods just to have a non-Span-based representation for these, which we could transform to in the funceltizer and recognize in translators. But I'd rather cross that bridge when we get to it, and unblock things here for now.

@AndriySvyryd
Copy link
Member

Thanks for looking at this @AndriySvyryd... There's still some Helix failure apparently, if you plan to look into it, feel free to merge when green...

It's failing at

     [Test Class Cleanup Failure (Microsoft.EntityFrameworkCore.Update.MismatchedKeyTypesSqlServerTest)] System.NullReferenceException
      System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
        D:\a\_work\1\s\test\EFCore.SqlServer.FunctionalTests\Update\MismatchedKeyTypesSqlServerTest.cs(800,0): at Microsoft.EntityFrameworkCore.Update.MismatchedKeyTypesSqlServerTest.MismatchedKeyTypesSqlServerFixture.DisposeAsync()

But I don't see how is that fixture any different from the others.

@AndriySvyryd AndriySvyryd merged commit 49b3b84 into dotnet:main Dec 18, 2024
7 checks passed
@roji roji deleted the RefStructFuncletizer branch December 18, 2024 21:49
@roji
Copy link
Member Author

roji commented Dec 18, 2024

Thanks for the help here @AndriySvyryd

@ChrisJollyAU
Copy link
Contributor

to be sure I understand, are you saying that there's still a problem here that this PR doesn't handle with its approach (or were you more discussing the other approach etc.)? If so, could you provide more details on those scenarios?

Was discussing the other approach. As with the arrays it didn't end up normalizing to a queryable. But that isn't a problem on this approach

Possibly, but I think it's OK to first concentrate on methods which "take over" standard enumerable methods which are already supported by EF (i.e. Contains/SequenceEquals). AFAICT most/all of the other methods are operations which don't have a corresponding Enumerable operator, so we can't use the approach in this PR to transform them to something else. Also, AFAICT most/all of these aren't currently supported by any EF provider - we can always add them later, possibly using your approach (i.e. mark them as non-evaluatable), though the same problems would apply for these... We could also have internal methods just to have a non-Span-based representation for these, which we could transform to in the funceltizer and recognize in translators. But I'd rather cross that bridge when we get to it, and unblock things here for now.

Yeah thats fine. For now it is just Contains and SequenceEqual that are needed. Just have to be aware of any new methods that get added to MemoryExtensions or anything else that gets upgraded to a ref struct/Span

@roji
Copy link
Member Author

roji commented Dec 19, 2024

Yeah, absolutely. This whole thing is unfortunate but for now I don't think it'll impact us too much beyond this.

@oyzar
Copy link

oyzar commented Feb 19, 2025

Is this included in 9.0.2? We are getting errors if the type of the collection is an array, but it works for lists. For example:
Guid[] ids = [Guid.NewGuid()];
queryable.Where(item => ids.Contains(item.Id)).ToList();

@ChrisJollyAU
Copy link
Contributor

@oyzar This is only a 10.0 thing as it depends on a c# compiler feature that is only part of c# 14 (still in preview).

What is your exact error message?
Also check in your project file if you have the LangVersion property set

@oyzar
Copy link

oyzar commented Feb 19, 2025

This will only fail if it is preview I believe. But it can still fail on .net 9. Does this mean it will not be released until the 10.0 package of EF is released?

@ChrisJollyAU
Copy link
Contributor

@oyzar As long as you are using c# 13 or lower with .Net 9 it should be fine. Trying to use a higher c# version is likely to cause issues.

It is workable around as long as you explicitly cast or cause it to use an Enumerable function instead of the Span

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants