Jens-G opened a new pull request, #3814: URL: https://github.com/apache/thrift/pull/3814
Fixes [THRIFT-6199](https://issues.apache.org/jira/browse/THRIFT-6199). > **Stacked on #3813 (THRIFT-6198).** The diff shown here includes that commit until it is merged; review only the second commit. Happy to rebase once #3813 lands. ### What is left after THRIFT-6198 #3813 drops a container extension method that an included program already emits. But the underlying conflict is not tied to the include relation at all — **any** two programs that end up in the same C# namespace and use the same container type declare the same extension method signature, and every call site seeing both classes fails with CS0121. Two topologies remain, both verified against master + #3813: **1. Siblings — the common one.** Two programs pulled into one build by a third, with no include relation between them: ```thrift // A1.thrift namespace * MyApp struct S1 { 1: list<i32> nums } // A2.thrift namespace * MyApp struct S2 { 1: list<i32> nums } // B.thrift namespace * MyApp include both; struct B1 { 1: A1.S1 one, 2: A2.S2 two } ``` ``` S1.cs(74,29): error CS0121: The call is ambiguous between the following methods or properties: 'A1Extensions.DeepCopy(List<int>?)' and 'A2Extensions.DeepCopy(List<int>?)' S2.cs(74,29): error CS0121: ... ``` **2. Container over base types shared with an otherwise unused include** — where #3813 deliberately does not defer, because the including program's code would then depend on files it never references. A generator instance only ever sees its own program and that program's include closure. In topology 1 neither sibling is in the other's closure, so **no per-program rule can decide this one.** A whole-run registry (first program in the run claims the type) was considered and dropped: it makes the content of `A1.Extensions.cs` depend on whether the compiler ran `-r` on a root program or per file, which breaks reproducible output. ### The fix: make the duplicates harmless Two static classes declaring the same extension method is perfectly legal C# — only an *unqualified call* matching both is an error. So generated code now calls a container's `DeepCopy()` through the class that owns it: ```csharp // before tmp5.Inners = this.Inners.DeepCopy()!; // after tmp5.Inners = MyAppTypesExtensions.DeepCopy(this.Inners)!; ``` That resolves the same way however many extension classes of the namespace are in scope. It also has to cover the containers #3813 leaves to an included program, which is why the ownership map is now built in `init_generator()` rather than at close time — the first call site is written long before the extensions file is. Struct and union `DeepCopy()` are ordinary instance methods, never ambiguous, and keep the syntax they had. ### Scope of the generated-code change, measured I diffed the generated output of the whole `Thrift.Compile` corpus (253 files: CassandraTest, ThriftTest, fb303, optional_required_default, name_conflicts, Recursive, Thrift5253/5320/5382/5794/5795/6198) against the previous compiler: | | net10 | netstd2 | |---|---|---| | changed lines | 306 | 296 | | **changed lines not containing `DeepCopy`** | **0** | **0** | Every one is a container `DeepCopy()` call site. A struct-typed field still reads `tmp15.Struct_thing = (global::ThriftTest.Xtruct)this.Struct_thing.DeepCopy()!;`, unchanged. One semantic note: the union path went from `As_x?.DeepCopy() ?? []` to `Ext.DeepCopy(As_x) ?? []`. Equivalent — the generated container `DeepCopy()` opens with `if (source == null) return null;`. ### What this does not fix The duplicate *declarations* still exist in those two topologies, so hand-written code calling `someList.DeepCopy()` in such a namespace still has to qualify. Removing them altogether needs the cross-program pass rejected above. ### Tests `Thrift6199.thrift` pulls two sibling programs into one compilation; both share a namespace and declare `list<i32>` and `map<string, list<i32>>`. Wired into all four `Thrift.Compile` targets. Without the fix it is **8 × CS0121**, covering struct `DeepCopy()`, union `DeepCopy()`, and the nested container copies inside the extension methods themselves. ### Verification * both residual topologies: CS0121 on master+#3813, clean build with this change * `lib/netstd` solution builds clean, all four targets (net8 / net9 / net10 / netstd2); only the two pre-existing CS0114 warnings remain * `Thrift.Tests`: 84/84 pass * `test/netstd` cross-language test app builds 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
