`ProviderAliasAttribute` type name reference changed from plain string to type safe expression by SetTrend · Pull Request #114607 · dotnet/runtime

SetTrend · 2025-04-13T22:10:35Z

ghost · 2025-04-13T22:10:40Z

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, 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 · 2025-04-13T22:10:42Z

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, 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.

dotnet-policy-service · 2025-04-13T22:11:19Z

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

…soft.Extensions.Logging.Abstractions` solution.

…g to type safe expression.

tarekgh · 2025-04-13T23:39:35Z

This LGTM. You may close the other PR #114606. I marked this PR as ready for review too.

Copilot

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/Microsoft.Extensions.Logging/tests/Common/ProviderAliasAttribute.cs:1

Removing the ProviderAliasAttribute test implementation may reduce test coverage for alias functionality; please ensure that equivalent tests exist elsewhere.

// Licensed to the .NET Foundation under one or more agreements.

tarekgh · 2025-04-14T00:00:29Z

CC @ericstj

jkotas · 2025-04-14T00:16:33Z

    internal static class ProviderAliasUtilities
    {
-        private const string AliasAttributeTypeFullName = "Microsoft.Extensions.Logging.ProviderAliasAttribute";
+        private static readonly string AliasAttributeTypeFullName = typeof(ProviderAliasAttribute).FullName!;


This is just an expensive way to compute the constant string. Is it really worth it?

It is very common to hardcode type names as strings. You can find many instances of hardcoded type name strings in this repo.

It's a single call, done only once during the lifetime of an application (static). Do you think this makes much of a difference?

I would say yes. This might root the reflection stack on Native AOT.

Do you think this makes much of a difference?

One-off cases won't make a difference. If we were to use pattern like this everywhere instead of hardcoded strings, it would make a difference.

In other words, this is inconsistent with what we typically do in the rest of the repo.

I gave it a second thought: Since ProviderAliasAttribute is statically linked in this project, there won't be any other class definition with this FQ name.

So, it may be logical and coherent to replace the const string comparison

if (attributeData.AttributeType.FullName == AliasAttributeTypeFullName &&

in line 22 with an is operator

if (attributeData.AttributeType is ProviderAliasAttribute

What do you think?

I forgot to mention that I very much agree with your valuable AOT argument.

I'm not sure if the is operator is made available for all .NET versions.

This particular change I provided just as an option. We can easily refrain from this change and move over to PR #114606. It contains all the changes I made without the string comparison being changed.

I just reopened #114606. Please choose.

KalleOlaviNiemitalo · 2025-04-14T04:40:50Z

-    /// Test implementation of ProviderAliasAttribute
-    /// </summary>
-    [AttributeUsage(AttributeTargets.Class)]
-    public class ProviderAliasAttribute : Attribute


This seems to be used in src/libraries/Microsoft.Extensions.Logging/tests/Common/TestLoggerProvider.cs for testing that the comparison in ProviderAliasUtilities is indeed by name and not by type identity. I think it is better to keep this file.

runtime/src/libraries/Microsoft.Extensions.Logging/tests/Common/TestLoggerProvider.cs

Lines 9 to 12 in 2f5c61c

#pragma warning disable CS0436 // Type conflicts with imported type

[ProviderAlias("TestLogger")]

#pragma warning restore CS0436 // Type conflicts with imported type

public class TestLoggerProvider : ILoggerProvider

@KalleOlaviNiemitalo: I can't find your comment in the "Files changed" tab. Is your comment still valid?

Since the ProviderAliasAttribute moved to Abstractions, it's now available in TestLoggerProvider.cs, so there is no need for duplicate definition. That's why I removed that file.

I found it nice to see that everything falls in place now with the change. No need for a quirk like a duplicate class definition or string comparisons originally implemented as a workaround due to the original class definition not having been available where it should have been.

Yes it is still valid.

I mean, if some third-party logger provider currently references Microsoft.Extensions.Logging.Abstractions rather than Microsoft.Extensions.Logging, and defines ProviderAliasAttribute as an internal type, then ProviderAliasUtilities should keep recognizing that. And there should be a test to verify that ProviderAliasUtilities does so.

In the existing tests, TestLoggerProvider currently uses the local definition of ProviderAliasAttribute from
src/libraries/Microsoft.Extensions.Logging/tests/Common/ProviderAliasAttribute.cs. If you delete that file, then it will instead use the ProviderAliasAttribute definition that you moved to Microsoft.Extensions.Logging.Abstractions. So this TestLoggerProvider can no longer be used for testing that ProviderAliasUtilities recognizes types by name.

So, you also plead for keeping the string comparison, like @jkotas does?

No problem. That's why I intentionally created a separate pull request for this. It is just a test drive.

As I wrote above, we can easily refrain from this change and move over to PR #114606. It contains all the changes I made without the string comparison being changed.

In fact, there are internal definitions of ProviderAliasAttribute in these projects:

https://github.com/DataDog/dd-trace-dotnet/blob/b4342eff1e242dda7b47cc2c90232efa1bf0b221/tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/DirectSubmission/ProviderAliasAttribute.cs#L18
https://github.com/microsoft/ApplicationInsights-dotnet/blob/a02249a047eb6f97f83e2b733aedc902a7c92363/NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/Implementation/ProviderAliasAttribute.cs#L9

If you replace the string comparison with a type comparison, it will be a breaking change for those. More breaking than just moving ProviderAliasAttribute from Microsoft.Extensions.Logging to Microsoft.Extensions.Logging.Abstractions.

I just reopened #114606. Please choose.

I can't find your comment in the "Files changed" tab.

That's because the comment is on a deleted file. If you click "Load diff" above "This file was deleted", then it will show the diff and the comment.

tarekgh · 2025-04-14T15:46:13Z

Closing this for the favor of #114606

ghost added area-Extensions-Logging new-api-needs-documentation labels Apr 13, 2025

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 13, 2025

build-analysis Bot mentioned this pull request Apr 13, 2025

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

SetTrend changed the title ~~ProviderAliasAttribute type name reference changed from plain string to type safe expression~~ WIP: ProviderAliasAttribute type name reference changed from plain string to type safe expression Apr 13, 2025

SetTrend added 2 commits April 14, 2025 01:18

Microsoft.Extensions.Logging.ProviderAliasAttribute moved to `Micro…

