AG interface not correctly referencing another AG interface

[OhRickCode]

Firstly, thank you for all the hard work you put in to this library. It is much appreciated.

So...this is how to reproduce the issue:

Create a new C#8 class library.

Put this class in the root:

using AutomaticInterface;
using Test.Tests;
namespace Test;

[GenerateAutomaticInterface]
public class Test1 : ITest1
{
    public void Get(ITest2 test)
    {
    }
}

and this class in a Tests folder.

using AutomaticInterface;
namespace Test.Tests;

[GenerateAutomaticInterface]
public class Test2: ITest2
{
}

This will not build and shows the following errors:

The type or namespace name 'ITest2' could not be found (are you missing a using directive or an assembly reference?)

'Test1' does not implement interface member 'ITest1.Get(ITest2)'

Luckily, I found a workaround. If you change Test1's contructor parameter declaration from (ITest2 test) to (Tests.ITest2 test) then it will compile.

If there's any further information I can provide, just ask. :thumbsup:

[simonmckenzie]

That's an interesting one @OhRickCode!

It stems from the fact that the interfaces are built independently, and their full names aren't known until after they've been built.

The core independence can be seen here, where the interfaces are built in a loop:

https://github.com/codecentric/net_automatic_interface/blob/becdf1252bc520d6877ecfee13a8c7461629316c/AutomaticInterface/AutomaticInterface/AutomaticInterfaceGenerator.cs#L42-L50

What we could possibly do is build a dictionary of fully qualified generated interface type names before starting the loop shown above, and pass it into Builder.BuildInterfaceFor, then, when building the method definitions, for any parameter that has an underlying type that's an IErrorTypeSymbol (as unresolved types always are), we could take the type name from the dictionary.

What do you think @ChristianSauer?

[OhRickCode]

It stems from the fact that the interfaces are built independently, and their full names aren't known until after they've been built.

I had suspected it was related to the order they are created - which is what led me to the workaround.

[OhRickCode]

I have only written one IIncrementalGenerator, so my knowledge is fairly basic, but could you not simply copy the Usings from the concrete class to the interface? (It is what I did for my generator.) But I don't really know enough to know if that would be a bad idea.

[simonmckenzie]

I have only written one IIncrementalGenerator, so my knowledge is fairly basic, but could you not simply copy the Usings from the concrete class to the interface? (It is what I did for my generator.) But I don't really know enough to know if that would be a bad idea.

That's how the generator used to work, but we hit some namespace collisions and other edge-cases, so it was removed in preference of fully qualified types: #60

[simonmckenzie]

I'm planning to have a look at this next week.

[simonmckenzie]

I spent a bit of time looking at this today, and it's harder than I expected - the generator uses ISymbol.ToDisplayString on the ISymbol instances to produce the string representation of parameters, type parameters, and return types.

I was originally thinking I'd be able to run a SymbolVisitor over the symbols before passing them to ToDisplayString, replacing the unrecognised types with fully qualified ones. Unfortunately you can't just create a new symbol without adding it to the compilation, so that idea is out. I could only see that working if we added two-pass generation so the symbols were genuinely available, but I don't feel it'd justify the performance impact.

Trying to replicate the output of ToDisplayString is also hard, as it provides features out-of-the-box that we don't want to have to reimplement, e.g. parameter definitions with default values, ref params etc. I feel this is probably still the way to go, and will look into it some more when I have some time.

[OhRickCode]

@simonmckenzie : Thank you for looking at this issue. It sounds like a bit of a tricky fix. There is a fairly simple workaround to this problem, so maybe this should be put at the back of the back log?

[ChristianSauer]

Yes, I think this is one of these edge-cases. I honestly do not want that this library goes the way of Automapper - having a bazillion complicated features which are not needed for the core value proposal. If an interface is getting complicated, please go for a manual implementation - it might be easier in the long run.

@simonmckenzie Thank you for the superb comments and investigation. I wish I could invite you as a maintainer, but corp politics makes that super difficult. I am swamped this year. I know I say that a lot, but life is...complicated the last years.