beakona / AutoInterface

C# interface-to-member source generator
MIT License
73 stars 9 forks source link

Can this be used where an aspect only partially implements the interface that the parent class is implementing? #17

Open waxingsatirical opened 9 months ago

waxingsatirical commented 9 months ago
interface IPrintableComplex
{
   void Print();
   void PrintComplex();
}
public class SimplePrinter
{
   public void Print() { Console.WriteLine("OK"); }
}
public partial class Person : IPrintableComplex
{
   [BeaKona.AutoInterface(typeof(IPrintableComplex))]
   private readonly SimplePrinter aspect1 = new SimplePrinter();

   public void PrintComplex() { Console.WriteLine("Oh, K."); }
}

If I write this at the moment, I just get an error saying that SimplePrinter doesn't implement IPrintableComplex. What would be useful for me is if the generator could just generate the methods that SimplePrinter does have, then I can fill in the rest inside the Person class.

beakona commented 9 months ago

I thought about that feature and decided that it would add too much confusion because AutoInterface is primarily an interface helper tool and SimplePrinter does not implement IPrintableComplex. Match by signatures is just relaxation of strictness but all interface members are still required.

For your use-case I can add additional parameter which explicitly grants that, but currently I do not have much free time to implement it. Here is an example:

[BeaKona.AutoInterface(typeof(IPrintableComplex), AllowMissingMembers=true)]

waxingsatirical commented 8 months ago

Thanks, that does sound like it would suit my use case.

Time, yes, who has it?

I've made an attempt in a fork, I'd appreciate it if you had a look. I've got some concerns, I don't know why in the ProcessClass method 'references' is a collection. Also, looks like there would need to be a parallel change where the code is built from a template?? but I don't really get how that bit works.

https://github.com/beakona/AutoInterface/compare/main...waxingsatirical:AutoInterface:issue_17

beakona commented 8 months ago

Variable references is collection because there could be many fields/properties/indexers that should be called/accessed during forward-call. In example multiple backers there are two fields paired to IPrintable interface.

Expression references.First().ReceiverType.IsMemberImplemented(i) would test first element and later inside block use all references as they all pass that test. What about this approach:

foreach(old expression)
{
   var validReferences = references.Where(i => i.ReceiverType.IsMemberImplemented(method, false));
   if(validReferences.Any())
   {
     //old code.. but use validReferences
     //writer.WriteMethodDefinition(builder, method, scope, @interface, validReferences);
   }
}

Filtering of references should be performed both for template (aka generator) and for default implementation (when generator is null).

I like your approach to IsMemberImplemented but tests for properties and indexers do not take into account explicit/noexplicit flag. Flag explicit/noexplicit could also be booleanbecause opposite of ExplicitInterfaceImplementationis not Ordinary | LambdaMethod.

It seem that in your usecase expression at BeaKona.AutoInterfaceGenerator/AutoInterfaceSourceGenerator.cs, 481 would always return false because method IsAllInterfaceMembersImplementedBySignature() would return false if SimplePrinter (variable receiverType) do not implement IPrintableComplex (variable type).

beakona commented 7 months ago

I've made nuget prerelease package 1.0.30-pre that should fix this issue.

waxingsatirical commented 7 months ago

Hi, thanks for making the change, if I can get this working it will really clean up some of my code. I've had a look at the pre release version. The behaviour is not exactly as I expected, in my example from above, if Person does not implement PrintComplex() then the source generator is putting in an empty method, in the generated partial class. like this: PrintComplex() { }

I hoped that the source generator would not do this, so that the compiler would then raise an error tell me 'Person2' does not implement interface member 'IPrintableComplex.PrintComplex'. Leaving the developer to fill in the gaps that have not been satisfied by the child aspect.

If this is by design can we have another flag to stop it from generating the empty methods?

On another note, I'm also getting this warning in my application when trying to use AutoInterface, I haven't really looked into it though, so probably just something stupid I've done, maybe better not to spend any time on it for now CSC : warning CS8785: Generator 'AutoInterfaceSourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'ArgumentException' with message 'Reported diagnostic has an ID 'BK-AG09', which is not a valid identifier.

beakona commented 7 months ago

Oh, I see your intentions now. I took the liberty to decouple recognition whether referenced method exists, and whether method should be generated at all. Now, we have additional attribute MemberMatch which tells how to search for existing method.

In order to maintain backward compatibility default value is Explicit.

In version v1.0.31-pre you should write:

[AutoInterface(typeof(IPrintableComplex), AllowMissingMembers=true, MemberMatch=MemberMatchTypes.Public)]
private readonly SimplePrinter aspect1 = new SimplePrinter();

I've upgraded project and c# Source Generators had minor chanes. I've fixed most of them, but apparently diagnostic identifiers cannot have dash in the name now. Id BK-AG09 is invalid so I removed all dashes, errors codes are BKAG09 now. Thanks.

waxingsatirical commented 6 months ago

Hi, I've been testing your latest release.

I have a different problem now, where my child aspect has a property, get & set, that implements the target interface property which is get only.

The problem stems from ITypeSymbolExtensions

public static ISymbol? FindImplementationForInterfaceMemberBySignature(this ITypeSymbol @this, ISymbol interfaceMember)
{
    string name = interfaceMember.Name;
    if (string.IsNullOrEmpty(name) == false)
    {
        foreach (ISymbol member in @this.GetMembers(name).Where(i => i.Kind == interfaceMember.Kind))
        {
            var comparer = new ComparerBySignature();
            if (comparer.Equals(member, interfaceMember))
            {
                return member;
            }
        }
    }

    return null;
}

This returns true only if the members match, but should only be interested in whether the interfaceMember is implemented.

beakona commented 6 months ago

Yes, match was very strict and faulty. I've made a fix.

waxingsatirical commented 5 months ago

Thanks for the update, the MemberMatchTypes has resolved that problem. Onto the next one, the generated properties have no access modifiers, despite the target implementation declaring them as public. I have also tried adding public to the interface declaration but this is not respected. Am I missing a setting somewhere?

public interface IPrintable
{
    public void Print();
}

public class SimplePrinter : IPrintable
{
    public void Print() { Console.WriteLine("OK"); }
}

public class MyPrinter : IPrintable
{
    [Beakona.AutoInterface]
    private IPrintable _simplePrinter  = new SimplePrinter()
}
beakona commented 5 months ago

I've created new project with your example, added keyword partial to the MyPrinter and here is the output:

// <auto-generated />
#nullable enable

namespace AutoInterfaceSample.Test
{
    partial class MyPrinter
    {
        void IPrintable.Print() => this._simplePrinter.Print();
    }
}

MyPrinter is generated without access keyword, intentionaly, because class/struct/record is partial and it will follow access you specify at other partial sites.

Print() is explicit interface member implementation, intentionaly. That is the main purpose of AutoInterface.. to redirect all interface members to target instance allowing MyPrinter behaves as IPrintable without interfering with other MyPrinter members.

waxingsatirical commented 5 months ago

Yes, I take your point the behaviour does make sense.

Was just wondering if I can leverage templates somehow in order to make the auto-implemented properties non-explicit?

The reason I'm trying to do so, is that I am trying to expose via a web api and the serialiser is not picking up the generated properties.