4

I have written some code in a WPF application, which looks like this:

var dialog = new OpenFileDialog { Filter = this.FileExtensionFilter };
var dialogResult = dialog.ShowDialog();
if (dialogResult.HasValue && dialogResult.Value)
{
    ... Process result of dialog ...
}

All well and good, I thought, but ReSharper has come up with a warning on the check for dialogResult.HasValue, "Expression is always true".

The first question is how ReSharper would even know that the dialogResult always has a result - it must have dived right down into the decompiled code of the Microsoft.Win32.OpenFileDialog class and observed that it never returns null. Either that, or it's just a hardcoded warning specifically for this class.

Secondly, it doesn't seem like good practice to just assume that the result will never be null. What if Microsoft release a future version of the library in which the null value does become available? The documentation on the matter says: "In the current implementation, the derived classes (OpenFileDialog and SaveFileDialog) will only return true or false" which does imply this is not a permanent arrangement that we can rely on forever.

Any thoughts on whether I'm being overcautious and should remove the line as ReSharper suggests?

Stephen Holt
  • 2,360
  • 4
  • 26
  • 34

2 Answers2

3

It does seem odd. The MSDN spec states that it will return either true or false, but still there must be a reason for the Nullable.

I would absolutely agree with you, that assuming an implementation underneath is bad practise. I would code as per the interface, so in this case I think checking the HasValue is the correct way to go.

How does Re-sharper know? I'm afraid I can't answer that. It's not something I use, they may have hard-coded it.

This may be of interest to you: When does Microsoft.Win32.OpenFileDialog.ShowDialog() return null?

It seems the reason there is the possbility of null is because that's the result before the user has closed the dialog.

Community
  • 1
  • 1
Ian
  • 33,605
  • 26
  • 118
  • 198
3

It's hard-coded.

If you look in the ReSharper directory in Program Files, you will see lot's of XML files with Nullness.Gen in the names, these contain rules regarding whether or not a particular element is/is not null, and these are used to display warnings such as the one you are seeing, where they wouldn't be displayed normally.

If you find the 2.0.5.0.Nullness.Gen.xml file in Bin\ExternalAnnotations.NETFramework\System.Windows, you will find the following entry about half way down:

<member name="M:System.Windows.Controls.OpenFileDialog.ShowDialog">
    <attribute ctor="M:JetBrains.Annotations.NotNullAttribute.#ctor" />
</member>

You can then look at JetBrains.Annotations in the bin directory to see the definition for NotNullAttribute:

 <member name="T:JetBrains.Annotations.NotNullAttribute">
     <summary>
     Indicates that the value of the marked element could never be <c>null</c>
     </summary>
     <example><code>
     [NotNull] public object Foo() {
       return null; // Warning: Possible 'null' assignment
     }
     </code></example>
 </member>
JMK
  • 27,273
  • 52
  • 163
  • 280
  • Good work answering the other half of the question - I believe this is a mistake from JetBrains and that it is a really bad assumption still that the API won't return null, it's just an implementation detail. – Ian Oct 29 '13 at 14:15
  • Thanks as well for clarifying this JMK. I agree with @Ian, it does seem like bad programming practice to make hardcoded assumptions like this. Public interfaces should generally be regarded as black boxes, and if it has a nullable return type you should cater for that. – Stephen Holt Oct 29 '13 at 16:28
  • It's a shame I can't mark both answers as accepted, since they each answer a different part of the multiple question! Upvoting both anyway. – Stephen Holt Oct 29 '13 at 16:30
  • 1
    @Ian's answer is more useful so I'm not complaining :) – JMK Oct 29 '13 at 16:32