GeezerVison is showing github.com — Update .NET adapter to handle interface static members properly by daxian-dbw · Pull Request #15908 · PowerShell/PowerShell
Conversation
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.

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:
- Update .NET adapter to ignore static virtual methods/properties on interface
- 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.
- 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.
- A minor change to the parameter type of the
MethodCacheEntry constructor, to avoid unneeded array creation.
PR Checklist
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
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?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
All comments are addressed. Please take another look, thanks!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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).
|
daxian-dbw reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
xtqqczze
pushed a commit
to xtqqczze/PowerShell-PowerShell
that referenced
this pull request
Aug 20, 2021
|
🎉v7.2.0-preview.9 has been released which incorporates this pull request.:tada:
Handy links:
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
🎉v7.2.0-preview.10 has been released which incorporates this pull request.:tada:
Handy links:
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
+2 more reviewers
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Labels
CL-Engine
Indicates that a PR should be marked as an engine change in the Change Log
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|