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.

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

Add overload resolution priority #49

Closed
Cheesebaron opened this issue Mar 3, 2025 · 6 comments · Fixed by #50
Closed

Add overload resolution priority #49

Cheesebaron opened this issue Mar 3, 2025 · 6 comments · Fixed by #50

Comments

@Cheesebaron
Copy link
Contributor

It is way too easy to hit the signature for the extension Format(string pattern, object data), since a Dictionary is also an object.

I suggest that [OverloadResolutionPriority(-1)] is added to give it less priority.

Additionally, I would warn users that it does reflection in the code behind, which is not compatible with platforms such as net-ios and net-android if you want to use NativeAOT.

@jeffijoe
Copy link
Owner

jeffijoe commented Mar 3, 2025

Good point. We should also slap on [RequiresUnreferencedCode]. The README does mention that the object-based overload uses reflection, though.

@Cheesebaron
Copy link
Contributor Author

If you want I can contribute these changes.

@jeffijoe
Copy link
Owner

jeffijoe commented Mar 3, 2025

That would be great!

@jeffijoe
Copy link
Owner

jeffijoe commented Mar 3, 2025

Does OverloadResolutionPriority work on older .NET versions?

@Cheesebaron
Copy link
Contributor Author

Cheesebaron commented Mar 3, 2025

It can be polyfilled by adding an attribute in the project. Something like this:

namespace System.Runtime.CompilerServices;

/// <summary>
/// Specifies the priority of a member in overload resolution. When unspecified, the default priority is 0.
/// </summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
internal sealed class OverloadResolutionPriorityAttribute : Attribute
{
    /// <summary>
    /// Initializes a new instance of the <see cref="OverloadResolutionPriorityAttribute"/> class.
    /// </summary>
    /// <param name="priority">The priority of the attributed member. Higher numbers are prioritized, lower numbers are deprioritized. 0 is the default if no attribute is present.</param>
    public OverloadResolutionPriorityAttribute(int priority)
    {
        Priority = priority;
    }

    /// <summary>
    /// The priority of the member.
    /// </summary>
    public int Priority { get; }
}

However, DynamycallyAccessedMembers or RequiresUnreferencedCode requires net6.0+.

So I was thinking of adding a preprocessor condition for these where needed.

Alternatively, you could drop support for netstandard, but not sure if you want that.

@jeffijoe
Copy link
Owner

jeffijoe commented Mar 3, 2025

Preprocessor condition sounds like the way to go.

I bet PolySharp can provide the attribute; feel free to add it if it does.

EDIT: Looks like PolySharp provides the other two as well, so a preprocessor directive wouldn't be required.

jeffijoe added a commit that referenced this issue Mar 3, 2025
…quiresunref-code-attrs

Add RequiresUnreferencedCode attribute for reflection code and add OverloadResolutionPriority attribute to prefer non-object methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants