Skip to content
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.

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

Invalid Codegen for Value Types (Breaks .NET 7) #901

Closed
Sewer56 opened this issue Oct 12, 2022 · 2 comments · Fixed by #903
Closed

Invalid Codegen for Value Types (Breaks .NET 7) #901

Sewer56 opened this issue Oct 12, 2022 · 2 comments · Fixed by #903
Assignees
Labels
Milestone

Comments

@Sewer56
Copy link

Sewer56 commented Oct 12, 2022

Describe the issue

Runtime changes in .NET 7 make the weaved code for the common use case (define PropertyChanged only) non-executable/invalid.

Minimal Repro

var dum = new Dummy();
dum.Test = "Text"; // Invalid program

public struct Dummy : INotifyPropertyChanged
{
    public string Test { get; set; }

    public event PropertyChangedEventHandler? PropertyChanged;
}

Build with net7.0 TFM to observe the issue.

Stack Trace:

Unhandled exception. System.InvalidProgramException: Common Language Runtime detected an invalid program.
   at Dummy.<>OnPropertyChanged(PropertyChangedEventArgs eventArgs)
   at Dummy.set_Test(String value) in C:\Users\sewer\Desktop\Projects\DummyProjects\Fody.PropertyChanged.Net7\Fody.PropertyChanged.Net7\Program.cs:line 11
   at Program.<Main>$(String[] args) in C:\Users\sewer\Desktop\Projects\DummyProjects\Fody.PropertyChanged.Net7\Fody.PropertyChanged.Net7\Program.cs:line 7

Problematic CodeGen:

.method private hidebysig 
	instance void '<>OnPropertyChanged' (
		class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs eventArgs
	) cil managed 
{
	.custom instance void [System.Runtime]System.CodeDom.Compiler.GeneratedCodeAttribute::.ctor(string, string) = (
		01 00 14 50 72 6f 70 65 72 74 79 43 68 61 6e 67
		65 64 2e 46 6f 64 79 07 34 2e 30 2e 34 2e 30 00 00
	)
	.custom instance void [System.Runtime]System.Diagnostics.DebuggerNonUserCodeAttribute::.ctor() = (
		01 00 00 00
	)
	// Header Size: 12 bytes
	// Code Size: 21 (0x15) bytes
	// LocalVarSig Token: 0x11000003 RID: 3
	.maxstack 3
	.locals init (
		[0] class [System.ObjectModel]System.ComponentModel.PropertyChangedEventHandler
	)

	/* 0x00000378 02           */ IL_0000: ldarg.0
	/* 0x00000379 7B04000004   */ IL_0001: ldfld     class [System.ObjectModel]System.ComponentModel.PropertyChangedEventHandler Dummy::PropertyChanged
	/* 0x0000037E 0A           */ IL_0006: stloc.0
	/* 0x0000037F 06           */ IL_0007: ldloc.0
	/* 0x00000380 2C0A         */ IL_0008: brfalse.s IL_0014

	/* 0x00000382 06           */ IL_000A: ldloc.0
	/* 0x00000383 02           */ IL_000B: ldarg.0
	/* 0x00000384 03           */ IL_000C: ldarg.1
	/* 0x00000385 FE14         */ IL_000D: tail.
	/* 0x00000387 6F0B00000A   */ IL_000F: callvirt  instance void [System.ObjectModel]System.ComponentModel.PropertyChangedEventHandler::Invoke(object, class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs)

	/* 0x0000038C 2A           */ IL_0014: ret
} // end of method Dummy::'<>OnPropertyChanged'

My guess based on the codegen is that Fody isn't boxing the value type, instead trying to pass it by reference; and earlier runtimes were able to tolerate that to some capacity; but the new one isn't.

I.e. There is nothing that resembles:

ldarg.0
ldobj     // Copy Dummy by Ptr
box       // Box the Dummy

Temporary Workaround

Manually add OnPropertyChanged to struct types that implement INotifyPropertyChanged such that .

private void OnPropertyChanged([CallerMemberName] string? propertyName = null) => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));

Fody.PropertyChanged will then delegate the calls to your OnPropertyChanged and this should avoid the issue.

About

This is probably a relatively niche bug; usually consumers wouldn't use INotifyPropertyChanged with value types.

I'm not asking/expecting a fix (or any action etc. to be taking) I'm just reporting this to raise awareness and hopefully help someone out there who may run across this.

@Sewer56 Sewer56 changed the title Invalid Codegen for Value Types which Breaks .NET 7 Invalid Codegen for Value Types (Breaks .NET 7) Oct 12, 2022
@ltrzesniewski
Copy link
Member

ltrzesniewski commented Oct 13, 2022

Thanks for the report. 🙂

This should definitely be fixed. I guess one possible "fix" would be to remove support for value types, but I think adding the box instruction would be a better solution as it would keep backwards compatibility.

Out of curiosity, why do you use INPC with value types?

@ltrzesniewski ltrzesniewski self-assigned this Oct 13, 2022
@Sewer56
Copy link
Author

Sewer56 commented Oct 13, 2022

Out of curiosity, why do you use INPC with value types?

Normally doing this wouldn't make much sense as boxing would be costly.

In my specific case, I had a collection of small objects (which itself grouped reference types, <4 registers in size) that would be more efficient to access in a sequential manner; so naturally I used a struct.

I used INPC because I was working with WPF and one of these objects had a caption which could be edited by the user. Normally this caption would be automatically filled so in most cases, it would not be edited; therefore sticking with value types logically made sense.

Random example(s) here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants