Skip to content

Conversation

@tmat
Copy link
Member

@tmat tmat commented Nov 17, 2025

Block additions to MethodImpl table when targeting .NET Framework.
The EnC impl in .NET Framework doesn't handle them correctly.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2631743 and https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1452066/

Public API change: #81305

@tmat tmat requested review from a team as code owners November 17, 2025 22:32
@dotnet-policy-service dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Nov 17, 2025
@dotnet-policy-service
Copy link
Contributor

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

@tmat
Copy link
Member Author

tmat commented Nov 17, 2025

@DustinCampbell @dotnet/roslyn-compiler ptal

@333fred
Copy link
Member

333fred commented Nov 18, 2025

Build failures look real.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM, assuming CI is passing.

or ErrorCode.ERR_SingleInapplicableUnaryOperator
or ErrorCode.ERR_AmbigOperator
or ErrorCode.ERR_UnexpectedArgumentListInBaseTypeWithoutParameterList
or ErrorCode.ERR_EncUpdateRequiresEmittingExplicitInterfaceImplementationNotSupportedByTheRuntime
Copy link
Contributor

Choose a reason for hiding this comment

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

ERR_EncUpdateRequiresEmittingExplicitInterfaceImplementationNotSupportedByTheRuntime

Isn't this a build only diagnostic?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's EnC-only diagnostic. Not sure if that should be considered build only or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 21, 2025

Choose a reason for hiding this comment

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

Not sure if that should be considered build only or not.

I think all emit time diagnostics are build-only diagnostics. Getting a diagnostic for a syntax tree won't return them

ERRID.ERR_CannotApplyOverloadResolutionPriorityToOverride,
ERRID.ERR_CannotApplyOverloadResolutionPriorityToMember,
ERRID.ERR_NextAvailable,
ERRID.ERR_EncUpdateRequiresEmittingExplicitInterfaceImplementationNotSupportedByTheRuntime,
Copy link
Contributor

Choose a reason for hiding this comment

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

ERR_EncUpdateRequiresEmittingExplicitInterfaceImplementationNotSupportedByTheRuntime

Similar question

/// The runtime supports adding to InterfaceImpl table.
/// The runtime supports adding to MethodImpl table.
/// </summary>
AddExplicitInterfaceImplementation = 1 << 10,
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 20, 2025

Choose a reason for hiding this comment

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

AddExplicitInterfaceImplementation

Consider renaming as well #Closed

Copy link
Member Author

@tmat tmat Nov 20, 2025

Choose a reason for hiding this comment

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

The name of the capability is semi-public. Internally it's represented by the internal enum. Other components use string names, which get translated to the enum value. We match the enum value name to the string.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

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

Labels

Area-Compilers Needs API Review Needs to be reviewed by the API review council VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants