Skip to content

ProviderAliasAttribute type name reference changed from plain string to type safe expression #114607

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

Closed
SetTrend wants to merge 2 commits into from
Closed

ProviderAliasAttribute type name reference changed from plain string to type safe expression #114607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
453a410
`Microsoft.Extensions.Logging.ProviderAliasAttribute` moved to `Micro…
SetTrend Apr 13, 2025
7847046
`ProviderAliasAttribute` type name reference changed from plain strin…
SetTrend Apr 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/Common/src/Extensions/ProviderAliasUtilities/ProviderAliasUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Extensions.Logging
{
internal static class ProviderAliasUtilities
{
private const string AliasAttributeTypeFullName = "Microsoft.Extensions.Logging.ProviderAliasAttribute";
private static readonly string AliasAttributeTypeFullName = typeof(ProviderAliasAttribute).FullName!;
Copy link
Member

@jkotas jkotas Apr 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@SetTrend SetTrend Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Apr 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@jkotas jkotas Apr 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jkotas jkotas Apr 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SetTrend SetTrend Apr 14, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@SetTrend SetTrend Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@SetTrend SetTrend Apr 14, 2025

Choose a reason for hiding this comment

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

I just reopened #114606. Please choose.


internal static string? GetAlias(Type providerType)
{
Expand Down
6 changes: 6 additions & 0 deletions ...icrosoft.Extensions.Logging.Abstractions/ref/Microsoft.Extensions.Logging.Abstractions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ public enum LogLevel
Critical = 5,
None = 6,
}
[System.AttributeUsageAttribute(System.AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
public partial class ProviderAliasAttribute : System.Attribute
{
public ProviderAliasAttribute(string alias) { }
public string Alias { get { throw null; } }
}
}
namespace Microsoft.Extensions.Logging.Abstractions
{
Expand Down
0 ...ons.Logging/src/ProviderAliasAttribute.cs → ...bstractions/src/ProviderAliasAttribute.cs
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Logging/ref/Microsoft.Extensions.Logging.Forwards.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
// ------------------------------------------------------------------------------

[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(Microsoft.Extensions.Logging.ILoggingBuilder))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(Microsoft.Extensions.Logging.ProviderAliasAttribute))]
6 changes: 0 additions & 6 deletions src/libraries/Microsoft.Extensions.Logging/ref/Microsoft.Extensions.Logging.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,4 @@ public static partial class LoggingBuilderExtensions
public static Microsoft.Extensions.Logging.ILoggingBuilder Configure(this Microsoft.Extensions.Logging.ILoggingBuilder builder, System.Action<Microsoft.Extensions.Logging.LoggerFactoryOptions> action) { throw null; }
public static Microsoft.Extensions.Logging.ILoggingBuilder SetMinimumLevel(this Microsoft.Extensions.Logging.ILoggingBuilder builder, Microsoft.Extensions.Logging.LogLevel level) { throw null; }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Class, AllowMultiple=false, Inherited=false)]
public partial class ProviderAliasAttribute : System.Attribute
{
public ProviderAliasAttribute(string alias) { }
public string Alias { get { throw null; } }
}
}
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Logging/src/Properties/TypeForwards.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
// The .NET Foundation licenses this file to you under the MIT license.

[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(Microsoft.Extensions.Logging.ILoggingBuilder))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(Microsoft.Extensions.Logging.ProviderAliasAttribute))]
21 changes: 0 additions & 21 deletions src/libraries/Microsoft.Extensions.Logging/tests/Common/ProviderAliasAttribute.cs
View file
Open in desktop

This file was deleted.

Loading