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

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

Open
jnm2 opened this issue Oct 24, 2021 · 6 comments
Open

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

jnm2 opened this issue Oct 24, 2021 · 6 comments

Comments

@jnm2
Copy link
Contributor

jnm2 commented Oct 24, 2021

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
Copy link
Contributor Author

jnm2 commented Oct 24, 2021

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
Copy link
Owner

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

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 24, 2021

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

@YairHalberstadt
Copy link
Owner

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
Copy link
Contributor Author

jnm2 commented Oct 25, 2021

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
Copy link
Owner

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.

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

No branches or pull requests

2 participants