[DependencyInjection] Use '==' instead of 'Equals()` to test service type equality

[benjaminpetit]

Related issue: dotnet/aspnetcore#49485

It seems that scrutor uses a custom implementation of Type: https://github.com/khellang/Scrutor/blob/master/src/Scrutor/DecoratedType.cs#L15

Unfortunately, this type override the Equals() method, making it non "symetrical" with Type:

Type t = typeof(object);
var decoratedType = new DecoratedType(t);
var result1 = t.Equals(decoratedType); // returns true
var result2 = decoratedType.Equals(t); // returns false

Using == to test if two types are the same seems to fix the issue.

[ghost]

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

Issue Details

---

Related issue: dotnet/aspnetcore#49485

It seems that scrutor uses a custom implementation of Type: https://github.com/khellang/Scrutor/blob/master/src/Scrutor/DecoratedType.cs#L15

Unfortunately, this type override the Equals() method, making it non "symetrical" with Type:

Type t = typeof(object);
var decoratedType = new DecoratedType(t);
var result1 = t.Equals(decoratedType); // returns true
var result2 = decoratedType.Equals(t); // returns false

Using == to test if two types are the same seems to fix the issue.

Author:	benjaminpetit
Assignees:	benjaminpetit
Labels:	area-Extensions-DependencyInjection
Milestone:	-

[IDisposable]

Seems like a design flaw on the Scrutor side, honestly

[jkotas]

operator== is overloaded for System.Type and it just forwards the call to Equals unless the type is RuntimeType: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Type.cs#L654-L666

Is it actually fixing anything?

[benjaminpetit]

As long as you are trying to compare RuntimeType against DecoratedType at least.

The comment

// If `left` is a non-runtime type with a weird Equals implementation
// this is where operator `==` would differ from `Equals` call.

seems to indicate that someone else already experienced that...

[steveharter]

Can we add a test for this?

[khellang]

Seems like a design flaw on the Scrutor side, honestly

@IDisposable How come?

[khellang]

Maybe the fix is to compare UnderlyingSystemType instead? I think this might be Best Practice™ anyway? See When is a Type not a Type?

[khellang]

Unfortunately, this type override the Equals() method, making it non "symetrical" with Type

That's the entire reason for the derived class. Scrutor used to do decoration by replacing existing ServiceDescriptor instances with a factory-based ServiceDescriptor that uses ActivatorUtilities to create an instance of the decorator, closing over an instance of the decoratee. The problem with this approach is that the registration of the decoratee is removed from the service collection and subsequently instance lifetime is no longer tracked for disposal by the service provider. This means that decorated instances weren't being disposed.

This was solved by DecoratedType having reference-based equality, effectively allowing you to have multiple instances of the same type in the service collection. When the decorator is resolved, it just "forwards" to the decoratee's registration, allowing the container to track the decoratee instance for disposal.

[benjaminpetit]

Maybe the fix is to compare UnderlyingSystemType instead? I think this might be Best Practice™ anyway? See When is a Type not a Type?

That's what Type.Equals() do. And that's causing a lot of confusion during dictionary lookup/insertion, because it will compare it to the DecoratedType.UnderlyingSystemType that will return the proxied UnderlyingSystemType...

Type t = typeof(object);
var decoratedType = new DecoratedType(t);
var result1 = t.Equals(decoratedType); // returns false
var result2 = decoratedType.Equals(t); // returns false

Is what you want.

[IDisposable]

Seems like a design flaw on the Scrutor side, honestly

@IDisposable How come?

Seems like the wrapper class that Scrutor is generating having only reference-equality is what's wrong here... the class is proxying/wrapping for some other class representing type, if it can't enforce the identity (e.g. always handing out the same instance for the underlying/wrapped type) then it shouldn't be enforcing reference identity.

[steveharter]

A test would be nice, but not blocking.