Tag Mock.Object as NotNull (JetBrains.Annotations)

[JeromeJ]

From static analysis I made myself, I don't think Mock.Object can ever be null but, since I set up my ReSharper as pessimistic, it raises it as a false positive of "Possible NullReferenceException" since it hasn't explicitly tagged as [NotNull].

Doing so would allow users to not have to check "to be sure" for things that it's actually not required.

(Actually it's the same with many methods, such as Setup, which always return a non null object but aren't tagged as such)

[stakx]

Hi @JeromeJ, I think it would be sensible to not apply these attributes in single locations—ideally, we'd enable #nullable for the whole project and annotate all method signatures with nullability info. Your suggestion would become part of that effort.

I've previously looked at starting work on nullable reference type annotations, what stopped me is that we currently have a ton of Debug.Assert(frobbler != null) and these interfere with C# 8's nullable reference type feature. I haven't quite dared removing those kinds of asserts because they have actually helped catch a ton of bugs in the past; I'm still not certain whether C# 8's nullable reference types feature will provide a "safety net" of equal or better value (but I sure hope so!).

[JeromeJ]

Oyeah I agree it would probably be better to not focus it on a single point.

Yeah... That's why I was mentioning JetBrains.Annotations for all of us who aren't ready to switch over to C# 8 before a (sad) long while but I also have high hopes for it but I do enjoy the safety that it can already provide now (mostly by using pessimistic config to emulate c#8's feature as close as possible).

[JeromeJ]

Using ReSharper there is a shortcut to add NotNull and CanBeNull using "!" and "?".If retyping "!" on a parameter marked as not null it auto adds `if(foo == null) throw new ArgumentNotNull(nameof(foo));` which is sorta equivalent to your Debug.Assert afaik but easy to add.

[stakx]

In my personal opinion, Debug.Assert(foo != null) and if (foo == null) throw new ArgumentNullException(nameof(foo)); serve different purposes. The first states an assumption which you think ought to be true (but you haven't actually proven it), while the latter validates external input (and so should be done in methods that make up the public API). If tooling unilaterally decides on one of these two types of check, you'll end up with too many of them and too few of the other kind.

It's pretty clear to me that when converting to C# nullable reference types, all Debug.Assert(foo != null) would simply vanish. Call me old-fashioned, I just don't like the idea to destroy the value they've had in the past without being really sure that the new thing will be equally good. (That said, I'm eventually going to do it if noone else beats me to it. Got to go with the times.)

Not a particularly difficult task, but one that takes patience and a good bit of time, as one has to go over the whole library.

[stakx]

@JeromeJ, I've been taking a brief look... it appears like one could prepare external annotations as an XML document and submit it to JetBrains/ExternalAnnotations. That would seem like a good solution: you get the code analysis result that you need, without us bringing in another dependency / tool.

[JeromeJ]

Indeed! This could be a nice solution! (even though those XML are painful to write..) I'll look into it!

Dependency-wise, if I'm not mistaken, while you could also use the nugget (and thus creating a dependency), you, alternatively, could get away with just using a good ol' c/c'ed .cs file (KISS) containing all the annotations as it works on a namespace base.
That's what I do internally but I'm not 100% positive on how it works regarding embarked external dll.

[stakx]

That's what I do internally but I'm not 100% positive on how it works regarding embarked external dll.

As far as I understand it, including the source and conditionally removing it during compilation (using [Conditional("JETBRAINS_ANNOTATIONS")] or similar) will work fine as long as you're working with actual source code. Since we're talking about a library that gets distributed in binary form, the annotation types would have to be preserved / embedded in the Moq DLL... in the best case, Rider/ReSharper would still recognize them even if they were declared internal.

While that may work in theory I think it would be neater to just ship the annotations out of band (as XML)... not everyone will need them but those using JetBrains tools would, I suppose, automatically get a copy.

[stakx]

I am going to close this, as I still believe external annotations as mentioned here are the way to go, since that means we don't make the Moq NuGet package dependent on JetBrains.Annotations.