123

Why is ReSharper judging me for this code?

    private Control GetCorrespondingInputControl(SupportedType supportedType, object settingValue)
    {
        this.ValidateCorrespondingValueType(supportedType, settingValue);

        switch(supportedType)
        {
            case SupportedType.String:
                return new TextBox { Text = (string)settingValue };
            case SupportedType.DateTime:
                return new MonthPicker { Value = (DateTime)settingValue, ShowUpDown = true };
            default:
                throw new ArgumentOutOfRangeException(string.Format("The supported type value, {0} has no corresponding user control defined.", supportedType));
        }
    }

    private void ValidateCorrespondingValueType(SupportedType supportedType, object settingValue)
    {
        Type type;

        switch(supportedType)
        {
            case SupportedType.String:
                type = typeof(string);
                break;
            case SupportedType.DateTime:
                type = typeof(DateTime);
                break;
            default:
                throw new ArgumentOutOfRangeException(string.Format("The supported type value, {0} has no corresponding Type defined.", supportedType));
        }
        string exceptionMessage = string.Format("The specified setting value is not assignable to the supported type, [{0}].", supportedType);
        if(settingValue.GetType() != type)
        {
            throw new InvalidOperationException(exceptionMessage);
        }
    }

The second method ValidateCorrespondingValueType's "settingValue" parameter is grayed out with the following message by ReSharper: "Parameter 'settingValue' is only used for precondition check(s)."

kame
  • 20,848
  • 33
  • 104
  • 159
Corpsekicker
  • 3,276
  • 6
  • 25
  • 34
  • You can move the declaration and assignment of `exceptionMessage` into the `if`-block :) – AakashM Nov 19 '14 at 11:31
  • You could also do this in the method: expectedText += ""; and it stops complaining since you used it in the method. – PHPGuru Nov 30 '18 at 00:33

7 Answers7

127

It's not judging, it's trying to help :)

If ReSharper sees that a parameter is only used as a check to throw an exception, it greys it out, indicating that you're not actually using it for "real" work. This is most likely a mistake - why pass in a parameter you're not going to use? It usually indicates that you've used it in a pre-condition, but then forgotten (or no longer need) to use it elsewhere in the code.

