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

The call is ambiguous when using collections literals with interpolated strings #7651

Open
xamir82 opened this issue Oct 4, 2023 · 38 comments
Milestone

Comments

@xamir82
Copy link

xamir82 commented Oct 4, 2023

The following code doesn't compile:

MyClass.MyMethod([$"one", $"two"]);

public static class MyClass
{
    public static void MyMethod(IEnumerable<string> _)
    {

    }
    public static void MyMethod(IEnumerable<IFormattable> _)
    {

    }
}

If we had used new[] { $"one", $"two" } it would work. I would expect collection literals to work the same way as new[] { } here. Let me emphasize that this only errs when you're using interpolated strings, it's kind of weird.

Sharplab: https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQAYACdbAFgG4CCBZATwGEAbCAZ3YDpG6BTAC4ALAPYIAFAG0AJACIRMPrLgk5AgO4jZAXQCUNfAXQBmMtgBsZTCUasO7AgG8CJV2VPlL6CjYb9hYuLkxgA8AJIAYiIATpACAhDALHwAfCQA+roubs6G+G4kAL60+W4mZl4+vIKiEsEh5ERpmdmuua1FBIVAA

@CyrusNajmabadi
Copy link
Member

@cston @333fred @jnm2 @RikkiGibson interesting case.

What's happening here, i imagine, is that both overloads are now applicable since we are taking the target type into account.

However, because there's no relationship between string and IFormattable, we can't gauge which is 'better'.

To me, this is actually a fairly good place to be. The only reason this worked in the past is because the lack of target-typing meant that new [] { ... } was always a string[], and that was only convertible to one of these types successfully.

I'm personally ok with this not working (under the auspices of: i have no idea what would be called in this case), but i wanted to get your thoughts on this.

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

I'm personally ok with this not working

But this particular combination of overloads is not uncommon, and therefore not being able to use collection expressions in this context would be a bummer, and pretty surprising. One example is the popular CliWrap library, where the args.Add method has these exact overloads (which is actually how I stumbled into this); that means the following won't be possible, which is weird:

Cli.Wrap("command").WithArguments(args =>
{
    args.Add(["--some-flag", $"some-value-{whatever}"]); // ERROR
    args.Add(new[] { "--some-flag", $"some-value-{whatever}" }); // But this worked
});

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

This already works by the way:

MyClass.Foo($"one");

public static class MyClass
{
    public static void Foo(IFormattable _)
    {

    }

    public static void Foo(string _)
    {

    }
}

