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

[maxkoshevoi]

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?

[dotnet-issue-labeler]

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]

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]

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

[eerhardt]

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.

[maxkoshevoi]

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]

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]

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]

@maryamariyan - did the CI run for this PR?

[maryamariyan]

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