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

RuntimeHandles should implement IEquatable #62944

Closed
MichalStrehovsky opened this issue Dec 17, 2021 · 6 comments
Closed

RuntimeHandles should implement IEquatable #62944

MichalStrehovsky opened this issue Dec 17, 2021 · 6 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer

Comments

@MichalStrehovsky
Copy link
Member

Background and motivation

RuntimeTypeHandle is often used as a key in a dictionary. See hits on github: https://grep.app/search?q=dictionary%3Cruntimetypehandle. The search results include high profile libraries such a CsWinRt.

The runtime handles already implement strongly typed Equals(XXXHandle other) methods, but without the interfaces, dictionaries are going to use the Equals(object other) implementations instead, needlessly boxing the argument, unless a custom comparer is provided (it isn't in the above search results...).

API Proposal

namespace System
{
    public struct RuntimeTypeHandle : IEquatable<RuntimeTypeHandle>
    {
        // bool Equals(RuntimeTypeHandle) already exists
    }

    public struct RuntimeMethodHandle : IEquatable<RuntimeMethodHandle>
    {
        // bool Equals(RuntimeMehtodHandle) already exists
    }

    public struct RuntimeFieldHandle : IEquatable<RuntimeFieldHandle>
    {
        // bool Equals(RuntimeFieldHandle) already exists
    }
}

API Usage

No boxing.

Alternative Designs

No response

Risks

No response

@MichalStrehovsky MichalStrehovsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime api-ready-for-review API is ready for review, it is NOT ready for implementation labels Dec 17, 2021
@ghost
Copy link

ghost commented Dec 17, 2021

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

Issue Details

Background and motivation

RuntimeTypeHandle is often used as a key in a dictionary. See hits on github: https://grep.app/search?q=dictionary%3Cruntimetypehandle. The search results include high profile libraries such a CsWinRt.

The runtime handles already implement strongly typed Equals(XXXHandle other) methods, but without the interfaces, dictionaries are going to use the Equals(object other) implementations instead, needlessly boxing the argument, unless a custom comparer is provided (it isn't in the above search results...).

API Proposal

namespace System
{
    public struct RuntimeTypeHandle : IEquatable<RuntimeTypeHandle>
    {
        // bool Equals(RuntimeTypeHandle) already exists
    }

    public struct RuntimeMethodHandle : IEquatable<RuntimeMethodHandle>
    {
        // bool Equals(RuntimeMehtodHandle) already exists
    }

    public struct RuntimeFieldHandle : IEquatable<RuntimeFieldHandle>
    {
        // bool Equals(RuntimeFieldHandle) already exists
    }
}

API Usage

No boxing.

Alternative Designs

No response

Risks

No response

Author: MichalStrehovsky
Assignees: -
Labels:

api-suggestion, area-System.Runtime, api-ready-for-review

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 17, 2021
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Dec 17, 2021
To make sure ApiCompat tooling runs at least in some legs, compile libs against NativeAOT corelib if we're building CLR, but not building the JIT flavor of the runtime.

The baselining is necessary because the reflection stack of NativeAOT doesn't live in CoreLib.

The CannotRemoveBaseTypeOrInteface baselining will go away once we fix dotnet#62944. It's one of the "overall goodness" things we can take out of NativeAOT and put it into all runtimes.
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Dec 17, 2021
MichalStrehovsky added a commit that referenced this issue Dec 18, 2021
To make sure ApiCompat tooling for NativeAOT's CoreLib runs at least in some legs, compile libs against NativeAOT CoreLib if we're building CLR, but not building the JIT flavor of the runtime.

The baselining is necessary because the reflection stack of NativeAOT doesn't live in CoreLib.

The CannotRemoveBaseTypeOrInteface baselining will go away once #62944 gets fixed. It's one of the "overall goodness" things we can take out of NativeAOT and put it into all runtimes.
@terrajobst terrajobst added code-analyzer Marks an issue that suggests a Roslyn analyzer api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 4, 2022
@terrajobst
Copy link
Member

terrajobst commented Jan 4, 2022

Video

  • Looks good as proposed
  • We should do a pass and find all structs that provide a custom Equals method without implementing IEquatable<T>
  • This should probably be an analyzer
namespace System
{
    public struct RuntimeTypeHandle : IEquatable<RuntimeTypeHandle>
    {
        // bool Equals(RuntimeTypeHandle) already exists
    }

    public struct RuntimeMethodHandle : IEquatable<RuntimeMethodHandle>
    {
        // bool Equals(RuntimeMethodHandle) already exists
    }

    public struct RuntimeFieldHandle : IEquatable<RuntimeFieldHandle>
    {
        // bool Equals(RuntimeFieldHandle) already exists
    }
}

@jkotas
Copy link
Member

jkotas commented Jan 5, 2022

@stephentoub
Copy link
Member

This should probably be an analyzer

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1066 ?

Yes, we need to fix or suppress all of our occurrences and then turn it on:
#56082 (comment)

@MichalStrehovsky
Copy link
Member Author

Fixed in #63358. IEquatable on all the other structs that have Equals should be a different issue, if we want such issue. Looks like one of those issues that are so big that we'll never get to them.

@stephentoub
Copy link
Member

IEquatable on all the other structs that have Equals should be a different issue, if we want such issue. Looks like one of those issues that are so big that we'll never get to them.

I opened #63687 with the list for API review and #63690 with an implementation.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

4 participants