FP: SA1205 for partial, file-scoped classes

[CaringDev]

SA1205 fires for partial file-scoped classes:

file partial class SA1205
{
}

[bjornhellander]

Interesting case 😄 Do you have an actual use case for file partial, or was this more of an accidental discovery?

[CaringDev]

:-D I do
Trying to switch a logging statement of a private inner class to a source-generated method:

public class Outer
{
  // ... some use of Inner ...
  private class Inner
  {
    // ...
      logger.LogWarning(...)
    // ...
  }
}

then

public class Outer
{
  // ... some use of Inner ...
  private class Inner // if partial: requires Outer to be partial
  {
    // ...
    [LoggerMessage(...)]
    private static partial void Log(...)  // requires Inner to be partial
  }
}

so, I thought, let's use the new file-scope:

public class Outer
{
  // ... some use of Inner ...
}

file sealed partial class Inner // even if partial, does NOT require Outer to be partial
{
  // ...
  [LoggerMessage(...)]
  private static partial void Log(...)  // requires Inner to be partial
}

Addendum, see #3906 (comment) ff.
However, this does not even compile... so is rather artificial.

[julealgon]

Just out of curiosity @CaringDev , why do you want the generated log methods to be in an inner class (or on a friend class in your second example using file), instead of just having them in the parent class directly?

Is it purely an organizational thing on your side, or is there more to it?

Just to be sure... you realize you don't need the autogenerated log methods to be static, correct?

[CaringDev]

@julealgon
If the logging methods were in the parent Outer class, they would need to be accessible from Inner... obviously the real code is a bit more complex :-)
The method needs to be static though: https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib1009

[julealgon]

@CaringDev

@julealgon If the logging methods were in the parent Outer class, they would need to be accessible from Inner... obviously the real code is a bit more complex :-)

Fair enough.

The method needs to be static though: https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib1009

That warning is incorrect:

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

var hostBuilder = Host.CreateApplicationBuilder();
hostBuilder.Services.AddTransient<Service>();

var host = hostBuilder.Build();
var service = host.Services.GetRequiredService<Service>();

service.DoIt();

public partial class Service(ILogger<Service> logger)
{
    public void DoIt()
    {
        LogSomething();
    }

    [LoggerMessage("Something here.", Level = LogLevel.Information)]
    public partial void LogSomething();
}

Try this. No warnings and the log is emited properly.

info: Service[1256132127]
      Something here.

I would assume this warning is only used in specific circumstances, maybe when the parent class is itself static.

[CaringDev]

@julealgon interesting... the generated logger callback is static in both cases. The analyzer might be about performance... to bad the rationale is not documented 🤷
In any case, the logger method being static or not is not relevant for SA1205.

[bjornhellander]

I have tried a few examples I found online using the LoggerMessage attribute, but I can't get them to compile with the file keyword. And that is pretty much what I expected: Source generators generate new files. The file keyword makes the type accessible only from within the same file. That sounds like a bad combination. I don't see how you can possibly implement methods in a file class using a source generator. But I might very well be missing something.

Can you provide a small but complete class that compiles? Just trying to understand how relevant this combination of keywords is.

[CaringDev]

@bjornhellander of course, you're absolutely right. I didn't even try to compile, once I got the SA-squigglies🤦
So this is only valid for partial classes within a single file, which is most likely not a real-world issue 😅

[bjornhellander]

Ok, good. The fix is trivial and I have a draft ready. Since you have found it I personally think that we might as well fix it. But the pull requests are piling up right now, so I don't want to add another one at this moment for something that we right now don't have a reasonable use case for. I suggest we keep the issue open though, to possibly be handled later on.