8000 Update .NET adapter to handle interface static members properly by daxian-dbw · Pull Request #15908 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Update .NET adapter to handle interface static members properly #15908

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
Merged
merged 3 commits into from
Aug 14, 2021
Merged

Update .NET adapter to handle interface static members properly #15908

merged 3 commits into from
Aug 14, 2021

Conversation

daxian-dbw
Copy link
Member
@daxian-dbw daxian-dbw commented Aug 11, 2021 •
edited
Loading

PR Summary

After switching to the latest dotnet 6 preview.7 SDK, we started to see the "Bad IL format" failure as shown below (see the reported dotnet bug here).
This is because a new preview feature is enabled in preview.7, which allows an interface to have abstract static methods (see the blog post). The type System.Int16 implements much more interfaces in preview.7, and many of them declare abstract static properties. However, the abstract static properties cannot be invoked with GetValue(null) reflection API, and doing so results in this Bad IL format error.

image

The recommendation from .NET team is to ignore static virtual/abstract members on an interface, especially as the set of APIs may change with .NET 7.

This PR includes the following changes:

  1. Update .NET adapter to ignore static virtual methods/properties on interface
  2. Given DisablePrivateReflectionAttribute is already obsolete and there is no restriction on private reflection in .NET Core, remove DisallowPrivateReflection and uses of it from our code.
  3. The way how PopulateMethodReflectionTable handles interface was to call type.GetInterfaceMap(interfaceType) and use the interface methods from the returned mapping object. The static non-virtual methods on an interface will be missed in this way, so, this is changed to call interfaceType.GetMethods(bindingFlags) directly.
  4. A minor change to the parameter type of the MethodCacheEntry constructor, to avoid unneeded array creation.

PR Checklist

iSazonov
iSazonov reviewed Aug 12, 2021
View reviewed changes
Copy link
Collaborator
@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

Interesting, could we benefit from a source generator to pre-build the cache at compile time or this will force to load extra assemblies at startup?

@daxian-dbw daxian-dbw marked this pull request as draft August 12, 2021 05:18
@daxian-dbw daxian-dbw marked this pull request as ready for review August 12, 2021 16:31
PaulHigin
PaulHigin approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor
@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan
Copy link
Member

@daxian-dbw Please respond to Rob's comments.

@cston cston mentioned this pull request Aug 12, 2021
Closed
@daxian-dbw
Copy link
Member Author

All comments are addressed. Please take another look, thanks!

@tannergooding
Copy link

Its worth calling out that, aside from static abstract being a new IL concept, short.MinValue is an existing public constant. However, whatever logic is doing the resolution is finding and preferring the explicitly implemented interface member from IMinMaxValue<TSelf>.MinValue { get; }.

I would have expected that such members were at least not preferred over matching public members (even once powershell can properly support static abstracts in interfaces).

@tannergooding
Copy link

A basic breaking-change issue documenting this has been created here: https://github.com/dotnet/runtime/issues/57323

@adityapatwardhan adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Aug 14, 2021
@adityapatwardhan adityapatwardhan merged commit 286f12a into PowerShell:master Aug 14, 2021
@adityapatwardhan adityapatwardhan added this to the 7.2.0-preview.9 milestone Aug 14, 2021
@daxian-dbw daxian-dbw deleted the adapter branch August 14, 2021 20:59
xtqqczze pushed a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Aug 20, 2021
@daxian-dbw @xtqqczze
Update .NET adapter to handle interface static members properly (Powe…
e06f943 
…rShell#15908)
@ghost
Copy link
ghost commented Aug 23, 2021

🎉v7.2.0-preview.9 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link
ghost commented Sep 28, 2021

🎉v7.2.0-preview.10 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov iSazonov left review comments

@rjmholt rjmholt rjmholt approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@PaulHigin PaulHigin PaulHigin approved these changes

Assignees

@adityapatwardhan adityapatwardhan

529B
Labels
CL-Engine Indicates that a PR should be marked as an engine change in the Change Log
Projects
None yet
Milestone
7.2.0-preview.9
Development

Successfully merging this pull request may close these issues.
6 participants
@daxian-dbw @adityapatwardhan @tannergooding @rjmholt @PaulHigin @iSazonov
0