IDE0028 catches `ICollection<KeyValuePair<TKey, TValue>> = new Dictionary<TKey, TValue>()` as being reduceable

[bzd3y]

IDE0028 catches `ICollection> = new Dictionary()` as being reduceable

from roslyn.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[CyrusNajmabadi]

this is by design as the feature has two options "when_types_loosely_match" (the default) and "when_types_exactly_match". If you want to ensrue that the final value at teh end of hte day has the exact same runtime type, and not just the same static type, set to the latter value and you'll be g2g.

[bzd3y]

Can you please not just instantly close these issues without any discussion? This isn't the first time you have done that.

I know about when_types_loosely_match (see #7257 where I suggested improving it for reasons related to this issue, still open, no responses, but maybe in the wrong place?), and maybe should have mentioned that, but that causes other issues, namely that if you configure that then the rule doesn't catch cases where they only loosely match but are still a valid simplification, e.g. the example I gave above: IList<string> l = new List<string>();.

Again, like with #73787, you are saying this is "by design", but that just means it was designed that way. A flawed design is not really "by design". A design can be fixed or improved, which seems to be the purpose of this issue system.

So here you seem to be saying that this analyzer rule was designed to produce an incorrect fix? The analyzer was deliberately designed to not be smart enough to know that its fix is wrong?

[CyrusNajmabadi]

you configure that then the rule doesn't catch cases where they only loosely match but are still a valid simplification, e.g. the example I gave above: IList l = new List();.

Those types don't loosely match. Those types exactly match (since hte specification says that we must emit Ilist<string> as List<string>. I'll look into whether the analyzer does hte right or wrong thing there.

[CyrusNajmabadi]

So here you seem to be saying that this analyzer rule was designed to produce an incorrect fix? The analyzer was deliberately designed to not be smart enough to know that its fix is wrong?

Correct. When the types loosely match, there is no perfect way to say waht is right/wrong. Some users want the code to be simplified, caring only about the static type of the final location. Some users want the code to not be simplified, with the runtime type beuing exactly preserved. That's what those two options control. It allows users to self-select which group they are in.

[CyrusNajmabadi]

Opened #73884 for the List<T>/IList<T> issue.

[CyrusNajmabadi]

Perhaps somewhat related (via how the collection literal is handled), but maybe a different issue, the following produces a compiler error: IDictionary<string, object> d = []; that says the collection expression can't be used because the type is not constructable. However, as seen above, or with something like IList l = []; there is no error even though neither IList nor ICollection<KeyValuePair<string, object>> are constructible either.

This is because IDictionary is not a supported target type. We're tracking that in the csharplang dictionary-expression's proposal here: https://github.com/dotnet/csharplang/blob/main/proposals/dictionary-expressions.md. May make c#13, or maybe c#14.

[bzd3y]

Okay, maybe the issue you opened (thank you) addresses the problem in that it can be configured away. It definitely seem to make it better. But I think I am still confused about something and/or still would make a case.

So I'm not arguing, I'm just trying to get an understanding and/or possibly better make my point if it is actually valid.

Those types don't loosely match. Those types exactly match (since hte specification says that we must emit Ilist as List. I'll look into whether the analyzer does hte right or wrong thing there.

But they don't match exactly. One is IList<string> and the other is List<string>(). Which types matching are we talking about? There are up to 3 types involved here, declared (the interfaces), assigned (the concrete like List or Dictionary), and then the compiled type that comes into play if the collection expression is used, which seems to basically always be List<T>, at least for interfaces/non-arrays.

So which are the two you are saying match exactly?

Correct. When the types loosely match, there is no perfect way to say waht is right/wrong.

Maybe not always, but in certain cases, right? It chooses List<T> because that is always safe for IList<T> or ICollection<T>. And that's per the spec. That part is fine.

But the right/wrong thing doesn't have to be which type the [] gets compiled to. The right thing to do might be for the analyzer to not flag it.

So for ICollection<KeyValuePair<TKey, TValue>> c = new Dictionary<TKey, TValue>() the perfect thing to do seems to be to NOT flag it and try to simplify it to [].

The reasoning there is you talk about the types matching, but it seems like the most important type is getting left out in the match, the one that the developer explicitly told the compiler that they wanted.

If the compiler can't do that through simplification, then that's perfectly fine. And in that case, don't offer it, right?

Some users want the code to not be simplified, with the runtime type beuing exactly preserved.

Which runtime type? Because I don't see any type being preserved here. The Dictionary got dropped when the developer accepted the offer to simplify the expression - and they might not even be aware of that. It seems reasonable to assume that the compiler saw them requesting Dictionary and is telling them that it can simplify that and that they will get the same thing.

The documentation does say this:

If you use the code fixer in Visual Studio, the change it offers might have different semantics in some cases. For example, int[] x = new int[] { } is replaced with int[] x = [];, which has slightly different semantics—the compiler uses a singleton for x instead of creating a new instance.

But that indicates the difference between a singleton and a new instantiation, not that it will get rid of the type.

That's what those two options control. It allows users to self-select which group they are in.

I understand. I guess my point is that there is maybe a third. Users who only want the compiler to simplify their code if it can/is going to produce more or less identical code.

[CyrusNajmabadi]

But they don't match exactly. One is IList and the other is List()

The 'match exactly' refers to the type of hte collection the compiler actually emits vs what the original source was. In the case of IList<T>, the compiler is guaranteed to still emit List<T> meaning that the original source and the new source exactly match on that type.

It ensures that semantics are preserved for those that care about the true runtime type of the object. 'loose match' means the compiler is allowed to substitute a different runtime type, as long as the code would still be statically safe.

[CyrusNajmabadi]

but it seems like the most important type is getting left out in the match, the one that the developer explicitly told the compiler that they wanted.

Right. But the developer also said: i'm ok with the suggestion if the runtime type would differ, so i'm ok with you suggesting this.

If the developer wants to say "the runtime type is not allowed to change" there is an option for that :)

