[Group 4] Enable nullable annotations for `Microsoft.Extensions.Logging.TraceSource` by maxkoshevoi · Pull Request #66892 · dotnet/runtime

maxkoshevoi · 2022-03-19T23:48:43Z

Related to #43605

Questions:

BeginScope should be annotated with where TState : notnull, but it cannot, since it's not annotated this way in ILogger

runtime/src/libraries/Microsoft.Extensions.Logging.TraceSource/src/TraceSourceLogger.cs

Line 73 in 3e2a45c

public IDisposable BeginScope<TState>(TState state)
Microsoft.Extensions.Logging.EventLog cannot be annotated since System.Diagnostics.EventLog is not annotated yet. Should something be done about it, or let it be for now?

ghost · 2022-03-19T23:48:49Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-03-19T23:48:51Z

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #43605

Questions:

BeginScope should be annotated with where TState : notnull, but it cannot, since it's not annotated this way in ILogger

runtime/src/libraries/Microsoft.Extensions.Logging.TraceSource/src/TraceSourceLogger.cs

Line 73 in 3e2a45c

public IDisposable BeginScope<TState>(TState state)
Microsoft.Extensions.Logging.EventLog cannot be annotated since System.Diagnostics.EventLog is not annotated yet. Should something be done about it, or let it be for now?