Since the method is an assertion method (that is, all it does is assert it's valid), you can suppress the message by marking the ValidateCorrespondingValueType as an assertion method, using ReSharper's annotation attributes, specifically the [AssertionMethod] attribute:

[AssertionMethod]
private void ValidateCorrespondingValueType(SupportedType supportedType, object settingValue)
{
  // …
}
Wai Ha Lee
  • 8,598
  • 83
  • 57
  • 92
citizenmatt
  • 18,085
  • 5
  • 55
  • 60
  • 4
    It's a nice check, but in *this* case R# has over-reached a bit, wouldn't you say? The check on `settingValue`'s type can't possible be a **pre**-condition, since the thing it's being checked against isn't known until some work has been done within the body of the method! – AakashM Nov 19 '14 at 11:36
  • 6
    That's why you need to tell ReSharper it's an assertion method. The sole point of this method is to perform the pre-condition check for another method. It's an assert, but ReSharper can't know that unless you tell it with `[AssertionMethod]`. – citizenmatt Nov 19 '14 at 17:42
  • 1
    I don't see `AssertionMethod` on the page you linked to and when I try to add it in code ReSharper isn't helping me out with using statements. What assembly and namespace is that attribute in? – reggaeguitar Dec 10 '14 at 16:35
  • 10
    I ended up just changing the Inspection Severity to "Do Not Show", this is another option. – reggaeguitar Dec 10 '14 at 18:07
  • 1
    Oh yeah, I didn't realise it's not on the page. Funnily enough, it's not in the default implementation that ReSharper 8 provides via the annotations options page, either. I think it was going to be phased out, in favour of `[ContractAnnotation]` which does the same thing, but is more versatile. However, phasing it out breaks old code. Either way, it's in the current version of the official [annotations nuget package](https://www.nuget.org/packages/JetBrains.Annotations/), with xml docs. – citizenmatt Dec 18 '14 at 19:36
  • 71
    It might have been a useful feature, if one could disable the 'pre-condition only' inspection independently from the regular unused parameter inspection; as it stands the inspection mixes up two issues of different severity and generally makes this functionality useless in certain situations. I'm also very sceptical of the idea of adding comments or attributes to the code just to keep a source code analysis tool happy, so at the moment I don't think there's a satisfactory solution to the issue. – Serge Belov Apr 07 '15 at 04:20
  • 9
    It might be trying to help but it's too aggressive. Now, if you verify the value and then never use it, fine, that's likely an error. However, it's yapping at me over one where I only use the value in the error. How else can that be anything but intentional? – Loren Pechtel May 09 '15 at 19:30
  • I began to scoff about a package that I need to include and production-package to aid in development only, but then I learned that the Attribute is conditional-compiled with JETBRAINS_ANNOTATIONS, so it won't make its way into the assembly unless you explicitly declare that symbol. So: Thanks for the tip with the attribute, I explore its possibilities :) – Rolf Sep 01 '15 at 13:15
  • Yep, the package doesn't require a binary dependency, unless you define `JETBRAINS_ANNOTATIONS`. You can also include the source directly in your code, so you still get the annotations, your users get the annotations, and you don't take a binary dependency. See [this blog post](http://blog.jetbrains.com/dotnet/2015/08/12/how-to-use-jetbrains-annotations-to-improve-resharper-inspections/) for more details. – citizenmatt Sep 01 '15 at 14:08
  • 1
    @LorenPechtel, I notice that if you use the new 'nameof' functionality in C#6 to specify the name of the argument passed in when throwing, say, an ArgumentNullException', then ReSharper backs off. – Holf Sep 25 '15 at 14:41
  • @Holf Seems like a bug in ReSharper, since that inspection should still apply in that case. – Nick Smith Dec 08 '15 at 21:45
  • I'm going to accept this an answer for opening insightful dialogue. – Corpsekicker Jun 02 '16 at 06:46
  • 2
    I don't like to add another component to my solution just for the sake of the message – DanielV Apr 03 '18 at 12:16
  • 1
    So, if I have a parameter `bool requiredResult` and it's `true` value tells the method to throw an exception if method is about to return a `null` is it still an `[AssertionMethod]`? I doubt it... or do I have a design problem here? :) Funny thing is, if I have a silly statement above `throw` which builds an exception message, all's good from Resharper. – Gabrielius Oct 20 '18 at 17:52
24

Interestingly, ReSharper backs off if you use the new nameof functionality in C#6:

static void CheckForNullParameters(IExecutor executor, ILogger logger)
{
    if (executor == null)
    {
        throw new ArgumentNullException(nameof(executor));
    }

    if (logger == null)
    {
        throw new ArgumentNullException(nameof(logger));
    }
}
Richard Ev
  • 52,939
  • 59
  • 191
  • 278
Holf
  • 5,605
  • 3
  • 42
  • 63
9

The following fixes the issue (in ReSharper 2016.1.1, VS2015), but I am not sure it solves the 'right' problem. In any case, it shows the ambiguity in ReSharper's mechanics regarding this topic:

This yields the warning:

    private void CheckForNull(object obj)
    {
        if (ReferenceEquals(obj, null))
        {
            throw new Exception();
        }
    }

But this does not:

    private void CheckForNull(object obj)
    {
        if (!ReferenceEquals(obj, null))
        {
            return;
        }
        throw new Exception();
    }

It is interesting that equivalent code (the inversion was done by ReSharper :D) gives different results. It seems that the pattern matching simply does not pick up the second version.

6

My preferred solution to this problem is make resharper think the parameter is used. This has an advantage over using an attribute such as UsedImplicitly because if ever you do stop using that parameter, resharper will start warning you again. If you use an attribute, resharper won't catch future real warnings either.

An easy way to make resharper think the paramter is used is by replacing throw with a method. So instead of...

if(myPreconditionParam == wrong)
    throw new Exception(...);

...you write:

if(myPreconditionParam == wrong)
    new Exception(...).ThrowPreconditionViolation();

This is nicely self-documenting for future programmers, and resharper quits whining.

The implementation of ThrowPreconditionViolation is trivial:

public static class WorkAroundResharperBugs 
{
    //NOT [Pure] so resharper shuts up; the aim of this method is to make resharper 
    //shut up about "Parameter 'Foobaar' is used only for precondition checks" 
    //optionally: [DebuggerHidden]
    public static void ThrowPreconditionViolation(this Exception e)
    {
        throw e;
    }
}

An extension method on Exception is namespace pollution, but it's fairly contained.

Eamon Nerbonne
  • 47,023
  • 20
  • 101
  • 166
  • +1 for mentioning `[UsedImplicitly]`, I didn't want to use `[AssertionMethod]` as it wasn't, and used implicitly sounds more accurate it in my case (I was passing a value to a callback in a constructor and returning the constructed object). – MrLore Apr 04 '17 at 15:13
3

Others have already answered the question, but no one mentioned the following ways of turning off the warning.

Add this above the method signature to turn it off for only that method:

    // ReSharper disable once ParameterOnlyUsedForPreconditionCheck.Local

Add this above the class declaration to turn it off for the entire file:

     // ReSharper disable ParameterOnlyUsedForPreconditionCheck.Local
todji
  • 141
  • 7
  • 1
    A disadvantage is that you can not specify witch parameter you mean. – comecme Apr 22 '20 at 14:38
  • 1
    @comecme You can disable for a single parameter by using [disable and restore](https://www.jetbrains.com/help/resharper/Code_Analysis__Configuring_Warnings.html#suppress) comments around that particular parameter. I'd suggest putting every parameter on its own line in that case; still ugly but less so (in my opinion). – Travis Jun 18 '20 at 22:42
1

I believe below is a legit case where I check all items in list with lambda Any method. Then I use the same way the next line but resharper fails.

if (actions.Any(t => t.Id == null)) // here says Paramter t is used only for precondition check(s)
    throw new Exception();
actionIds = actions.Select(t => t.Id ?? 0).ToList();

I had to ignore with comment.

mkb
  • 1,106
  • 1
  • 18
  • 21
-1

use this comment above parameter

// ReSharper disable once ParameterOnlyUsedForPreconditionCheck.Local

to disable suggestion in Rider

Michael Snytko
  • 327
  • 3
  • 13