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.

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

[Weaver] Fix System.Linq.Queryable not correctly referenced when not imported yet #2639

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

LaPeste
Copy link
Contributor

@LaPeste LaPeste commented Sep 21, 2021

Description

In the unlikely scenario where System.Linq.Queryable is not used at all in the user code, but the user calls a method of an interface that System.Linq.Queryable implements, the weaver fails to set the correct version number of System.Linq.Queryable to import. This resulted in a runtime issue that states that a certain (and wrong) "x.x.x.x" version of System.Linq.Queryable can't be found.
The problem comes from the fact that the version number of each assembly was extracted by the weaver from System.Runtime giving the wrong number version and only getting it right in most cases by luck. Reading the version number from the assembly with the majority of our dependencies (Remotion.Linq) fixed the issue.

Although, to my limited knowledge, no other similar issue has been reported (mostly because of how unlikely this type of usage is), this fix should likely avoid similar problems with other assemblies found in Remotion.Linq.

Fixes #1568

TODO

  • Changelog entry

Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Need to fix the warnings, but otherwise looks good.

@nirinchev nirinchev merged commit 78933da into master Sep 22, 2021
@nirinchev nirinchev deleted the ac/linq-queryable-exception branch September 22, 2021 08:20
@fealebenpae
Copy link
Member

What flavor of .NET does this occur in - Framework, Standard, Core, the new .NET 5, or all of the above? If it's just a specific flavor, like Standard, then maybe it makes sense to hardcode the assembly version?
Alternatively, we can keep using the approach here, but not on the Remotion.Linq assembly, because we don't fully control it. I imagine we could add those references to the Realm assembly and look them up there?

@nirinchev
Copy link
Member

We've reproduced it on UWP and .NET Core 3.1. What's the problem with using Remotion.Linq? Since we take a dependency on it and it takes a dependency on System.Linq.Queryable, I would expect that a version of System.Linq.Queryable at least equal to the version requested by Remotion.Linq should end up in the compiled app.

@fealebenpae
Copy link
Member

The likelihood is very slim, but for us Remotion's dependency on System.Linq.Queryable is purely an implementation detail and might break in a hypothetical point release of theirs. What's more, we're still up in the air whether we will keep using Remotion after the LINQ Provider revamp, so I really think it's the most future-proof approach to just slap the assembly reference on the Realm assembly and resolve it that way in the fallback scenario.

Alternatively we can break down .NETCore and .NETCoreApp from .NETPortable in ImportedReferences and hard-code the assembly version.

@nirinchev
Copy link
Member

Yeah, I'm not overly worried about that - there's no way to implement Remotion.Linq without System.Linq.Queryable.

But I agree - we should revisit this when we drop Remotion.Linq and move assembly resolution to look for Realm assembly dependencies.

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

Successfully merging this pull request may close these issues.

UWP - Relational data LINQ Queryable exception
4 participants