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

Overload resolution of virtual methods is overly restrictive #1502

Closed
glopesdev opened this issue Jul 31, 2023 · 2 comments · Fixed by #1512
Closed

Overload resolution of virtual methods is overly restrictive #1502

glopesdev opened this issue Jul 31, 2023 · 2 comments · Fixed by #1512
Labels
bug Something isn't working critical This is a blocking issue affecting all users
Milestone

Comments

@glopesdev
Copy link
Member

glopesdev commented Jul 31, 2023

Following #1452 the following workflow will not compile with a "method overload resolution is ambiguous" error:

Workflow
<?xml version="1.0" encoding="utf-8"?>
<WorkflowBuilder Version="2.7.3"
                 xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xmlns:rx="clr-namespace:Bonsai.Reactive;assembly=Bonsai.Core"
                 xmlns:sys="clr-namespace:System;assembly=mscorlib"
                 xmlns:al="clr-namespace:Bonsai.Audio;assembly=Bonsai.Audio"
                 xmlns="https://bonsai-rx.org/2018/workflow">
  <Workflow>
    <Nodes>
      <Expression xsi:type="rx:PublishSubject" TypeArguments="sys:Object" />
      <Expression xsi:type="Combinator">
        <Combinator xsi:type="al:StopSource" />
      </Expression>
    </Nodes>
    <Edges>
      <Edge From="0" To="1" Label="Source1" />
    </Edges>
  </Workflow>
</WorkflowBuilder>

This happens because of the specific overloads declared in UpdateAudioState sink combined with the current overload resolution strategy where the compiler will accept matches to overloads where input types can be coerced:

// matches any input but is declared in base type, so if any other overload matches this will be excluded
public override IObservable<TSource> Process<TSource>(IObservable<TSource> source)

// matches IObservable<object> because there is an explicit conversion from object to AudioSource
public IObservable<AudioSource> Process(IObservable<AudioSource> source)

// matches IObservable<object> because there is an explicit conversion from object to AudioSource[]
public IObservable<AudioSource[]> Process(IObservable<AudioSource[]> source)

The base generic method is thus excluded and, since both of the other two matching overloads are supported on an explicit user conversion from the object type (hence leaving no preferential conversion that can be used as tie-break), overload resolution is left ambiguous.

The above example illustrates that signature resolution matching rules in the current compiler are too permissive. Matching signatures from more derived types should indeed exclude matching base type signatures, but this should not work for overloads which are matched based only on explicit conversions, as this violates C# overload resolution rules.

In fact, even implicit conversions between types are probably too permissive. Likely the only implicit conversion rules that should be allowed here are the rules for variance in generic interfaces, which allow generic interfaces with more derived type parameters to match against generic interfaces with a less derived type. Variance in generic interfaces is only supported for reference types, so conversions between value type sequences should not be considered for the purposes of excluding virtual overloads in combinators.

IGrouping<string, object> a = default;
IGrouping<string, int> b = default;
IGrouping<object, object> c = a; // accepted since IGrouping is covariant on TKey, TValue
IGroupgin<object, object> c = b; // rejected because b signature contains a value type

Reference:

@glopesdev glopesdev added the bug Something isn't working label Jul 31, 2023
glopesdev added a commit to glopesdev/bonsai that referenced this issue Jul 31, 2023
@glopesdev glopesdev added the critical This is a blocking issue affecting all users label Aug 4, 2023
@glopesdev
Copy link
Member Author

This is apparently affecting overload resolution in multiple operators, even when the input type is exactly identical to the overriding overload, e.g. using the operator Floor of the Numerics package:

This operator has a double overload, but the current overload resolution prefers decimal in this case.

glopesdev added a commit to glopesdev/bonsai that referenced this issue Aug 4, 2023
@glopesdev
Copy link
Member Author

glopesdev commented Aug 5, 2023

We need to bear in mind that implicit conversions need to be accounted for. In the C# standard they are sufficient to match a derived overload against arguments, and thus supersede the override, even if override is exact type match.

Example with implicit conversion:

class C
{
    public virtual float P(float v) => v;
}

class D : C
{
    public override float P(float v) => v;
    public double P(double v) => v;
}

var d = new D();
// the following call will use the double overload in the derived class
// even though override is exact type match with float type!
// this is because float has implicit conversion to double
d.P(5F);

Counterposed example with explicit conversion:

class E
{
    public virtual float P(float v) => v;
}

class F : E
{
    public override float P(float v) => v;
    public decimal P(decimal v) => v;
}

var f = new F();
// the following call will match the float overload in the derived class
// because there is no implicit conversion to decimal (although there is an explicit one!)
f.P(5F);

Note

P.S.: Actually this argument does not apply for the most part since all workflow method signatures are generic type interfaces (i.e. IObservable<T>) which means no implicit or explicit conversions are defined other than covariance / contravariance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working critical This is a blocking issue affecting all users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant