HotReload crashes with "An item with the same key has already been added"

[Youssef1313]

I don't have a minimal repro yet, but:

roslyn/src/Compilers/Core/Portable/Emit/EditAndContinue/SymbolChanges.cs

Line 497 in 3acd33d

changesBuilder.Add(member, change);

At this line, a duplicate named type symbol is added.

The case involves nested classes, so I'm wondering if it's related to this:

roslyn/src/Compilers/Core/Portable/Emit/EditAndContinue/SymbolChanges.cs

Line 467 in 3acd33d

AddContainingTypesAndNamespaces(changesBuilder, newContainingSymbol);

where the deletion of the method will add containing types, then we get another change for the same type.

[davidwengier]

@Youssef1313 next time you hit it, can you capture a dump and share it (privately on Discord is fine), or even just switch your debug frame to the CalculateChanges method and see what is in the edits parameter and share that?

I tried to create a couple of failing tests in davidwengier@f15cf98 but sadly they both pass. Ordering of the changes in the file don't seem to matter, we still produce the right edits, and emit works fine. I think you must be hitting a specific case where we don't produce edits that we expect in emit.

[nickrandolph]

Hey @davidwengier can you point me at how I create the dump you requested?

[nickrandolph]

Here's a screenshot showing the edits parameter:

[nickrandolph]

The issue seems to be that when calculting changes for MainPage_Bindings (which is a nested class within MainPage), the MainPage type is added to the dictionary of changed types. Then on the next iterationg, when the MainPage edit is being processed, it throws the exception because MainPage is already in the changes dictionary

[davidwengier]

Thanks @nickrandolph! I see you have Replace edits there, my tests don't cover that scenario so will see if that is the trick. That's hopefully enough for me to repro the issue.

If you've caught the exception under the debugger in VS, the easiest way to create a dump is from the Debug menu, "Save Dump As...".

[Youssef1313]

@davidwengier fwiw, I'm suspecting the dictionary is trying to use reference equality. It's created using the parameterless constructor where it should have been supplied SymbolEqualityComparer.Default. Just a guess

Edit: Wrong guess, still a good idea to use SymbolEqualityComparer.Default though.

---

Edit 2: We first add MainPage_Bindings (and its containing types including MainPage)

Then on the next edit which is replacing MainPage, we add it without checking whether it's already there.

The fix is not a simple ContainsKey, it should be something like if (TryGetValue(symbol, our var symbolChange) && symbolChange == ContainsChanges) { replace the symbolChange, we got a better reason for this symbol }

[nickrandolph]

Thanks @nickrandolph! I see you have Replace edits there, my tests don't cover that scenario so will see if that is the trick. That's hopefully enough for me to repro the issue.

If you've caught the exception under the debugger in VS, the easiest way to create a dump is from the Debug menu, "Save Dump As...".

@Youssef1313 will share the dump with you. Let me know if you need anthing more

[davidwengier]

The issue is that [CreateNewOnMetadataUpdate] is being used on a nested type and its containing type. Suspect this was just not part of any scenarios of the original design, so missed in testing.

FYI @tmat in case you had any thoughts about appropriate usage here :)

[nickrandolph]

I can try again but I think the issue exists even if we remove the attribute off the nested type

[davidwengier]

The order of edits produced is also vital. Even with the attribute on both types, if the compiler sees the edit for the containing type first, then things work.

But the good news is I got a repro in a failing test

[nickrandolph]

Removing the [CreateNewOnMetadataUpdate] attribute from the nested type gives the following changes and the same exception raised. I think this issue is more due to the ordering of changes, rather than the inclusion of the attribute on the nested type

The symbols are in the same order: MainPage_Bindings, followed by MainPage

[davidwengier]

PR opened. FYI, it's a long weeked in the US so I've no idea if anyone will be around to look at it for a couple of days.

[nickrandolph]

@davidwengier is there a way that I can test this fix using the packages generated by the CI? I've tried referencing the packages from my test app but it seems that VS just defaults to using the version packaged with VS.

[davidwengier]

You should be able to clone my fork, switch to the branch, and run ./build.cmd -restore -deployExtensions -launch and that should run an instance of VS with the changes.