GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Update .NET adapter to handle interface static members properly by daxian-dbw · Pull Request #15908 · PowerShell/PowerShell

Conversation

@daxian-dbw

@daxian-dbw Dongbo Wang (daxian-dbw) commented Aug 11, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@daxian-dbw
Update .NET adapter to handle interface static members properly
d179604
@ghost ghost assigned Aditya Patwardhan (adityapatwardhan) Aug 11, 2021
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from Paul Higinbotham (PaulHigin), Aditya Patwardhan (adityapatwardhan) and Rob Holt (rjmholt) August 11, 2021 23:27
iSazonov
Ilya (iSazonov) reviewed Aug 12, 2021
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

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.

All reactions
Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw
Dongbo Wang (daxian-dbw) marked this pull request as draft August 12, 2021 05:18
@daxian-dbw
Fix a method overloads resolution issue
3abb6a7
@daxian-dbw
Dongbo Wang (daxian-dbw) marked this pull request as ready for review August 12, 2021 16:31
rjmholt
Rob Holt (rjmholt) reviewed Aug 12, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/System.Management.Automation/engine/CoreAdapter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Aug 12, 2021
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 12, 2021

Copy link
Copy Markdown
Member

Dongbo Wang (@daxian-dbw) Please respond to Rob's comments.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw
Address Rob's comments
77f6580
@cston Charles Stoner (cston) mentioned this pull request Aug 12, 2021
Bad IL Format error on creating a dynamic method using System.Linq.Expressions.Compiler.LambdaCompiler.Compile dotnet/runtime#57106
Closed
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Aug 12, 2021

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt
Rob Holt (rjmholt) approved these changes Aug 12, 2021
View reviewed changes
@tannergooding

Tanner Gooding (tannergooding) commented Aug 12, 2021

Copy link
Copy Markdown

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tannergooding

Tanner Gooding (tannergooding) commented Aug 12, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Aug 14, 2021
View reviewed changes
@adityapatwardhan Aditya Patwardhan (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
Aditya Patwardhan (adityapatwardhan) merged commit 286f12a into PowerShell:master Aug 14, 2021
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) added this to the 7.2.0-preview.9 milestone Aug 14, 2021
@daxian-dbw
Dongbo Wang (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

Deleted user (ghost) commented Aug 23, 2021

Copy link
Copy Markdown

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

Handy links:

  • Release Notes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

Deleted user (ghost) commented Sep 28, 2021

Copy link
Copy Markdown

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

Handy links:

  • Release Notes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@daxian-dbw
Update .NET adapter to handle interface static members properly (Powe…
e78bb83
…rShell#15908)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
+2 more reviewers
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@daxian-dbw @adityapatwardhan @tannergooding @rjmholt @PaulHigin @iSazonov
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.