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.

Sign up for GitHub

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

Jump to bottom

[Group 4] Enable nullable annotations for Microsoft.Extensions.Logging.EventSource #66802

Merged
merged 7 commits into from
Mar 23, 2022
Merged

[Group 4] Enable nullable annotations for Microsoft.Extensions.Logging.EventSource #66802

merged 7 commits into from
Mar 23, 2022

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Mar 17, 2022 •
edited
Loading

Related to #43605

Questions:

CS0433	The type 'Unsafe' exists in both 'System.Runtime.CompilerServices.Unsafe, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'	Microsoft.Extensions.Logging.EventSource (net7.0)

image

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 17, 2022
@ghost
Copy link

ghost commented Mar 18, 2022

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:

CS0433	The type 'Unsafe' exists in both 'System.Runtime.CompilerServices.Unsafe, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'	Microsoft.Extensions.Logging.EventSource (net7.0)

image

Author: maxkoshevoi
Assignees: -
Labels:

area-Extensions-Logging, community-contribution
Milestone: -

@eerhardt
Copy link
Member

eerhardt commented Mar 18, 2022 •
edited
Loading

I'm unable to build this repository for the pas couple of weeks, installing .Net 7 Preview 3 from main doesn't help. Is there something I'm missing?

I would try cleaning my repo: git clean -xdf and then rebuild from the root .\build.cmd libs+clr -rc Release. If you still have errors, let me know.

eerhardt
eerhardt reviewed Mar 18, 2022
View reviewed changes
...ies/Microsoft.Extensions.Logging.EventSource/ref/Microsoft.Extensions.Logging.EventSource.cs Outdated
@@ -17,7 +17,7 @@ namespace Microsoft.Extensions.Logging.EventSource
public partial class EventSourceLoggerProvider : Microsoft.Extensions.Logging.ILoggerProvider, System.IDisposable
{
public EventSourceLoggerProvider(Microsoft.Extensions.Logging.EventSource.LoggingEventSource eventSource) { }
public Microsoft.Extensions.Logging.ILogger CreateLogger(string categoryName) { throw null; }
public Microsoft.Extensions.Logging.ILogger CreateLogger(string? categoryName) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILoggerProvider has categoryName as non-nullable. So this should be string categoryName.

We don't want to allow this to be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should EventSourceLogger.CategoryName be nullable then? Maybe it doesn't really matter sines it's an internal API, and nothing breaks when it's nullable, but still.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should EventSourceLogger.CategoryName be nullable then?

I would say no. If nothing ever creates it with a null name, then I wouldn't make it nullable.

@maxkoshevoi maxkoshevoi marked this pull request as ready for review March 18, 2022 21:15
@eerhardt
Copy link
Member

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@eerhardt
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 21, 2022
Closed
Closed
eerhardt
eerhardt approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution here, @maxkoshevoi!

src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs
{
if (!command.Arguments.TryGetValue("FilterSpecs", out string filterSpec))
if (!command.Arguments!.TryGetValue("FilterSpecs", out string? filterSpec))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place that concerns me. But if it hasn't been a problem, I guess we can just suppress the null check here. Looking around I see similar code elsewhere:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

Lines 393 to 397 in c5d40c9

if ((command.Command == EventCommand.Update || command.Command == EventCommand.Enable) &&
IsEnabled(EventLevel.Informational, Keywords.Events))
{
string? filterAndPayloadSpecs = null;
command.Arguments!.TryGetValue("FilterAndPayloadSpecs", out filterAndPayloadSpecs);

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs

Lines 53 to 59 in c5d40c9

private void OnEventSourceCommand(object? sender, EventCommandEventArgs e)
{
if (e.Command == EventCommand.Enable || e.Command == EventCommand.Update)
{
Debug.Assert(e.Arguments != null);
if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value))

@eerhardt eerhardt merged commit 103fb84 into dotnet:main Mar 23, 2022
@maxkoshevoi maxkoshevoi mentioned this pull request Mar 23, 2022
Merged
@maxkoshevoi maxkoshevoi deleted the mk/43605-Logging-EventSource branch March 23, 2022 21:30
@danmoseley
Copy link
Member

+1 we appreciate your hard work on all these annotations @maxkoshevoi .

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@maxkoshevoi @radekdoulik
[Group 4] Enable nullable annotations for `Microsoft.Extensions.Loggi…
72ca72d 
…ng.EventSource` (dotnet#66802)

* Annotate src

* Annotate ref

* Make internal parameters non-nullable if they never receive null
@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Extensions-Logging community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxkoshevoi @eerhardt @danmoseley