postsharp / Metalama

Metalama is a Roslyn-based meta-programming framework. Use this repo to report bugs or ask questions.
164 stars 4 forks source link

Usage of AgumentNullException.ThrowIfNull #269

Closed jon-armen closed 4 months ago

jon-armen commented 4 months ago

When creating aspects, if we do not check builder for null, the compiler warns warning CA1062: In externally visible method 'void CustomValidationAttribute.BuildAspect(IAspectBuilder<IProperty> builder)', validate parameter 'builder' is non-null before using it. If appropriate, throw an 'ArgumentNullException' when the argument is 'null'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1062)

If we validate with

  if (builder == null)
  {
      throw new ArgumentNullException(nameof(builder));
  }

we get the warning warning CA1510: Use 'ArgumentNullException.ThrowIfNull' instead of explicitly throwing a new exception instance (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1510)

When we use ArgumentNullException.ThrowIfNull(builder);, Metalama throws an error while compiling error LAMA0222: The compile-time project could not be compiled. In most cases, this is due to a problem in your code and can be diagnosed using the other reported errors. If, however, you believe this is due to a bug in Metalama, please report the issue and include diagnostic information available

Ideally, we would not have to add any of these checks to satisfy code analyzers, however the ArgumentNullException.ThrowIfNull() concept would be easiest to follow if necessary, however it causes a crash. Our current solution is to manually suppress these warnings.

gfraiteur commented 4 months ago

You cannot use ArgumentNullException.ThrowIfNull because this is not a .NET Standard 2.0 API -- I guess. The error message is odd but I think this is the root cause.

jon-armen commented 4 months ago

That makes sense why we can't use ThrowIfNull. It's easy enough to suppress the warnings as a work around with our analyzer rules. I would consider this a nice to have enhancement, but doesn't create and roadblocks in our usage.