Author:	maxkoshevoi
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-Extensions-Logging`, `community-contribution`
Milestone:	-

eerhardt · 2022-03-21T18:25:19Z

BeginScope should be annotated with where TState : notnull, but it cannot, since it's not annotated this way in ILogger

We can still change it in ILogger, if that is the correct thing to do. Doing a real quick look, I think it is.

BeginScope should also be annotated as returning IDisposable? because there are multiple places that return null:

runtime/src/libraries/Microsoft.Extensions.Logging.EventLog/src/EventLogLogger.cs

Lines 51 to 54 in eb51b02

    
           public IDisposable BeginScope<TState>(TState state) 
        
           { 
        
               return _externalScopeProvider?.Push(state); 
        
           }

https://github.com/dotnet/aspnetcore/blob/17695facaeaf9d6d83aea9bfe87747180922117a/src/Logging.AzureAppServices/src/BatchingLogger.cs#L21-L24

…acesource

eerhardt · 2022-03-25T13:41:25Z

Microsoft.Extensions.Logging.EventLog cannot be annotated since System.Diagnostics.EventLog is not annotated yet. Should something be done about it, or let it be for now?

@maxkoshevoi - is annotating System.Diagnostics.EventLog something you would be interested in doing as well? If not, that is totally fine.

eerhardt

LGTM. Just two tiny comments.

src/libraries/Microsoft.Extensions.Logging.TraceSource/src/TraceSourceLogger.cs

maxkoshevoi · 2022-03-25T13:48:16Z

is annotating System.Diagnostics.EventLog something you would be interested in doing as well? If not, that is totally fine.

Sure, I was running out of things to annotate anyway 😄
Does this apply to System.Security.Cryptography.Xml too (prerequisite for annotating Microsoft.Extensions.Configuration.Xml)?

eerhardt · 2022-03-25T13:58:46Z

Does this apply to System.Security.Cryptography.Xml too?

For Microsoft.Extensions.Configuration.Xml, I only know of 1 place that uses System.Security.Cryptography.Xml:

runtime/src/libraries/Microsoft.Extensions.Configuration.Xml/src/XmlDocumentDecryptor.cs

Lines 92 to 102 in 000e1a8

    
           protected virtual XmlReader DecryptDocumentAndCreateXmlReader(XmlDocument document) 
        
           { 
        
               // Perform the actual decryption step, updating the XmlDocument in-place. 
        
               EncryptedXml encryptedXml = _encryptedXmlFactory?.Invoke(document) ?? new EncryptedXml(document); 
        
               encryptedXml.DecryptDocument(); 
        
               // Finally, return the new XmlReader from the updated XmlDocument. 
        
               // Error messages based on this XmlReader won't show line numbers, 
        
               // but that's fine since we transformed the document anyway. 
        
               return document.CreateNavigator().ReadSubtree(); 
        
           }

I think it would be pretty easy to nullable annotate Microsoft.Extensions.Configuration.Xml without having System.Security.Cryptography.Xml annotated.

But if you wanted to annotate System.Security.Cryptography.Xml, we should check with @bartonjs before starting on it, to be sure he would be open to accepting a PR to nullable annotate it.

bartonjs · 2022-03-25T16:22:22Z

But if you wanted to annotate System.Security.Cryptography.Xml, we should check with @bartonjs before starting on it, to be sure he would be open to accepting a PR to nullable annotate it.

Yeah. It's not SO dead that we're not willing to nullable annotate it. 😄 (In fact, there's an open PR for fixing a bug in it.)

eerhardt · 2022-03-25T19:50:56Z

@maryamariyan - did the CI run for this PR?

…ng.TraceSource` (dotnet#66892)

maryamariyan

LGTM. @eerhardt not sure what happened with the CI here.

Annotate things

e8243c1

ghost added community-contribution Indicates that the PR has been added by a community member area-Extensions-Logging new-api-needs-documentation labels Mar 19, 2022

This was referenced Mar 21, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

maxkoshevoi mentioned this pull request Mar 21, 2022

Improve nullable annotations for ILogger.BeginScope #66960

Merged

maxkoshevoi added 3 commits March 23, 2022 23:45

Merge branch 'main' into mk/43605-logging-tracesource

5f632a5

Update TraceSourceLogger.cs

a740cef

Merge remote-tracking branch 'upstream/main' into mk/43605-logging-tr…

2cf01a4

…acesource

eerhardt approved these changes Mar 25, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.TraceSource/src/TraceSourceLogger.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Logging.TraceSource/src/TraceSourceLogger.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

972f04e

maryamariyan approved these changes Mar 25, 2022

View reviewed changes

maryamariyan merged commit a6d2b10 into dotnet:main Mar 25, 2022

maxkoshevoi deleted the mk/43605-logging-tracesource branch March 25, 2022 19:55

This was referenced Mar 27, 2022

Annotate Microsoft.Extensions.* assemblies for nullable reference types #43605

Closed

[Group 5] Enable nullable annotations for Microsoft.Extensions.Configuration.Xml #67178

Merged

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022

[Group 4] Enable nullable annotations for `Microsoft.Extensions.Loggi…

5f1d2b3

…ng.TraceSource` (dotnet#66892)

maryamariyan reviewed Apr 1, 2022

View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Group 4] Enable nullable annotations for `Microsoft.Extensions.Logging.TraceSource`#66892

[Group 4] Enable nullable annotations for `Microsoft.Extensions.Logging.TraceSource`#66892
maryamariyan merged 5 commits intodotnet:mainfrom
maxkoshevoi:mk/43605-logging-tracesource

maxkoshevoi commented Mar 19, 2022

Uh oh!

ghost commented Mar 19, 2022

Uh oh!

ghost commented Mar 19, 2022

Uh oh!

eerhardt commented Mar 21, 2022

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

eerhardt left a comment

Uh oh!

Uh oh!

Uh oh!

maxkoshevoi commented Mar 25, 2022 •

edited

Loading

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

bartonjs commented Mar 25, 2022

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

maryamariyan left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

maxkoshevoi commented Mar 19, 2022

Uh oh!

ghost commented Mar 19, 2022

Uh oh!

ghost commented Mar 19, 2022

Uh oh!

eerhardt commented Mar 21, 2022

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maxkoshevoi commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

bartonjs commented Mar 25, 2022

Uh oh!

eerhardt commented Mar 25, 2022

Uh oh!

maryamariyan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxkoshevoi commented Mar 25, 2022 •

edited

Loading

maryamariyan left a comment •

edited

Loading