Prevent Code Analysis to warn about "unused parameters" for operator weaving instruction

[BrunoJuchli]

Description

Version 3 brought a breaking change which requires operators to be defined in source code like:

public static bool operator ==(OnlyOperator left, OnlyOperator right) => Operator.Weave();
public static bool operator !=(OnlyOperator left, OnlyOperator right) => Operator.Weave();

This causes code analysis to warn about the equality operator's parameters (left, right) being unused (CA1801)

The solution

Replace Operator.Weave<T>() with Operator.Weave<T>(T left, T right).
Due to this being a breaking change, the major version is increased (V4).

Alternate Solutions

Support multiple Operator.Weave overloads

Instead of replacing Operator.Weave(), we could have introduced an additional Operator.Weave(left, right) for those who care about CA1801.

However, this would increase the required test and maintenance effort.

Migration: as long as the user has used invarying names for the parameters, just like left and right, he can perform a solution wide string replacement of Operator.Weave() by Operator.Weave(left, right).

throw exception

Adapt Operator.Weave to return an NotSupportedException / Exception. Adapt operators to throw:
public static bool operator ==(T left, T right) => throw Operator.Weave(left, right);
public static bool operator !=(T left, T right) => throw Operator.Weave(left, right);

This, however, also results in code analysis warnings (CA1605: op_Equality creates an exception of type *****. Exceptions should not be raised in this type of method. If this exception instance might be raised, change this method's logic so it no longer raises an exception.)

So not an improvement.

Todos

• Related issues
• Tests
• Documentation

[BrunoJuchli]

@SimonCropp
Could I get your opinion on:

Is it better to have a breaking change from:

public static bool operator ==(T left, T right) => Operator.Weave();

to

public static bool operator ==(T left, T right) => Operator.Weave(left, right);

OR

Support both, which increases test coverage and makes maintaining this harder?

I'd lean towards the breaking change. After all the breaking change can easily be resolved by performing a simple "replace all in solution".

[SimonCropp]

@BrunoJuchli yep go for the breaking change :)

[BrunoJuchli]

@SimonCropp
I've increased the version from 3.1 to 4.0 because of the breaking change. And I've documented the breaking change @ Readme.md.

I've also done a quick local test with the appveyor-built nuget package, and it worked as expected.

I hope it's good to merge & I'd recommend a squash commit.

[BrunoJuchli]

@SimonCropp
Could you please release this? i.E. merge to master (squash commit?) & release package?

[SimonCropp]

sorry for the delay

This is now deployed. NuGet may take some time to make it available for download.

[ndrwrbgs]

@BrunoJuchli were these warnings coming out during build? I'm curious if it's possible to direct StaticAnalysis to run AFTER Fody, and what the behavior of that would be.

I'm curious because I'm about to contribute to another project that replaces code with a pattern that involves throwing an exception to make sure things fail if the code replacement isn't done (to make it clearer to readers that the code they see isn't the code that will be run), and I don't want to just make the same problem you hit here worse for people with FxCop enabled.

[ndrwrbgs]

As an aside, @BrunoJuchli it looks like an alternative might have been just renaming the arguments to == and != to _1 and _2, which are ignored for the purposes of the rule. This could have helped avoid needing a breaking change perhaps?

[BrunoJuchli]

@ndrwrbgs
FxCop runs after Fody, but Roslyn based analyzers run before Fody. Running them after Fody would mean you'd have to create c# from IL and then run them on that c# code. No one's going to do that.

Also, I've experimented with discard parameter names and I couldn't get rid of the warnings.
Do you have a source for

(...) to _1 and _2, which are ignored for the purposes of the rule

?
It would have been great to avoid a breaking change.

[ndrwrbgs]

The CA1801 link you provide in the original post says it :)