YairHalberstadt / stronginject

compile time dependency injection for .NET
MIT License
845 stars 24 forks source link

SI1103 warning location forces suppression to be all or nothing #163

Open jnm2 opened 2 years ago

jnm2 commented 2 years ago

These warnings are all reported on the identifier token of the container class. When it is necessary to suppress one of them, placing #pragma warning disable SI1103 in the container class file causes all future such warnings to be suppressed.

⚠ SI1103 Warning while resolving dependencies for 'Foo': Return type 'Bar' of delegate 'System.Func' has a single instance scope and so will always have the same value.

A solution could be to report the warning on the syntax Func<Bar> which triggered the warning. Willing to implement.

Could this rationale apply to other warnings?

jnm2 commented 2 years ago

If that feels like the wrong place, maybe it would be better to place it on the attribute which registered the type declaring the parameter.

YairHalberstadt commented 2 years ago

Neither of those feel like the right place as both could be in a completely separate assembly.

jnm2 commented 2 years ago

What about leaving the current behavior as a fallback in the case where it's not all in the same assembly?

YairHalberstadt commented 2 years ago

Even then, philosophically it feels the wrong place.

Consider the following two modules:

public record A(Func<B> b);
public record B();

[Register(typeof(A))]
public class Module1{}

[Register(typeof(B), Scope.SingleInstance)]
public class Module2{}

On it's own both of those modules are absolutely fine. It's only when we put the two together in a container that we have a problem.

jnm2 commented 2 years ago

I'd see Func<B> as a logical place in that scenario, even though it would only appear when something in the same project puts the two modules together.

YairHalberstadt commented 2 years ago

I think Func<B> is definitely wrong - I don't want to be putting diagnostics on perfectly valid user code, because somewhere else decided to register it with StrongInject. Registration code should change to reflect business code, not the other way round.

[Register(typeof(A))] is an option, but it's kind of weird. Firstly there's not always a clear place to put it (e.g. different assemblies, multiple locations which register the same type, etc.). Secondly the same registration on a module might be used by multiple containers, only one of which leads to an issue, so there's nothing wrong with the registration. Thirdly it doesn't even fully solve your problem. The same type might have multiple delegate arguments, and you would have to suppress them all together.

Also the complexity of this change is very high. We have to associate every InstanceSource with a unique location and put the diagnostic there. I don't really want to do this for something which I'm just not really sure is the correct solution semantically.

I understand the problem that you're trying to solve. I just don't really have a good solution.