453a410

…soft.Extensions.Logging.Abstractions` solution.

ProviderAliasAttribute type name reference changed from plain strin…

7847046

…g to type safe expression.

tarekgh added this to the 10.0.0 milestone Apr 13, 2025

tarekgh marked this pull request as ready for review April 13, 2025 23:41

Copilot AI review requested due to automatic review settings April 13, 2025 23:41

Copilot AI reviewed Apr 13, 2025

View reviewed changes

tarekgh changed the title ~~WIP: ProviderAliasAttribute type name reference changed from plain string to type safe expression~~ ProviderAliasAttribute type name reference changed from plain string to type safe expression Apr 13, 2025

jkotas reviewed Apr 14, 2025

View reviewed changes

This was referenced Apr 14, 2025

[wasm][AOT][net8.0] emcc : error - received SIGKILL (-9) #89402

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts #114241

Closed

KalleOlaviNiemitalo reviewed Apr 14, 2025

View reviewed changes

KalleOlaviNiemitalo mentioned this pull request Apr 14, 2025

ProviderAliasAttribute moved to Microsoft.Extensions.Logging.Abstractions solution. #114606

Merged

tarekgh closed this Apr 14, 2025

github-actions Bot locked and limited conversation to collaborators May 15, 2025

	#pragma warning disable CS0436 // Type conflicts with imported type
	[ProviderAlias("TestLogger")]
	#pragma warning restore CS0436 // Type conflicts with imported type
	public class TestLoggerProvider : ILoggerProvider

Conversation

SetTrend commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 13, 2025

Uh oh!

ghost commented Apr 13, 2025

Uh oh!

dotnet-policy-service Bot commented Apr 13, 2025

Uh oh!

tarekgh commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SetTrend Apr 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SetTrend Apr 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SetTrend Apr 14, 2025 • 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!

KalleOlaviNiemitalo Apr 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SetTrend commented Apr 13, 2025 •

edited

Loading

tarekgh commented Apr 13, 2025 •

edited

Loading

SetTrend Apr 14, 2025 •

edited

Loading

SetTrend Apr 14, 2025 •

edited

Loading

SetTrend Apr 14, 2025 •

edited

Loading

KalleOlaviNiemitalo Apr 14, 2025 •

edited

Loading