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

Order of implicit MapOf.New constructors leads to inconsistent behaviour #521

Open
fneu opened this issue Aug 25, 2022 · 3 comments
Open
Labels

Comments

@fneu
Copy link

fneu commented Aug 25, 2022

Expected Behavior

Inferred type of generic maps does not depend on the number of Kvps passed to MapOf.New() constructors

Actual Behavior

This map, like any map with an odd number of Kvp arguments:

var map = 
    MapOf.New(
        KvpOf.New(0, "zero"),
        KvpOf.New(1, "one"),
        KvpOf.New(2, "two")
);

uses this constructor:

/// <summary>
/// A map from the given KeyValuePairs
/// </summary>
public static IDictionary<Key, Value> New<Key, Value>(KeyValuePair<Key, Value> item, params KeyValuePair<Key, Value>[] more)
=> new MapOf<Key, Value>(item, more);

and is consequently of type Yaapii.Atoms.Map.MapOf<int, string>.


In contrast, this map, like any map with an even number of Kvp arguments:

var map = 
    MapOf.New(
        KvpOf.New(0, "zero"),
        KvpOf.New(1, "one"),
        KvpOf.New(2, "two"),
        KvpOf.New(3, "three")
);

appears to use this constructor (or an equivalent for the number of arguments):

/// <summary>
/// A map from the given keys and values.
/// </summary>
public static IDictionary<Key, Value> New<Key, Value>(
Key key1, Value value1,
Key key2, Value value2
)
=> new MapOf<Key, Value>(
key1, value1,
key2, value2
);

and is consequently of type Yaapii.Atoms.Map.MapOf<Yaapii.Atoms.IKvp<int, string>, Yaapii.Atoms.IKvp<int, string>>

Mention any other details that might be useful

present in Yaapii.Atoms 2.7.2

@fneu fneu added the Bug label Aug 25, 2022
@Meerownymous
Copy link
Collaborator

Meerownymous commented Nov 10, 2023

@fneu I believe there must be a misobservation of some kind.

var map = 
    MapOf.New(
        KvpOf.New(0, "zero"),
        KvpOf.New(1, "one"),
        KvpOf.New(2, "two")
);

It cannot be the case that this code points to the ctor

/// <summary> 
 /// A map from the given KeyValuePairs 
 /// </summary> 
 public static IDictionary<Key, Value> New<Key, Value>(KeyValuePair<Key, Value> item, params KeyValuePair<Key, Value>[] more) 
     => new MapOf<Key, Value>(item, more); 

KvpOf is of type IKvp, while the constructor you mention accepts KeyValuePair<,>. There is no implicit conversion present for IKvp<,> - so I guess there must be some confusion about the types. How did you find the mentioned constructor? When I do a "go to reference" for your Map.New code, it takes me to:

Screenshot 2023-11-10 at 16 06 50

@fneu
Copy link
Author

fneu commented Nov 13, 2023

@Meerownymous hope you're doing well :) It seems that you're right and it uses IKvp. However, the problem is still the same. The problematic case is the one with 4 arguments:

var map = 
    MapOf.New(
        KvpOf.New(0, "zero"),
        KvpOf.New(1, "one"),
        KvpOf.New(2, "two"),
        KvpOf.New(3, "three")
    );
var val = map[0];

image
image

Meanwhile, the case with 3 arguments behaves as expected:

image

@Meerownymous
Copy link
Collaborator

Meerownymous commented Nov 13, 2023

Yes thanks, all good. Hope you too.

Now I could reproduce.
So what it should take, is the params method - New<>(IKvp item, params IKvp[]) but it does not.

C# defines rules about how to decide for a method if it has ambiguity, so it may be solvable by studying them. I found this snippet:

In case the parameter type sequences {P1, P2, …, PN} and {Q1, Q2, …, QN} are equivalent > (i.e. each Pi has an identity conversion to the corresponding Qi), the following tie-breaking rules are applied, in order, to determine the better function member.

If MP is a non-generic method and MQ is a generic method, then MP is better than MQ.
Otherwise, if MP is applicable in its normal form and MQ has a params array and is applicable only in its expanded form, then MP is better than MQ.
Otherwise, if MP has more declared parameters than MQ, then MP is better than MQ. This can occur if both methods have params arrays and are applicable only in their expanded forms.

So if I git it right - one solution is to change KvpOf.New to return KvpOf instead of IKvp, and offer a Map.New method accepting KvpOf (not IKvp) - with matching parameter count for every Map.New method that accepts alternating Key, Value paremeters.
However, it is then only solved for KvpOf, not for other concrete implementations of IKvp.

Other possibility is to remove the params MapOf.New constructor.

Workaround in this version is to explicitely define the generics or not use Kvps but the shorthand constructors with the alternating Key, Value parameters. You might find other ways too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants