Preview 6 regression with AddRange on ICollection/Collection

[clairernovotny]

When updating the SDK from Preview 5 to the latest Preview 6 nightly (3.0.100-preview6-012131) and recompiling my app, I started getting runtime exceptions in ListCollectionView due to changes in AddRange.

I have an extension method keyed on ICollection<T> called AddRange that accepts an IEnumerable<T>. It loops over the items and calls Add(T item) on the collection. I believe this is a common extension method people have added for convenience.

The trouble is that the new implementation of AddRange on Collection<T> does the add as a bulk insert at the end. This changes the behavior for consumers like the WPF ListCollectionView. That type listens for property changes on ObservableCollection<T>, but it does not support bulk operations, and throws.
https://github.com/dotnet/wpf/blob/master/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Data/ListCollectionView.cs#L2521

The net effect here is that a common helper method that is used in many WPF apps (AddRange to an ObservableCollection<T> no longer binds and the replacement method's behavior has a negative and observable side effect.

The workaround is to create an AddRangeScalar method with the same signature as the original AddRange extension method and ensure that's always called. However, that has a wide impact on existing code and it's really easy for people to miss it and trigger a runtime exception on that code path.

[clairernovotny]

This is the commit that added AddRange to Collection<T> and caused the behavior difference: dotnet/corefx@b705522

[karelz]

I wonder if we should just try to fix up WPF ListCollectionView instead of backing out the change ...

@safern can you please take a look? This may impact our ability to get feedback on WPF apps. At minimum it will be blocker for Preview 7.

cc @danmosemsft @vatsan-madhavan @amit-kabra @fabiant3 @ericstj

[safern]

@safern can you please take a look? This may impact our ability to get feedback on WPF apps. At minimum it will be blocker for Preview 7.

Yes, I assigned to myself and will follow up with WPF folks. We could back out the change before the snap today and then do a deeper investigation and try to put ti back for Preview 7 as another alternative.

[karelz]

Thanks @safern! That may be best option - can you please send red-bang email to WPF folks? (Vatsan, Amit, Fabian above)

cc @wtgodbe - another last minute must-have change in Preview 6.

[robertmclaws]

TBH, when I first proposed the API I was afraid there were going to be farther-reaching changes than people anticipated. A lot of code was based on flawed implementations on how to handle those events (the original implementation of observable incorrectly accounted for the possibility of multiple events to be fired at once, for example).

It's going to have a lot farther downstream changes than just this. My concern would be that if you roll back now and then put it back in for Preview 7, then you'll find other downstream breakages too late in the game for the 3.0 RTM. I realize nobody asked my opinion, but I would think it would be better to document the change now, break people, and then work with the teams to fix the broken downstream stuff for the next preview.

[vatsan-madhavan]

@rladuca fyi.

[karelz]

We should not break WPF apps in Preview 6 - we need feedback. Note that Preview 7 is RC (go-live), so we don't have much runway to break partners and react.

We had a quick call and decided to back out the change from Preview 6 (@safern works on PR) and reopen discussion next week what to do in Preview 7.
If WPF changes are simple, we could consider it still for Preview 7 (pending @danmosemsft approval), otherwise we will have to bump it to next major version -- .NET 5.

[SkyeHoefling]

@karelz and team, I am sorry this was missed during the Pull Request and the months of hard work we all did to get this into tn to CoreFX and NET Standard.

What can I do to help in the meantime so we can get this right for Preview 7. I am worried that we aren't going to be able to get this working and it is going to be excluded when .NET Core 3.0 is Generally Available. I understand this is a convenience API but I am willing to help out since I contributed this change.

Note on the change

The change was committed in many PRs in both CoreFX and CoreCLR. I am not sure if it is required to revert all PRs or just the 1 cited above. Here is my list of PRs attached to the original issue

• Added Range Manipulation APIs to Collection<T> and ObservableCollection<T> coreclr#23018
• Collection<T>: Validate parameters in the public methods coreclr#23166
• dotnet/corefx#37552 (PR open to revert in Revert "Added Range Manipulation APIs to Collection<T> and ObservableCollection<T> (#35772)" corefx#38115)
• Add bulk operations to Collection<T> standard#1175 (already reverted in merged pr Investigate IO differences in sanitizing characters when creating directories #14379)

What can I do to as far as testing scenarios to try and get the issues resolved for Preview 7?

[safern]

Thanks @ahoefling -- this is not your fault or anyone else's this was just bad timing and unfortunately we don't have time now to do the proper fix before Preview 6. However, be sure that I will follow up with the WPF guys to take action so that we can try and get this into Preview 7 and if we can sort it out, the way that we will do it is by Reverting my Revert 😄.

Also, we don't need to revert anything in coreclr as of now, since that is just implementation. As long as we don't expose this APIs in the reference assembly we're fine, so that is why just reverting corefx is enough.

[SkyeHoefling]

@safern thanks for the explanation of how we will proceed on this as it answers a lot of the questions I had 👍.

If there are any actionable items I can help out with such as adding better test coverage in CoreFX, please let me know as I am willing to jump in to get this released in 3.0

[karelz]

@ahoefling if you (or anyone) is up to it -- debugging through WPF, figuring out what went wrong there and suggesting solutions in WPF code would be very appreciated.
It would save mainly WPF team's time ... which is precious as they have plenty of things to finish for .NET Core 3.0, more than CoreFX team has.

And thanks a lot for your offer! We really appreciate it! As @safern mentioned, this is NOT your fault at all ... we all missed it and the key is to react now :)

[karelz]

I realized I said it only in email -- but HUGE kudos to @onovotny for saving us from shipping these bits with such bad WPF experience. Thank you!
You're savior of the day (as usual) ;) Thanks!!!

Next time we meet, I am buying ;)

[robertmclaws]

@onovotny Can you post a WPF app that exhibits the behavior somewhere so I can debug through it? Maybe I can track down the issue and fix it in the ListCollectionView.

Thanks!

[clairernovotny]

NuGet Package Explorer shows it in many places. Clone master and run with preview6 (before this fix). First place I noticed is in the “open from feed” dialog where there was an error message instead of displaying the feed info. There are other places too.

[SkyeHoefling]

@onovotny thanks for sharing information about the project. I had a similar question as I plan to dig into this as well. @robertmclaws we should try and compare notes sometime next week on what we find.

Here are some useful links:

• NuGet Package Explorer
• PR that updates it to Preview 6

[karelz]

You don't need to build master - just use daily build from yesterday and you will get the "bad" bits end-to-end via SDK download.