Skip to content

Fixed ILVerify incorrectly flagging valid default interface implementations #61185

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 5 commits into from
Jan 3, 2022
Merged

Fixed ILVerify incorrectly flagging valid default interface implementations #61185

merged 5 commits into from
Jan 3, 2022

Conversation

bbartels
Copy link
Contributor

@bbartels bbartels commented Nov 3, 2021

This PR fixes #46236

I moved tests regarding default implementation to a new file for visibility. If this is an undesired change, let me know and I'll revert it.

The following test still fails, even though the default interface method that is re-abstracted in IReabstractDefaultImplementation is implemented within the class. I will open another issue for this scenario.

.class public auto ansi beforefieldinit ChildClassThatInheritsFromInterfaceThatReabstractedDefaultInterfaceMethodImplementsContract_ValidType_Valid
	extends [System.Runtime]System.Object
	implements IReabstractDefaultImplementation, IExtendedDefaultImplInterface, IInterface
{
	.method public final hidebysig newslot virtual 
		instance void Method () cil managed 
	{
		.maxstack 8

		IL_0001: ret
	}

	.method public hidebysig specialname rtspecialname 
		instance void .ctor () cil managed 
	{
		.maxstack 8

		IL_0000: ldarg.0
		IL_0001: call instance void [System.Runtime]System.Object::.ctor()
		IL_0006: nop
		IL_0007: ret
	}
}

/cc @EgorBo
/cc @jkotas

@ghost ghost added area-Tools-ILVerification Issues related to ilverify tool and IL verification in general community-contribution Indicates that the PR has been added by a community member labels Nov 3, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR fixes #46236

I moved tests regarding default implementation to a new file for visibility. If this is an undesired change, let me know and I'll revert it.

The following test still fails, even though the default interface method that is re-abstracted in IReabstractDefaultImplementation is implemented within the class. I will open another issue for this scenario.

.class public auto ansi beforefieldinit ChildClassThatInheritsFromInterfaceThatReabstractedDefaultInterfaceMethodImplementsContract_ValidType_Valid
	extends [System.Runtime]System.Object
	implements IReabstractDefaultImplementation, IExtendedDefaultImplInterface, IInterface
{
	.method public final hidebysig newslot virtual 
		instance void Method () cil managed 
	{
		.maxstack 8

		IL_0001: ret
	}

	.method public hidebysig specialname rtspecialname 
		instance void .ctor () cil managed 
	{
		.maxstack 8

		IL_0000: ldarg.0
		IL_0001: call instance void [System.Runtime]System.Object::.ctor()
		IL_0006: nop
		IL_0007: ret
	}
}

/cc @EgorBo
/cc @jkotas

Author: bbartels
Assignees: -
Labels:

area-ILVerification
Milestone: -

@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Nov 3, 2021
MichalStrehovsky
MichalStrehovsky reviewed Nov 3, 2021
View reviewed changes
src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs Outdated
Comment on lines 299 to 304
// If no base type contains interface method, attempt to find default implementation
if (result == null)
{
_ = thisType.ResolveInterfaceMethodToDefaultImplementationOnType(interfaceMethodToResolve, out result);
}

Copy link
Member

Choose a reason for hiding this comment

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

ILVerify needs to call ResolveInterfaceMethodToDefaultImplementationOnType manually after this resolution fails; it cannot be done in this method. There are nuances around default interface method resolution that will not work as expected if this is done here. The nuances might not affect ILVerify right now, but will affect it eventually.

E.g. if we want to do reporting of diamond cases in ILVerify (when multiple interfaces on the type provide a default implementation for a method), we'll have to call into ResolveInterfaceMethodToDefaultImplementationOnType manually anyway.

Copy link
Member

Choose a reason for hiding this comment

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

(This change breaks other consumers of TypeSystemHelper.cs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate on what you mean with ILVerify needs to call ResolveInterfaceMethodToDefaultImplementationOnType manually after this resolution fails? Why is it a problem that ILVerify needs to call the method after the resolution fails? Does the method affect state in some way, meaning a second call won't succeed anymore?

Copy link
Member

Choose a reason for hiding this comment

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

ResolveInterfaceMethodTarget (which is in a source file shared with other components in this repo and not unique to ILVerify) should not call ResolveInterfaceMethodToDefaultImplementationOnType. Instead ILVerify should call ResolveInterfaceMethodToDefaultImplementationOnType when ResolveInterfaceMethodTarget returns null.

This is how other components do this as well. We don't want to hide failure modes that are unique to default interface methods here (I'm referring to the value returned from ResolveInterfaceMethodToDefaultImplementationOnType that is being swallowed here).

Copy link
Contributor Author

@bbartels bbartels Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right I think I understand correctly now. So would it be sufficient to move the call to ResolveInterfaceMethodToDefaultImplementationOnType into VerifyInterfaces if ResolveInterfaceMethodTarget returns null?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sound good. It will also put ILVerify into a better position to report unique error messages for things like the diamond case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I adjusted the callsite of ResolveInterfaceMethodToDefaultImplementationOnType. Apologies for misunderstanding your original comment, thank you for elaborating :)
I'll create another issue for the bug mentioned in this PR's description.

@runfoapp runfoapp bot mentioned this pull request Nov 4, 2021
Closed
@JulieLeeMSFT
Copy link
Member

@MichalStrehovsky Is there anything else to be done for this PR? Are we good to merge this PR?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky Is there anything else to be done for this PR? Are we good to merge this PR?

The change no longer touches any of the files I own (which was the reason I looked at it in the first place). The change in ILVerify looks good. I didn't look at the tests.

jkotas
jkotas approved these changes Jan 3, 2022
View reviewed changes
@jkotas
Copy link
Member

jkotas commented Jan 3, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas jkotas merged commit f97546b into dotnet:main Jan 3, 2022
@jkotas
Copy link
Member

jkotas commented Jan 3, 2022

@bbartels Thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@jkotas jkotas jkotas approved these changes

Assignees

@EgorBo EgorBo

Labels
area-Tools-ILVerification Issues related to ilverify tool and IL verification in general community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

ILVerify rejects classes which inherit default interface method implementations
5 participants
@bbartels @JulieLeeMSFT @MichalStrehovsky @jkotas @EgorBo