It picks the string overload, as expected. So, there's currently an asymmetry; why couldn't collection expressions do the same?! Why not follow the same overloading preference rules (sorry I don't know the actual technical term)?

@CyrusNajmabadi
Copy link
Member

It picks the string overload, as expected. So, there's currently an asymmetry; why couldn't collection expressions do the same?!

It could potentially could. If there's a betterness rule between interpolated strings and strings vs iformattables, then that possibly should apply in terms of determining betterness here. I'm curious what @333fred thinks.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Oct 4, 2023

using System;

class Program
{
    void M0(IFormattable format) { }
    void M0(string format) { }

    void M1() { M0($"a"); } // ok
    
    
    void M2(IFormattable[] format) { }
    void M2(string[] format) { }

    void M3() { M2([$"a"]); } // currently error CS0121: The call is ambiguous between the following methods or properties: 'Program.M2(IFormattable[])' and 'Program.M2(string[])'

}

In the above sample I basically expect the call to M2 to work because the call to M0 works. SharpLab. Ideally we could adjust the feature design if needed to ensure that is the case.

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

@RikkiGibson Yeah exactly, that's what I was saying

@333fred
Copy link
Member

333fred commented Oct 4, 2023

This is something that I somewhat expect we will need natural typing to solve. new[] { $"" } works here for a simple reason: it isn't target-typed. If you look at the SharpLab when removing the IEnumerable<string> overload for that case, you'll find that new[] { $"" } cannot be converted to IEnumerable<IFormattable> or IEnumerable<FormattableString>. The natural type of the array is string[], and that is simply not convertible to IEnumerable<IFormattable>. Overload resolution doesn't even come into play: there's only one applicable M in this example for new[] { $"" }.

[$""], on the other hand, is target-typed. You can pass that to both M(IEnumerable<string>), and M(IEnumerable<IFormattable>). Overload resolution therefore needs to come into play, and you cannot convert IEnumerable<IFormattable> to IEnumerable<string> or vice versa. Therefore, it's ambiguous. When collection expressions have a natural type, we may be able to then say, in the face of an ambiguous overload, if one is a natural type match, that should win out.


Looking at the M0 example Rikki posted, there's a betterness rule that applies here: which conversion is better. $"a" (which I will refer to as expression E) is a string. It has an identity conversion. E to IFormattable is an implicit interpolated string conversion. Because the type of E is string, that is the better conversion, so it wins in betterness. There is no nested conversion comparisons in betterness there; the algorithm considers top-level betterness only.


To sum this up, this is (imo) expected behavior, and will need to wait until C# 13 to be addressed.

@CyrusNajmabadi
Copy link
Member

@333fred that makes perfect sense to me. We'll put this on the list of things to try to ensure works in C# 13.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Oct 4, 2023

Triaging to 17.9 as we are no longer using Compiler.Next etc milestones. @jaredpar feel free to adjust if this milestone doesn't look right to you.

@333fred
Copy link
Member

333fred commented Oct 4, 2023

@RikkiGibson I don't think 17.9 is right. I don't expect that we can address this in C# 12.

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

When collection expressions have a natural type, we may be able to then say, in the face of an ambiguous overload, if one is a natural type match, that should win out.

Isn't the natural type thing related to the "wrapping" collection type? e.g. List<string> vs string[] etc.? I'm struggling to see how that would solve this problem.

@333fred
Copy link
Member

333fred commented Oct 4, 2023

Isn't the natural type related to the collection type? e.g. List vs string[] etc.? How would that solve this problem?

Yes, it is, and that's exactly why it would solve this case. List<string> is convertible to IEnumerable<string>, but not to IEnumerable<IFormattable>, so the former would win. Now, if the answer you were hoping for was that it would pick the IEnumerable<IFormattable> overload... well, that may need more specific spec changes to enable.

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

Yes, it is, and that's exactly why it would solve this case. List is convertible to IEnumerable, but not to IEnumerable, so the former would win. Now, if the answer you were hoping for was that it would pick the IEnumerable overload... well, that may need more specific spec changes to enable.

@333fred No I was hoping for IEnumerable<string> so that's fine, I just want it to behave the same way as the singular string vs IFormattable overloads.

But let's say you decide that the natural type is List<T>, what would then happen if you use a collection expression where there are two overloads that receive string[] and IFormattable[], for example? Would the taget-typed algorithm kick in again in that case, leading to the same issue?

@333fred
Copy link
Member

333fred commented Oct 4, 2023

But what happens if you decide that the natural type is List and the overloads receive string[] and IFormattable[]? Would the taget-typed algorithm kick in again in that case, leading to the same issue?

Potentially, yes. I do think the working group needs to consider this case @CyrusNajmabadi @jnm2 @captainsafia @cston. I don't have any immediate ideas for how best to solve it, but I do think that we need to. The closest analogy I can find here is the tuple case, which does work:

using System;

MyClass.MyMethod(($"one", $"two")); // Calls the string tuple overload

public static class MyClass
{
    public static void MyMethod((string, string) _)
    {

    }
    public static void MyMethod((IFormattable, IFormattable) _)
    {

    }
}

Given that we've previously said we want to consider the tuple behavior the model for how we want collection expressions to work, it does feel like we're missing a rule somewhere here.

@jnm2
Copy link
Contributor

jnm2 commented Oct 4, 2023

The tuple case was exactly what I was going to suggest we follow. Can we glean rules straight across perhaps?

@xamir82
Copy link
Author

xamir82 commented Oct 4, 2023

Potentially, yes.

Hmm okay that certainly doesn't sound like the right solution then.

Given that we've previously said we want to consider the tuple behavior the model for how we want collection expressions to work

That's the right path IMO, and again, I would argue this should really be done in v12. It's the type of thing one would very much intuitively expect to work and would be surprised to see fail.

@RikkiGibson
Copy link
Contributor

@RikkiGibson I don't think 17.9 is right. I don't expect that we can address this in C# 12.

No worries, feel free to re-triage as appropriate.

I think the issue with doing it for 12 is time. There's so much we're trying to get in for this feature in 12 right now with a rapidly shrinking window of opportunity to do so.

@aradalvand
Copy link
Contributor

aradalvand commented Oct 4, 2023

I obviously have no clue about the inner workings of the compiler but given that the logic for this has already been implemented for tuples as mentioned earlier, it should not be too time-consuming to just adapt it to collection expressions before the C# 12 release, but correct me if I'm wrong.

I agree that the current behavior is definitely surprising.

@333fred
Copy link
Member

333fred commented Oct 4, 2023

it should not be too time-consuming to just adapt it to collection expressions before the C# 12 release

This is never a safe assumption to make 😆

@aradalvand
Copy link
Contributor

This is never a safe assumption to make 😆

Fair enough 😬😅 But hopefully you give it a try.

@333fred
Copy link
Member

333fred commented Oct 4, 2023

But hopefully you give it a try.

It isn't going to happen. The cutoff date for such changes is basically today. It will be a post-release thing.

@xamir82
Copy link
Author

xamir82 commented Oct 5, 2023

It will be a post-release thing.

Does that mean we need to wait until C# 13 or will this fix make it to the compiler before that?

@CyrusNajmabadi
Copy link
Member

Given that we've previously said we want to consider the tuple behavior the model for how we want collection expressions to work, it does feel like we're missing a rule somewhere here.

@333fred @cston could you look into why this works for tuples but not collections? That does feel strange. Could it just be an implementation bug where were doing something for tuples, and forgetting to do the same here?

@CyrusNajmabadi
Copy link
Member

@333fred @cston . Something else surprising:

MyClass.MyMethod($"one", $"two");

public static class MyClass
{
    public static void MyMethod(params string[] _)
    {

    }
    public static void MyMethod(params IFormattable[] _)
    {

    }
}

Note the use of 'params' and no collection-expression. Here we do expand things out, and find that the string overload is the preferred one. This also breaks the intuition that a params-arrays and a collection-expr are nearly the same (except for brackets).

I'm guessing/hoping, that there's just somethgin small being missed here. Maybe not calling through a common 'betterness' helper versus just writing the rules out explicitly?

@xamir82
Copy link
Author

xamir82 commented Oct 6, 2023

@333fred @cston Could you please respond to @CyrusNajmabadi's comment?

@CyrusNajmabadi
Copy link
Member

@xamir82 Please give people time to respond. Everyone has enormously full plates right now and are working on many things at the same time. We care about this issue and will respond when there is time. Thanks! :)

