dotnet / docs

This repository contains .NET Documentation.
https://learn.microsoft.com/dotnet
Creative Commons Attribution 4.0 International
4.17k stars 5.84k forks source link

False alarm for generic types - CA2225 #22042

Open Mikejo5000 opened 3 years ago

Mikejo5000 commented 3 years ago

Moved issue to .NET Old issue: https://github.com/MicrosoftDocs/visualstudio-docs/issues/3851, opened by @koszeggy

The "Do not suppress a warning from this rule" part is way too rigorous this way. Consider the following example:

public static implicit operator Range<T>(T upperBound) => new Range<T>(upperBound);

public static implicit operator Range<T>((T lowerBound, T upperBound) bounds) =>
    new Range<T>(bounds.lowerBound, bounds.upperBound);

As Range<T> is generic the "best" solution to satisfy the rule would be two ToRange extension methods on any T and on ValueTuple<T, T>, respectively. But that would introduce a ToRange on every object, which is insane.

Not mentioning that the main purpose of the operator is to spare typing new Range<T>(low, high) and use simply (low, high) or high in place of any Range<T> assignment, which of course would be much bulkier in a (low, high).ToRange() form.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

gewarren commented 3 years ago

@koszeggy I'm a bit confused. According to this, you cannot overload the range operator. Are you overloading the range operator when you get the CA2225 warning?

koszeggy commented 3 years ago

@gewarren Sorry for the confusion. The example is not about the C# 8 range operator feature and its similarly named non-generic Range struct but a custom type, which happens to be called Range<T>.

And here are the mentioned operators defined.

And then consider the following example:

public class SomeClass
{
    public Range<double> ValidRange { get; set; }
}

Now with the operators the property can be set using the instance.ValidRange = 10.5 or instance.ValidRange = (1.5, 6.25) syntax but the operators trigger the CA2225 warning. My concern is that though the rule could be satisfied by a generic extension method for literally any object and two-element tuples it would be a supobtimal solution.

gewarren commented 3 years ago

I see, these are user-defined conversion operators. @mavasani Do you think the text should be updated to:

In general, do not suppress a warning from this rule if you're implementing a shared library. However, for user-defined conversion operators on generic types, it's okay to suppress the warning instead of implementing the corresponding To<YourType>(this T) named-conversion method.

no-response[bot] commented 3 years ago

This issue has been automatically closed due to no response from the original author. Please feel free to reopen it if you have more information that can help us investigate the issue further.

gewarren commented 3 years ago

Reopening because the actual OP did respond.

tdykstra commented 3 years ago

Ping @mavasani