[CyrusNajmabadi]

I understand. I guess my point is that there is maybe a third. Users who only want the compiler to simplify their code if it can/is going to produce more or less identical code.

That option exists. That is "when_types_exactly_match".

[CyrusNajmabadi]

Which runtime type? Because I don't see any type being preserved here.

Values have a static-type specified in source, and a runtime type that the value will actually have when the code executes. When we see:

T t = new U(); there are two types in play T and U. In this space, many users are ok with changing new U() to compile down to some different type as long as they get a value compatible with T. After all, T is how that value is exposed out to their own code, so they see it as acceptable to have some different runtime type since they still get the interface they expect.

However, some users are not ok with this. They depend truly on that actual U type being the type the value is when the code is truly executing. This is perhaps because they cast back to that type later. Or they depend on different runtime semantics of that type versus the type the compiler would choose instead when writing a collection expr.

THat's why this option exists. Users can say:

1. i only want absolute safety here. The change should only be suggested when teh compiler would emit the same code that i was writing originally.
2. i'm ok with this being possibly unsafe. the choice the compiler makes here is going to actually be fine with me in all cases. i don't actually depend on the semantics exactly needing the type to be the same.

[bzd3y]

Sorry, I made long post but then stepped away and didn't keep up.

This is because IDictionary is not a supported target type. We're tracking that in the csharplang dictionary-expression's proposal here: https://github.com/dotnet/csharplang/blob/main/proposals/dictionary-expressions.md. May make c#13, or maybe c#14.

Ah, okay. This along with the issue you opened seems like it will address the problem completely.

The 'match exactly' refers to the type of hte collection the compiler actually emits vs what the original source was. In the case of IList, the compiler is guaranteed to still emit List meaning that the original source and the new source exactly match on that type.

It ensures that semantics are preserved for those that care about the true runtime type of the object. 'loose match' means the compiler is allowed to substitute a different runtime type, as long as the code would still be statically safe.

Got it. This makes sense. And that is a good point about statically safe, my issue is that it isn't necessarily runtime safe and that would be the case for the situation where this came up for me.

But, like I said, the issue you opened and Dictionary being a valid target type takes care of that.

That option exists. That is "when_types_exactly_match".

Yep, I my confusion was from the analyzer not catching the case of setting an IList to a new List. Because "loosely match" caught the IList/List I thought that was the one for me. But I think the end solution for me is to change my config to when_types_exactly_match.

By the way, and maybe I am missing something again, but the documentation for IDE0028 doesn't seem to mention that that configuration applies to it like it does for IDE0300. But IDE0028 is the rule getting flagged, which probably added to my confusion.

Thanks for taking the time to discuss/explain all this.

[CyrusNajmabadi]

By the way, and maybe I am missing something again, but the documentation for IDE0028 doesn't seem to mention that that configuration applies to it like it does for IDE0300. But IDE0028 is the rule getting flagged, which probably added to my confusion.

Thanks! I'll work with docs to improve thigns here :)

Thanks for taking the time to discuss/explain all this.

Happy to :)