Closed virzak closed 7 months ago
@virzak, thanks for reporting this issue.
In 17.10 and later, conversion of a collection expression to a type that implements IEnumerable
and does not have a CollectionBuilderAttribute
requires that the type has an Add
method that takes an argument of the iteration type of the type - see breaking change.
For this particular example, ElementCollection
could implement IEnumerable<Element>
or an Add(object)
overload could be added.
In that case should IDE0028 shouldn't show up for cases like this.
Also shows up in SharpLab:
Here is a common scenario for context. https://github.com/microsoft/perfview/blob/f312aef7c4e415557899bf1303c98b7d48b05612/src/PerfView/GuiUtilities/TextEditor/TextEditorControl.xaml.cs#L206-L207
Also why doesn't dotnet build
throw that error?
🚀 dotnet build
MSBuild version 17.10.0-preview-24101-01+07fd5d51f for .NET
Restore complete (0.6s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
CS1503-FalsePositive succeeded (6.2s) → bin\Debug\net8.0\CS1503-FalsePositive.dll
Build succeeded in 8.0s
@jcouv I'm also seeing this behavior, and it's a regression from 17.9. The WPF class InputGestureCollection
has an Add
method, and it worked with collection initializers until I upgraded to 17.10p1. dotnet build
works, even tried on the .NET 9.0.100-preview1 SDK.
@MadsTorgersen @KathleenDollard real world cases where this caused breaking changes. These examples seem very reasonable to be. They are places where a collection initializer would be legal. The original rules for collection expressions thus also were legal. But our recent restriction now breaks things.
I don't love that we're breaking code, and that collection expressions don't live up to their original promise (a unified way to write collections that can handle the prior ways we had in the language).
@aelij, this is the expected behavior in 17.10 with the breaking change.
InputGestureCollection
implements IEnumerable
and has an Add(InputGesture)
method, but the collection expression conversion now requires an accessible Add(object)
since the iteration type of the collection type is object
.
dotnet build
works, even tried on the .NET 9.0.100-preview1 SDK.
It looks like the .NET 9.0.100-preview.1 SDK is using an earlier commit of Roslyn, and does not include this change.
Started an internal discussion on this topic. Will escalate this to the LDM asap.
@cston The problem here is that that strict rules cuts out many normal collection types that were not generic. They are IEnumerable (The normal iteration type of the time), but have strongly typed Add methods so people wouldn't add inappropriate elements to them. These types worked fine with collection-initializers, and the initial shipping form of collection-expressions.
Unfortunately, requiring that Add
take an object here is an actual negative as it would make these types not strongly typed wrt to being able to add to them.
In the internal email i've proposed that if the type implements IEnumerable
(but not IEnumerable<T>
), then we relax things here, and we allow the Add
method to not have to strictly match the element type. It just needs to be able to convert to it.
Since nongeneric collections often indicated element type through the indexer property type, that would be the moral equivalent of determing the T
in IEnumerable<T>
to use to require Add(T)
to exist.
Since nongeneric collections often indicated element type through the indexer property type, that would be the moral equivalent of determing the
T
inIEnumerable<T>
to use to requireAdd(T)
to exist.
@jnm2 I like that idea. I Will raise it internally!
@jnm2 I've sent out that idea. Note: i prefer your idea a lot as:
A list_pattern is compatible with any type that is countable as well as indexable — it has an accessible indexer that takes an Index as an argument or otherwise an accessible indexer with a single int parameter. If both indexers are present, the former is preferred.
Add
. Specifically the Add
method can often take a string
instead of a strongly typed UI element, leading to an open question of which parameter type applies. However, with the indexer, we are starting from looking just at the indexer that takes an int (the indexable requirement), and looking just at the indexer's type. --
The open question, in my mind, is whether we are going the route of saying:
object
, but you are allowed to call .Add
methods that take a more specific type. OR:T this[int i] { get; }
indexer.In other words, both approaches would allow this to work. But each would be saying something quite different about the element type. Personally, i prefer '2' as it really seems to convey the type information that these types were trying to convey in the pre-generics world.
Tagging @RikkiGibson as well.
And @333fred :)
@cston Thanks for the explanation. BTW that document is not very visible. It would be helpful to publish such breaking changes under https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0
@aelij FYI, a link is being added in https://github.com/dotnet/docs/pull/39681 Thanks for the suggestion
This used to work in versions prior to 17.10. Might be related to https://github.com/dotnet/sdk/issues/38777
Version Used: 17.10.0-preview-24101-01+07fd5d51f
Steps to Reproduce:
This code compiles fine with
dotnet build
, but notmsbuild
Diagnostic Id: CS1503
Expected Behavior:
Actual Behavior: