Fix IDE0004 for OverloadResolutionPriority

[xtqqczze]

Follow-up to #109602

• Fix IDE0004: Remove unnecessary cast

[stephentoub]

Thank you, but I do not want to take this change. Even if the compiler ends up preferring to call the ROS overload, it's very non-obvious at the call site. I'd rather leave the casts in place as an obvious indication that a different method is being called rather than it being recursive.

[xtqqczze]

@stephentoub Is this behaviour of the IDE0004 analyser desirable in general?

[stephentoub]

@stephentoub Is this behaviour of the IDE0004 analyser desirable in general?

In general, unnecessary casts are unnecessary; they don't cost anything, but they can clutter up the code, and so in general removing them is fine. But in some cases, like this one, the visual indicator of having them is actually a benefit.

[xtqqczze]

@stephentoub Is this behaviour of the IDE0004 analyser desirable in general?

In general, unnecessary casts are unnecessary; they don't cost anything, but they can clutter up the code, and so in general removing them is fine. But in some cases, like this one, the visual indicator of having them is actually a benefit.

I mean, the IDE0004 diagnostic is only flagging when OverloadResolutionPriorityAttribute is present.

[stephentoub]

I mean, the IDE0004 diagnostic is only flagging when OverloadResolutionPriorityAttribute is present.

I don't know what you mean.

[xtqqczze]

I don't know what you mean.

[OverloadResolutionPriority(-1)]
public static unsafe bool Contains<T>(this Span<T> span, T value) where T : IEquatable<T>? =>
    Contains((ReadOnlySpan<T>)span, value); // IDE0004

//[OverloadResolutionPriority(-1)]
public static unsafe bool Contains<T>(this Span<T> span, T value) where T : IEquatable<T>? =>
    Contains((ReadOnlySpan<T>)span, value); // No IDE0004

[stephentoub]

By adding the attribute, you're changing what overload is bound. And which overload is bound impacts whether the cast is necessary or not.

[xtqqczze]

By adding the attribute, you're changing what overload is bound. And which overload is bound impacts whether the cast is necessary or not.

I understand that adding the attribute changes which overload is bound, and that this affects whether the cast is necessary. My question is whether you believe it is desirable for this diagnostic to be flagged in cases where the cast, while unnecessary, serves to clarify non-obvious behavior (as in this PR).

[stephentoub]

My question is whether you believe it is desirable for this diagnostic to be flagged in cases where the cast, while unnecessary, serves to clarify non-obvious behavior (as in this PR).

I think it's in the eye of the beholder whether it's non-obvious. I think it makes sense for the rule to flag it, as it's correct that it's technically not required. It's then up to the developer to decide what they prefer.

[xtqqczze]

I think it's in the eye of the beholder whether it's non-obvious.

In that case, I'll reopen this PR for other reviewers.

[stephentoub]

I think it's in the eye of the beholder whether it's non-obvious.

In that case, I'll reopen this PR for other reviewers.

As a maintainer of this repo, I do not want this to merge. But thank you.