[EnC] Upgrade change types for containing types

[davidwengier]

Fixes #68335

Minimal repo:

A.cs:

internal partial class C
{
    public void M()
    {
        Console.WriteLine("C.M");
    }
}

B.cs:

[CreateNewOnMetadataUpdate]
internal partial class C
{
    public class A
    {
        public void M()
        {
            Console.WriteLine("C.A.M25");
        }
    }
}

[davidwengier]

@tmat PTAL

[Youssef1313]

LGTM 🎉

[Youssef1313]

AddContainingTypesAndNamespaces will add properties/methods with SymbolChange.Updated

Can edits have one edit with some symbol contained within a property (with any level of nested), and another edit with the property itself?

If this can happen, then AddContainingTypesAndNamespaces would add the property with SymbolChange.Updated for the first edit, then when processing the second edit for the property itself, we will try to add it again.

[davidwengier]

It shouldn't be possible, but then again this bug shouldn't have been possible either. In general I would say that this area of the compiler is full of nastiness if you stray from the happy path, and EnC is no exception. This code is written assuming the edits will be of a certain known form. In fact, this bug could probably be fixed equally by changes to the AbstractEditAndContinueAnalyzer where it creates the edits, to ensure it checks for containing types in the Replace scenario, or in EditSession where it merges the semantic edits from all documents, to ensure a predictable order. I chose this fix because it seemed the easiest, but also deliberately left the call to .Add so that its not masking any future odd scenarios.

We'll have to wait and see where Tomas thinks that was the right call :)

[tmat]

Should we assert that member is a INamedTypeSymbol? I think this only happens for nested types, right?

We don't have great coverage for nested combo updates in the IDE layer, we should add the following tests:

        [Fact]
        public void NestedType_Replace_WithUpdateInNestedType_Partial_DiffertDocument()
        {
            var srcA1 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]partial class C { int N() => 1; }";
            var srcB1 = "partial class C { class D { int M() => 1; } }";
            var srcA2 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]partial class C { int N() => 2; }";
            var srcB2 = "partial class C { class D { int M() => 2; } }";

            var editsA = GetTopEdits(srcA1, srcA2);
            var editsB = GetTopEdits(srcB1, srcB2);

            EditAndContinueValidation.VerifySemantics(
                new[] { editsA, editsB },
                new[]
                {
                    DocumentResults(semanticEdits: new[]
                    {
                        SemanticEdit(SemanticEditKind.Replace, c => c.GetMember("C"), partialType: "C")
                    }),
                    DocumentResults(semanticEdits: new[]
                    {
                        SemanticEdit(SemanticEditKind.Update, c => c.GetMember("C.D.M"))
                    })
                },
                capabilities: EditAndContinueCapabilities.NewTypeDefinition);
        }

        [Fact]
        public void NestedType_Replace_WithUpdateInNestedType_Partial_SameDocument()
        {
            var src1 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]partial class C { int N() => 1; } partial class C { class D { int M() => 1; } }";
            var src2 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]partial class C { int N() => 2; } partial class C { class D { int M() => 2; } }";

            var edits = GetTopEdits(src1, src2);

            EditAndContinueValidation.VerifySemantics(
                edits,
                semanticEdits: new[]
                {
                    SemanticEdit(SemanticEditKind.Replace, c => c.GetMember("C")),
                    SemanticEdit(SemanticEditKind.Update, c => c.GetMember("C.D.M"))
                },
                capabilities: EditAndContinueCapabilities.NewTypeDefinition);
        }

        [Fact]
        public void NestedType_Replace_WithUpdateInNestedType()
        {
            var src1 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]class C { int N() => 1; class D { int M() => 1; } }";
            var src2 = ReloadableAttributeSrc + "[CreateNewOnMetadataUpdate]class C { int N() => 2; class D { int M() => 2; } }";

            var edits = GetTopEdits(src1, src2);

            EditAndContinueValidation.VerifySemantics(
                edits,
                semanticEdits: new[]
                {
                    SemanticEdit(SemanticEditKind.Replace, c => c.GetMember("C")),
                    SemanticEdit(SemanticEditKind.Update, c => c.GetMember("C.D.M"))
                },
                capabilities: EditAndContinueCapabilities.NewTypeDefinition);
        }

        [Fact]
        public void NestedType_Update_WithUpdateInNestedType()
        {
            var src1 = @"[System.Obsolete(""A"")]class C { int N() => 1; class D { int M() => 1; } }";
            var src2 = @"[System.Obsolete(""B"")]class C { int N() => 1; class D { int M() => 2; } }";

            var edits = GetTopEdits(src1, src2);

            EditAndContinueValidation.VerifySemantics(
                edits,
                semanticEdits: new[]
                {
                    SemanticEdit(SemanticEditKind.Update, c => c.GetMember("C")),
                    SemanticEdit(SemanticEditKind.Update, c => c.GetMember("C.D.M"))
                },
                capabilities: EditAndContinueCapabilities.ChangeCustomAttributes);
        }

The last one demonstrates it's not an issue just for Replace edit, but we can also have "nested" Update edits.

[davidwengier]

The last one demonstrates it's not an issue just for Replace edit, but we can also have "nested" Update edits.

I don't think it demostrates that. The last two tests here would not have hit the bug. For the bug to repro, the edit for the nested type has to come first. I couldn't find a single-document repro of this issue (which is why I suspect it was never hit in the wild in .cshtml files). It is possible that the first test you have here could be made to hit it, if the compiler say the second document first, but none of the EnC test infra can do that end-to-end type of test that I'm aware of.

Have added the tests and assert either way :)

[tmat]

the nested type has to come first.

That can be arranged if we split into partial in two documents like in the first test. What I meant is that it can occur with two Updates rather then just Replace and Update.

[davidwengier]

ping @tmat

[Youssef1313]

@davidwengier Can you restart the failing job?