Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging Source Generator fails with a NullReferenceException #52277

Closed
eerhardt opened this issue May 4, 2021 · 3 comments
Closed

Logging Source Generator fails with a NullReferenceException #52277

eerhardt opened this issue May 4, 2021 · 3 comments
Assignees
Labels
area-Extensions-Logging bug enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented May 4, 2021

Repro

  1. Create a new ClassLibrary
  2. Add <PackageReference Include="Microsoft.Extensions.Logging" Version="6.0.0-preview.4.21222.10" />
  3. Write the following code in Class1.cs
using System;
using Microsoft.Extensions.Logging;

namespace ClassLibrary1
{
    public partial class Class1
    {
        private ILogger _logger;
        public Class1(ILogger logger) => _logger = logger;

        internal static class EventIds
        {
            public const int HealthCheckErrorId = 100;
        }

        [LoggerMessage(EventId = HealthCheckErrorId, Level = LogLevel.Error, Message = "Health check {HealthCheckName} threw an unhandled exception after {ElapsedMilliseconds}ms")]
        private partial void HealthCheckError(string HealthCheckName, double ElapsedMilliseconds, Exception exception);
    }
}
  1. Build

Actual results

I'm seeing the following warning in my Error List

Severity	Code	Description	Project	File	Line	Suppression State
Warning	CS8785	Generator 'LoggerMessageGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'	ClassLibrary1	C:\Users\eerhardt\source\repos\ClassLibrary1\ClassLibrary1\CSC	1	Active

Expected results

I only expect my compile error (here I didn't put EventIds. in front of HealthCheckErrorId in the LoggerMessage attribute) to exist in the Error List. I don't expect the Logging Source Generator to fail and add more errors.

cc @maryamariyan

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Logging untriaged New issue has not been triaged by the area owner labels May 4, 2021
@ghost
Copy link

ghost commented May 4, 2021

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

Issue Details

Repro

  1. Create a new ClassLibrary
  2. Add <PackageReference Include="Microsoft.Extensions.Logging" Version="6.0.0-preview.4.21222.10" />
  3. Write the following code in Class1.cs
using System;
using Microsoft.Extensions.Logging;

namespace ClassLibrary1
{
    public partial class Class1
    {
        private ILogger _logger;
        public Class1(ILogger logger) => _logger = logger;

        internal static class EventIds
        {
            public const int HealthCheckErrorId = 100;
        }

        [LoggerMessage(EventId = HealthCheckErrorId, Level = LogLevel.Error, Message = "Health check {HealthCheckName} threw an unhandled exception after {ElapsedMilliseconds}ms")]
        private partial void HealthCheckError(string HealthCheckName, double ElapsedMilliseconds, Exception exception);
    }
}
  1. Build

Actual results

I'm seeing the following warning in my Error List

Severity	Code	Description	Project	File	Line	Suppression State
Warning	CS8785	Generator 'LoggerMessageGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'	ClassLibrary1	C:\Users\eerhardt\source\repos\ClassLibrary1\ClassLibrary1\CSC	1	Active

Expected results

I only expect my compile error (here I didn't put EventIds. in front of HealthCheckErrorId in the LoggerMessage attribute) to exist in the Error List. I don't expect the Logging Source Generator to fail and add more errors.

cc @maryamariyan

Author: eerhardt
Assignees: -
Labels:

area-Extensions-Logging, untriaged

Milestone: -

@maryamariyan maryamariyan added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels May 4, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone May 4, 2021
@ericstj ericstj added bug and removed enhancement Product code improvement that does NOT require public API changes/additions labels May 5, 2021
@ericstj
Copy link
Member

ericstj commented May 5, 2021

I only expect my compile error (here I didn't put EventIds. in front of HealthCheckErrorId in the LoggerMessage attribute) to exist in the Error List. I don't expect the Logging Source Generator to fail and add more errors.

Agree with @eerhardt that we need to ensure our source generators are resilient to bad syntax in user's source files (cc @layomia in case similar concerns apply to Json). We probably need test cases for invalid source to ensure our generators handle these cases well.

@ericstj ericstj added the enhancement Product code improvement that does NOT require public API changes/additions label May 5, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 17, 2021
@maryamariyan
Copy link
Member

Closed via PR #54305

@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging bug enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants