Refine the `client-namespace-conflict` diagnostic #6161

ArcturusZhang · 2025-02-26T10:02:06Z

I removed the report diagnostic in our emitter - because not every generator would have this issue
and move the diagnostic reporting into our C# generator, when we build the namespace for a client type.

I also added a few test cases for the Emitter class in our C# generator, which leads to series of other changes about the disposing issue.

azure-sdk · 2025-02-26T10:11:24Z

API change check

API changes are not detected in this pull request.

…ype deserialization, which might be incorrect. Will change it later

ArcturusZhang · 2025-02-27T07:15:36Z

...nt-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs

+        private const string namespaceConflictCode = "client-namespace-conflict";
+
+        private string? _namespace;
+        protected override string BuildNamespace() => _namespace ??= BuildNamespaceCore();


this method is called multiple times - but we expect it should always return the same result, therefore I added the cache here.
If this is called multiple times, the diagnostic would be reported multiple times which is quite annoying

Why is this called multiple times?

actually, twice:
first time here:

typespec/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs

Line 113 in 134f7e7

CustomCodeView?.BuildNamespace() ?? BuildNamespace(),

second time here:

typespec/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs

Line 38 in 134f7e7

=> CodeModelPlugin.Instance.SourceInputModel.FindForType(BuildNamespace(), BuildName());

we cannot change the second invocation to Namespace, because it would cause infinite invocation of this method.
Therefore I think we cannot actually resolve it

ArcturusZhang · 2025-02-27T07:16:49Z

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs


-        private Emitter()
+        internal Emitter()


changing to internal because with the singleton pattern, the emitter would be disposed when one test case is done, and the stream would be disposed and we cannot get things out of it then.

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs

ArcturusZhang · 2025-02-27T07:18:13Z

...ges/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/StringExtensions.cs

+        /// <param name="left">the first namespace</param>
+        /// <param name="right">the second namespace</param>
+        /// <returns></returns>
+        public static bool IsLastNamespaceSegmentTheSame(string left, string right)


I introduce this method to verify if the namespace has been changed in one client
I added tests for this to verify it is working properly.

ArcturusZhang · 2025-02-27T08:17:42Z

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelPlugin.cs

@@ -46,6 +47,7 @@ public CodeModelPlugin(GeneratorContext context)
            Configuration = context.Configuration;
            _inputLibrary = new InputLibrary(Configuration.OutputDirectory);
            TypeFactory = new TypeFactory();
+            Emitter = null!;


we will inject a value for this property later after the plugin was loaded.

We should throw from the getter instead of doing this. Similar to what we do for other non-nullable properties.

Sure let me change

#6161 (comment)
I changed this so that the emitter could be initialized here.

ArcturusZhang · 2025-02-27T08:20:06Z

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs

        }

-        public static Emitter Instance => _emitter ??= new Emitter();


the singleton pattern was messing things up in our test cases because it might dispose the stream prematurely
therefore I decide to remove the singleton pattern here, and when we load the plugin, we inject the constructed instance in main into the CodeModelPlugin.
We have using statement in the Main method therefore this emitter instance would only be dispose when the program exits.

ArcturusZhang · 2025-02-27T08:21:50Z

...es/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/TestHelpers/MockHelpers.cs

@@ -69,6 +70,8 @@ public static Mock<CodeModelPlugin> LoadMockPlugin(
            // initialize the singleton instance of the plugin
            var mockPlugin = new Mock<CodeModelPlugin>(new GeneratorContext(Configuration.Load(configFilePath, configuration))) { CallBase = true };

+            mockPlugin.Setup(p => p.Emitter).Returns(new Emitter(Console.OpenStandardOutput()));


I did not add a hook here so that we could mock it in our UT - but if in the future we need to check something, we could do it here.

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/perf/PluginInitializer.cs

JoshLove-msft · 2025-02-27T22:59:46Z

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/StartUp/PluginHandler.cs


 namespace Microsoft.TypeSpec.Generator
 {
    internal class PluginHandler
    {
-        public void LoadPlugin(CommandLineOptions options)
+        public void LoadPlugin(Emitter emitter, CommandLineOptions options)


Why do we need the emitter param?

See this comment and I have changed this back

JoshLove-msft · 2025-02-27T23:00:03Z

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/StartUp/PluginHandler.cs

@@ -30,6 +31,7 @@ internal void SelectPlugin(CommandLineOptions options)
                {
                    CodeModelPlugin.Instance = plugin.Value;
                    CodeModelPlugin.Instance.IsNewProject = options.IsNewProject;
+                    CodeModelPlugin.Instance.Emitter = emitter;


Can we just construct the emitter here, and call Dispose on the instance from Program.cs?

Previously Program needs this emitter to write debug information when we attach the debugger.
I think it should be fine if we just simply write the information into stderr - normal users would not come across it.
Now the emitter has been changed to initialize inside the CodeModelPlugin - and Program disposes it if it is still there.

…update

microsoft-github-policy-service bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Feb 26, 2025

remove the diagnostic, will add it into csharp side later

07c774f

ArcturusZhang force-pushed the namespace-diagnostic-update branch from 694d17a to 07c774f Compare February 26, 2025 10:02

ArcturusZhang added 5 commits February 27, 2025 11:00

Merge branch 'main' into namespace-diagnostic-update

a69a088

move the report diagnostic for namespace conflict into input client t…

87180c8

…ype deserialization, which might be incorrect. Will change it later

update

af44eb6

add a few test cases for the emitter rpc

05c59a7

Merge branch 'main' into namespace-diagnostic-update

bca9388

ArcturusZhang commented Feb 27, 2025

View reviewed changes

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs Outdated Show resolved Hide resolved

ArcturusZhang commented Feb 27, 2025

View reviewed changes

ArcturusZhang marked this pull request as ready for review February 27, 2025 07:25

ArcturusZhang requested review from m-nash, JoshLove-msft and jorgerangel-msft as code owners February 27, 2025 07:25

ArcturusZhang added 3 commits February 27, 2025 15:47

regen

88e2180

refactor where the emitter located and resolve the disposing issue

36c0e7e

format

39d4c49

ArcturusZhang commented Feb 27, 2025

View reviewed changes

clean up

ed2947a

ArcturusZhang commented Feb 27, 2025

View reviewed changes

ArcturusZhang mentioned this pull request Feb 27, 2025

Remove the client-namespace-conflict diagnostic warning Azure/autorest.csharp#5249

Draft

6 tasks

JoshLove-msft reviewed Feb 27, 2025

View reviewed changes

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/perf/PluginInitializer.cs Outdated Show resolved Hide resolved

JoshLove-msft reviewed Feb 27, 2025

View reviewed changes

ArcturusZhang added 2 commits February 28, 2025 13:35

Merge remote-tracking branch 'origin/main' into namespace-diagnostic-…

4842a09

…update

resolve comments

3754988

JoshLove-msft approved these changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine the `client-namespace-conflict` diagnostic #6161

Refine the `client-namespace-conflict` diagnostic #6161

ArcturusZhang commented Feb 26, 2025 •

edited

Loading

azure-sdk commented Feb 26, 2025

ArcturusZhang Feb 27, 2025

JoshLove-msft Feb 27, 2025

ArcturusZhang Feb 28, 2025

ArcturusZhang Feb 27, 2025

ArcturusZhang Feb 27, 2025 •

edited

Loading

ArcturusZhang Feb 27, 2025

JoshLove-msft Feb 27, 2025

ArcturusZhang Feb 28, 2025

ArcturusZhang Feb 28, 2025

ArcturusZhang Feb 27, 2025 •

edited

Loading

ArcturusZhang Feb 27, 2025

JoshLove-msft Feb 27, 2025

ArcturusZhang Feb 28, 2025

JoshLove-msft Feb 27, 2025 •

edited

Loading

ArcturusZhang Feb 28, 2025 •

edited

Loading

		}

		public static Emitter Instance => _emitter ??= new Emitter();

Refine the client-namespace-conflict diagnostic #6161

Are you sure you want to change the base?

Refine the client-namespace-conflict diagnostic #6161

Conversation

ArcturusZhang commented Feb 26, 2025 • edited Loading

azure-sdk commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshLove-msft Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

ArcturusZhang Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Refine the `client-namespace-conflict` diagnostic #6161

Refine the `client-namespace-conflict` diagnostic #6161

ArcturusZhang commented Feb 26, 2025 •

edited

Loading

ArcturusZhang Feb 27, 2025 •

edited

Loading

ArcturusZhang Feb 27, 2025 •

edited

Loading

JoshLove-msft Feb 27, 2025 •

edited

Loading

ArcturusZhang Feb 28, 2025 •

edited

Loading