Logging Source Generator fails with partial methods with implicit ILogger definition when using partial classes
MithrilMan opened this issue · 8 comments
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
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: |
|
| Milestone: | - |
@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 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.
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:
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.
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.
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.
This can be worked around by defining redundant
ILoggermembers 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 🤞