Logging Source Generator fails with partial methods with implicit ILogger definition when using partial classes

[MithrilMan]

Description

I found a bug that causes the error SYSLIB1019 when it shouldn't be triggered.

If you declare the ILogger field in a file containing part of your partial class (let's call the class Foo)and you define the log methods that uses source generation logging in another physical file that's of course a partial class of Foo, the compiler complain with the error

SYSLIB1019 Couldn't find a field of type Microsoft.Extensions.Logging.ILogger in class Foo

If I move the ILogger declaration from first physical file to the other containing log methods, it works.

Reproduction Steps

Create a project that makes use of logging, create a Foo class like this, in two distinct files (probably even if you declare them in a single file should trigger the problem too)

public partial class Foo
{
   ILogger _logger;

   public void Test()
   {
      LogPeriodicWorkFailure("workName");
   }
}

public partial class Foo
{
   [LoggerMessage(0, LogLevel.Critical, "An unhandled exception has been raised in the {PeriodicWork} work.")]
   partial void LogPeriodicWorkFailure(string periodicWork);
}

If you move the declaration ILogger _logger; to the second partial class, it works.

Expected behavior

Source Generator should find ILogger declaration in every partial class.

If it isn't possible, the problem should be documented.

Actual behavior

Described into "Reproduction Steps"

Regression?

No response

Known Workarounds

Moving the declaration from one physical file to the file containing logging method works, but if you define partial methods in multiple partial classes, the problem can't be solved and you can't use partial methods that implicitly get reference to the ILogger.

Configuration

.Net 6
Visual Studio 2022

Other information

Having to move ILogger definition, even if easy, isn't the best because when using dependency injection the workflow is usually to declare the constructor parameter you want to inject and then use helpers like VS suggestion action "create and assign field" that of course defines the filed in the class containing the constructor

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

I found a bug that causes the error SYSLIB1019 when it shouldn't be triggered.

If you declare the ILogger field in a file containing part of your partial class (let's call the class Foo)and you define the log methods that uses source generation logging in another physical file that's of course a partial class of Foo, the compiler complain with the error

SYSLIB1019 Couldn't find a field of type Microsoft.Extensions.Logging.ILogger in class Foo

If I move the ILogger declaration from first physical file to the other containing log methods, it works.

Reproduction Steps

Create a project that makes use of logging, create a Foo class like this, in two distinct files (probably even if you declare them in a single file should trigger the problem too)

public partial class Foo
{
   ILogger _logger;

   public void Test()
   {
      LogPeriodicWorkFailure("workName");
   }
}

public partial class Foo
{
   [LoggerMessage(0, LogLevel.Critical, "An unhandled exception has been raised in the {PeriodicWork} work.")]
   partial void LogPeriodicWorkFailure(string periodicWork);
}

If you move the declaration ILogger _logger; to the second partial class, it works.

Expected behavior

Source Generator should find ILogger declaration in every partial class.

If it isn't possible, the problem should be documented.

Actual behavior

Described into "Reproduction Steps"

Regression?

No response

Known Workarounds

Moving the declaration from one physical file to the file containing logging method works, but if you define partial methods in multiple partial classes, the problem can't be solved and you can't use partial methods that implicitly get reference to the ILogger.

Configuration

.Net 6
Visual Studio 2022

Other information

No response

Author:	MithrilMan
Assignees:	-
Labels:	untriaged, area-Extensions-Logging
Milestone:	-

[MithrilMan]

@maryamariyan Hello I see you tagget this on 7.0.0
Is there any technically issue that prevent to be fixed earlier or is a low priority issue?

Also I've found other nuances that present the problem: when you inherit a class that defines a protected ILogger logger you can't use that member and you are forced to create a local private variable that hold the ILogger reference.

A simple repro code is this

public class BaseClass
{
   protected ILogger _logger { get; }

   public BaseClass(ILogger logger)
   {
      _logger = logger;
   }
}

public partial class MyClass : BaseClass
{
   public MyClass(ILogger<MyClass> logger) : base(logger) { }

   [LoggerMessage(0, LogLevel.Debug, "Test.")]
   partial void Test();
}

If you try to compile it generates the error SYSLIB1019
Couldn't find a field of type Microsoft.Extensions.Logging.ILogger in class MyClass

[maryamariyan]

@maryamariyan Hello I see you tagget this on 7.0.0. Is there any technically issue that prevent to be fixed earlier or is a low priority issue?

Thanks for your feedback, we're gathering feedback from the logging source generator and would fix them early in the 7.0 cycle.

Fixing this issue has priority as it is part of a newly released feature. 6.0 has already shipped that's why this is tagged as 7.0, unless we think this also needs to be serviced in 6.0.

[ericstj]

Probably we can ask someone from roslyn what's the best way to locate a field across type partial type definitions. Looks like the generator does:

runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

Lines 525 to 555 in 24df302

	private (string? loggerField, bool multipleLoggerFields) FindLoggerField(SemanticModel sm, TypeDeclarationSyntax classDec, ITypeSymbol loggerSymbol)
	{
	string? loggerField = null;
	foreach (MemberDeclarationSyntax m in classDec.Members)
	{
	if (m is FieldDeclarationSyntax fds)
	{
	foreach (VariableDeclaratorSyntax v in fds.Declaration.Variables)
	{
	var fs = sm.GetDeclaredSymbol(v, _cancellationToken) as IFieldSymbol;
	if (fs != null)
	{
	if (IsBaseOrIdentity(fs.Type, loggerSymbol))
	{
	if (loggerField == null)
	{
	loggerField = v.Identifier.Text;
	}
	else
	{
	return (null, true);
	}
	}
	}
	}
	}
	}
	return (loggerField, false);
	}

@chsienki @sharwell do you have suggestions?

[elinor-fung]

I'd expect you want to search on the class symbol rather than the syntax - looks like you already have / use the semantic model to grab symbols at this point. With the symbol, you should be able to iterate through all the class members rather than just the ones on that part of the declaration.

[SteveDunn]

I'm no expert in source generators, but I had a stab at it: #71308

It fixes both scenarios mentioned here, and all existing tests are still green.

[dss539]

This can be worked around by defining redundant ILogger members in every partial file. e.g. _logger, _logger2, _logger3.
Assign the same logger to each field and the generated code will use it.

[SteveDunn]

This can be worked around by defining redundant ILogger members in every partial file. e.g. _logger, _logger2, _logger3. Assign the same logger to each field and the generated code will use it.

That's a reasonable workaround, although I would guess analyzers, such as ReSharper would compain of 'field is assigned but never used'. Hopefully this PR will be approved soon and it'll make it into .NET 7 🤞