Annotate source with JetBrains Annotations

[smoogipoo]

We will probably want to consider annotating the entire codebase using JetBrains Annotations, at some point after a nuget release.

https://github.com/JetBrains/ExternalAnnotations These are shipped by default with ReSharper/Rider.

[FreezyLemon]

I know this is low priority, but wouldn't it be possible to make these annotations available at an earlier stage?

This would make it possible for contributors to update annotations while they're working on stuff (instead of having to re-read all the code later), and would also make it less work later since new code will be annotated already.

This looks like a really nice feature, but seems like a huge task if it's supposed to be done all at once.

[peppy]

You're welcome to start adding them.

[jorolf]

Just going to mention that dynamic recompilation doesn't work currently when a class uses the annotations error CS0122: 'UsedImplicitlyAttribute' is inaccessible due to its protection level

[peppy]

Do you have a branch that we can see this happening on?

[FreezyLemon]

Something else that should be answered: Let's say we have a constructor (example taken from TrackBass, modified) that takes an instance of a reference type as an argument and checks it for null:

public TrackBass(Stream data)
{
  if (data == null)
    throw new ArgumentNullException(nameof(data));

  dataStream = data as AsyncBufferStream ?? new AsyncBufferStream(data, -1);
  // do more stuff
}

If we add a [NotNull] annotation now (since data should never be null), this happens:

This is a "Warning" level error (and it should be, usually), but here we have a problem: We can't leave it like this because CI will fail (and we can't change the inspection severity because this is a more general inspection error).

We also can't remove the null check here since passing null to this will NOT fail CI (passing null as parameter data here will only show as a hint, not a warning). We also can't even "accept" null here and wait for it to throw a null ref or anything like that, due to the dataStream = ... line.

We should probably (?) change the inspection severity of passing null here to warning.. The problem with that would be that if we do something like:

// no annotation
private Stream getStream() => null;

new TrackBass(getStream());

after removing the null check, is that this will not even trigger the possible null assignment inspection hint, and would in this scenario mess up silently. (What I'm trying to get at here is: If another method is not annotated at all (very possible in the beginning) or annotated incorrectly, this will fail without throwing an exception and give unexpected results).

Not sure how to handle this.

[FreezyLemon]

Even weirder:

This doesn't trigger any inspection warnings, even though sample is [NotNull]..