@cston
Copy link
Member

cston commented Oct 6, 2023

For overload resolution, better conversion from expression prefers the type where the expression is an exactly-matching-expression.

For the tuple example, the expression ($"one", $"two") has a natural type of (string, string) so that expression exactly matches one of the two overloads, which is preferred.

Note however, the tuple ($"one", null) does not have a natural type, so that case is ambiguous - see sharplab.io.

For collection expressions, perhaps better conversion from expression should recurse into the collection expression and check if the elements exactly match the element type of the target type. If we check elements, we'd need to decide whether elements without types, such as null in [$"one", null] are considered.

@CyrusNajmabadi
Copy link
Member

@cston We probably need to do this. With tuples, it's less of an issue as there's only 'one possible tuple' you can ever get. So the dependence on natural type is ok. But with collection expressions our natural type will be something like List<T>. But that won't work properly for all the different collection types a user might have. for example, we'd want consistent behavior if you had:

public static void MyMethod(IEnumerable<string> _)
public static void MyMethod(IEnumerable<IFormattable> _)

// or

public static void MyMethod(string[] _)
public static void MyMethod(IFormattable[] _)

// or

public static void MyMethod(Span<string> _)
public static void MyMethod(Span<IFormattable> _)

etc. :)

@jaredpar
Copy link
Member

jaredpar commented Nov 1, 2023

@cston

For the tuple example, the expression ($"one", $"two") has a natural type of (string, string) so that expression exactly matches one of the two overloads, which is preferred.

My take away from this is that fixing this requires us to have natural type support in collection expressions. Is that correct?

@aradalvand
Copy link
Contributor

@jaredpar I think this was already discussed, and the conclusion was that no, even with a natural type, this would still be problematic.

@cston
Copy link
Member

cston commented Nov 2, 2023

My take away from this is that fixing this requires us to have natural type support in collection expressions. Is that correct?

If we use natural type, we'd probably want a natural element type only, to avoid ambiguities when the overloads use other collection types.

@jaredpar
Copy link
Member

jaredpar commented Nov 2, 2023

Okay given this is natural type related that means it's not in scope for C# 12. This would be part of the C# 13 design of collection expressions improvements. It's effectively "by design" for the current version. As such going to close this issue out.

Will address it when we approach the C# 13 design.

@jaredpar jaredpar closed this as completed Nov 2, 2023
@xamir82
Copy link
Author

xamir82 commented Nov 2, 2023

@jaredpar But why would you close the issue?

@jaredpar
Copy link
Member

jaredpar commented Nov 2, 2023

@xamir82 generally try to keep issues in this repo (the compiler repo) for tracking bugs in the compiler implementation. This instead effectively a request to change the design of the feature. It's a better discussion for csharplang.

I'm happy to transfer the issue over there if that's what people want.

@jnm2
Copy link
Contributor

jnm2 commented Nov 3, 2023

I like the idea of transfer.

@xamir82
Copy link
Author

xamir82 commented Nov 3, 2023

This instead effectively a request to change the design of the feature. It's a better discussion for csharplang.

@jaredpar I don't think so, I don't really understand how you drew that conclusion, this is very much an implementation bug in my view.

But either way, doesn't it make sense to keep the issue open just to track this particular problem so that it's not missed in C# 13?

@jnm2
Copy link
Contributor

jnm2 commented Nov 3, 2023

It's not an implementation bug when it's the spec that would need design work. Spec bugs are discussed at csharplang, compiler-not-matching-spec bugs are discussed at roslyn.

@jaredpar jaredpar reopened this Nov 3, 2023
@jaredpar jaredpar transferred this issue from dotnet/roslyn Nov 3, 2023
@Ai-N3rd
Copy link
Contributor

Ai-N3rd commented Feb 22, 2024

I think we should stick to how other features in the language handle this, for simplicity's and consistency's sake.

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

No branches or pull requests

10 participants