Compilation issue with source generator

Describe the issue

Source generator version doesn't compile when a base class is used

The error is:

1>MSBUILD : error : Fody: The type 'FodySourceGenerator.ShouldHaveSourceGeneratedINPC' already implements INotifyPropertyChanged so [AddINotifyPropertyChangedInterfaceAttribute] is redundant.

Works fine if the base class also has [AddINotifyPropertyChangedInterface], but the previous version didn't require it.

Minimal Repro

using FodySourceGenerator;
using PropertyChanged;

var zz = new ShouldHaveSourceGeneratedINPC();

namespace FodySourceGenerator
{

    [AddINotifyPropertyChangedInterface]
    public partial class ShouldHaveSourceGeneratedINPC : SomeBaseClass
    {
    }

    public class SomeBaseClass
    {
    }
}

Well, oops. 😨

That's quite an oversight. We haven't considered the interaction between the source generator and the weaver.

I'll suggest a fix for this, but there may be other issues like this one.

Fix submitted, but...

I'm puzzled by the fact that the following code doesn't raise an error:

[AddINotifyPropertyChangedInterface]
public class Foo : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
}

The "already implements INotifyPropertyChanged" error is raised only if Foo has a base class, like in the code in the OP. Is this the intended behavior @SimonCropp? I guess changing it now would break lots of users' code...

This is due to the following logic:

PropertyChanged/PropertyChanged.Fody/TypeNodeBuilder.cs

Lines 33 to 38 in 902f31d

    
           PopulateINotifyNodes(Nodes); 
        
           foreach (var notifyNode in NotifyNodes) 
        
           { 
        
               Nodes.Remove(notifyNode); 
        
           } 
        
           PopulateInjectedINotifyNodes(Nodes);

IMO this is a weird edge case. I'm not sure if we should really put so much effort to support this. If you need such strange constructs you can disable the source generator or better avoid using the attribute at all.

Or maybe we should just stop supporting the AddINotifyPropertyChangedInterface for the source generator?

No, you should not introduce such a massive breaking change IMO.

We could also remove the error on a redundant [AddINotifyPropertyChangedInterface]. I don't think this check is really necessary, though I may be missing an edge case where it could be important. Currently it seems a bit broken to me (see my previous comment).

Ignoring the error if it's caused by generated code isn't really a big effort though, and seems safe.

In eiter case, I'd add the [GeneratedCode]/[DebuggerNonUserCode] to the source generator either way, since it makes the generated code consistent with what the weaver does.

No, you should not introduce such a massive breaking change IMO.

Only for the source generator, not the weaver of course

So, what do we do with this?

Sorry, been on vacation, will check this the next days

Ok, no worries. I've released the other fix in the meantime.

I use 4.1.0 and stumbled on this issue while converting old code.

The issue was, not code generation occurs on the child class I made partial and inherited INotifyPropertyChanged. And there was no warning nor error.

I finally undestood, thanks to this issue, that the parent class was decorated with AddInotifyPropertyChanged. When I converted it too, it worked.

	PopulateINotifyNodes(Nodes);
	foreach (var notifyNode in NotifyNodes)
	{
	Nodes.Remove(notifyNode);
	}
	PopulateInjectedINotifyNodes(Nodes);