Sample Generation - ExampleValueExpressionBuilder and OperationSample parity with autorest generator#10739
Sample Generation - ExampleValueExpressionBuilder and OperationSample parity with autorest generator#10739MaiLinhP wants to merge 12 commits into
Conversation
commit: |
|
No changes needing a change description found. |
jorgerangel-msft
left a comment
There was a problem hiding this comment.
Sorry if I missed this, but do you have a design doc of the proposed sample generation pipeline? I think that will help with understanding where these changes fit in the overall picture.
| ]; | ||
| } | ||
|
|
||
| protected virtual IReadOnlyList<OperationSample> BuildSamples() |
There was a problem hiding this comment.
Is there a reason why this is protected virtual? Is it for testing? Also do you think the sample building should be decoupled from the client method construction we do here?
|
Can we link to the design? |
|
Can we add some example data to our test TSP? |
|
@JoshLove-msft design linked in description, and I added some examples while keeping some autogenerated mock examples for comparison :) Also interesting that examples are added using JSON load format, and TCGC doesn't seem to process the @opExample decorator at all? No .tsp file currently uses this decorator either 🤔 |
|
@copilot resolve the merge conflicts in this pull request |
…or sample generation on sample client
|
Can we please get a preview pipeline run? |
JoshLove-msft
left a comment
There was a problem hiding this comment.
Automated PR review — C# emitter sample generation (M1–M4)
Reviewed the non-generated source (ExampleMockValueBuilder, ExampleValueExpressionBuilder, ExampleParameterValue, OperationSample, ClientSampleProvider, and the supporting infra changes) plus spot-checked the generated Samples_*.cs output. Overall this is well-structured, thoroughly documented, and the generated bodies in the Local Sample-TypeSpec project look compilable. Test coverage is solid (~170 test methods across the new sample test files).
A few things worth addressing, left as inline comments:
- Silent
catch { continue; }inClientSampleProvider.BuildMethodshides failures and drops samples with no diagnostic. - Reflection into the non-public
CSharpTypeconstructor (CreateExternalCSharpType) is brittle and unguarded by tests. - Generation-time numeric
Convertcalls on raw example strings inExampleValueExpressionBuildercan throwFormatExceptionfor non-numeric spec example values. - No backtracking on
visitedModelsinExampleMockValueBuilder.BuildModelExampleValueunder-populates sibling properties that share a model type. - Unused
using System.ClientModel;added toScmKnownParameters.cs.
Minor / non-blocking: ConsoleWriteLine splices a member-access string as a method name; several known-parameter checks rely on type.Name == "WaitUntil" / "RequestConditions" string matching (acceptable given the cross-assembly setup); convenience samples declare an unused response local (compiles cleanly).
Note: the InputPrimitiveType.IsNumber expansion is low-risk — its only consumer in the generator is the new mock builder.
--generated by Copilot
| } | ||
| catch | ||
| { | ||
| continue; |
There was a problem hiding this comment.
The bare catch { continue; } swallows every exception (including real bugs like NullReferenceException or contract violations) and silently drops the operation from the generated samples. If GetMethodCollectionByOperation starts throwing for a legitimate reason, samples will quietly go missing with no diagnostic. Consider catching a specific expected exception type, or at minimum emitting a diagnostic/warning through the generator's reporting so the drop is observable.
--generated by Copilot
| typeof(CSharpType), typeof(IReadOnlyList<CSharpType>), | ||
| typeof(bool), typeof(bool), typeof(CSharpType), typeof(Type)], | ||
| null)!; | ||
| return (CSharpType)ctor.Invoke([name, ns, false, false, null, Array.Empty<CSharpType>(), true, false, null, null]); |
There was a problem hiding this comment.
Invoking the non-public CSharpType constructor via reflection is brittle: it isn't checked by the compiler, so any change to that constructor's parameter list compiles fine here and then throws MissingMethodException/NullReferenceException at generation time. Is there a public/internal factory for external types (an existing CSharpType overload, or InternalsVisibleTo so the constructor can be called directly)? If reflection has to stay, please add a focused unit test that constructs _testAttributeType so the reflection contract is guarded against silent breakage.
--generated by Copilot
| /// Since the generated file already has <c>using System;</c>, this helper emits the shorter form. | ||
| /// </summary> | ||
| private static MethodBodyStatement ConsoleWriteLine(ValueExpression expression) | ||
| => new InvokeMethodExpression(null, "Console.WriteLine", [expression]).Terminate(); |
There was a problem hiding this comment.
Passing the string Console.WriteLine as the method name to InvokeMethodExpression splices a member-access expression where a method identifier is expected. It works today only because the writer emits the string verbatim and using System; is present. This is fragile (it bypasses any identifier validation/escaping). Consider Static(typeof(Console)).Invoke(...) and, if the global::System.Console qualification is the concern, addressing that at the writer level rather than string-splicing a member access.
--generated by Copilot
| { | ||
| if (format == SerializationFormat.DateTime_Unix) | ||
| { | ||
| var unixValue = rawValue is string us ? Convert.ToInt64(us) : rawValue is int or long ? Convert.ToInt64(rawValue) : 0L; |
There was a problem hiding this comment.
Convert.ToInt64(us) (and the analogous Convert.ToDouble(ds) for TimeSpan below) runs at generation time on the raw example string. For a spec-provided example whose unix-encoded DateTimeOffset / seconds-encoded TimeSpan value is a non-numeric placeholder string, this throws FormatException and crashes the whole emit. The synthesized mock builder currently emits numeric 0 here so it's safe today, but spec examples are not guaranteed to be numeric. Suggest long.TryParse / double.TryParse with a fallback to Default on failure.
--generated by Copilot
|
|
||
| var properties = new Dictionary<string, InputExampleValue>(); | ||
| var result = InputExampleValue.Object(model, properties); | ||
| visitedModels.Add(model); |
There was a problem hiding this comment.
visitedModels.Add(model) is never removed, so this is cycle prevention without backtracking: if two sibling properties at the same level share a model type, the second one resolves to InputExampleValue.Null and the AllParameters mock ends up under-populated. Compare ClientSampleProvider.BuildResponseParseStatements, which does Add then Remove so the same type can appear on parallel branches. Recommend matching that backtracking pattern (remove the model from the set before returning) so only true recursion cycles are cut.
--generated by Copilot
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.ClientModel; |
There was a problem hiding this comment.
This using System.ClientModel; looks unused — nothing in the file references a type from that namespace root (ApiKeyCredential / ClientResult / BinaryContent are not used here). Appears to be a leftover from an earlier iteration; please remove it to avoid an unused-using warning.
--generated by Copilot
| [assembly: InternalsVisibleTo("Microsoft.TypeSpec.Generator.Input.Tests.Perf, PublicKey=002400000480000094000000060200000024000052534131000400000100010041df4fe80c5af6ff9a410db5a173b0ce24ad68764c623e308b1584a88b1d1d82277f746c1cccba48997e13db3366d5ed676576ffd293293baf42c643f008ba2e8a556e25e529c0407a38506555340749559f5100e6fd78cc935bb6c82d2af303beb0d3c6563400659610759b4ed5cb2e0faf36b17e6842f04cdc544c74e051ba")] | ||
| [assembly: InternalsVisibleTo("Microsoft.TypeSpec.Generator.Input.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010041df4fe80c5af6ff9a410db5a173b0ce24ad68764c623e308b1584a88b1d1d82277f746c1cccba48997e13db3366d5ed676576ffd293293baf42c643f008ba2e8a556e25e529c0407a38506555340749559f5100e6fd78cc935bb6c82d2af303beb0d3c6563400659610759b4ed5cb2e0faf36b17e6842f04cdc544c74e051ba")] | ||
| [assembly: InternalsVisibleTo("Microsoft.TypeSpec.Generator.Tests.Common, PublicKey=002400000480000094000000060200000024000052534131000400000100010041df4fe80c5af6ff9a410db5a173b0ce24ad68764c623e308b1584a88b1d1d82277f746c1cccba48997e13db3366d5ed676576ffd293293baf42c643f008ba2e8a556e25e529c0407a38506555340749559f5100e6fd78cc935bb6c82d2af303beb0d3c6563400659610759b4ed5cb2e0faf36b17e6842f04cdc544c74e051ba")] | ||
| [assembly: InternalsVisibleTo("Microsoft.TypeSpec.Generator, PublicKey=002400000480000094000000060200000024000052534131000400000100010041df4fe80c5af6ff9a410db5a173b0ce24ad68764c623e308b1584a88b1d1d82277f746c1cccba48997e13db3366d5ed676576ffd293293baf42c643f008ba2e8a556e25e529c0407a38506555340749559f5100e6fd78cc935bb6c82d2af303beb0d3c6563400659610759b4ed5cb2e0faf36b17e6842f04cdc544c74e051ba")] |
There was a problem hiding this comment.
Why is this needed? Typically we shouldn't expose the internals like this outside of tests. Should we consider making whatever types are needed non-internal instead ?
| var methods = new List<MethodProvider>(); | ||
|
|
||
| // Ensure client methods are built so MethodCache is populated | ||
| _ = _client.Methods; |
There was a problem hiding this comment.
should we be using the canonical view _client.CanonicalView.Methods ?
| } | ||
| else | ||
| { | ||
| streamExpr = responseVar.Invoke("GetRawResponse", []).Property("ContentStream"); |
There was a problem hiding this comment.
will this method invocation work for azure clients? In general, I recommend we use the ClientResponseApi whenever possible.
| streamExpr = responseVar.Invoke("GetRawResponse", []).Property("ContentStream"); | ||
| } | ||
|
|
||
| var parseExpr = Static(typeof(JsonDocument)).Invoke(nameof(JsonDocument.Parse), streamExpr) |
There was a problem hiding this comment.
nit: can we use the JsonDocumentSnippets?
| yield return stmt; | ||
| } | ||
|
|
||
| visitedTypes.Remove(modelType); |
There was a problem hiding this comment.
question: any chance of a race condition here between the add and remove ?
| InputExampleValue.Stream(InputPrimitiveType.String, "<filePath>")); | ||
|
|
||
| // Should produce File.OpenRead("...") | ||
| Assert.IsInstanceOf<InvokeMethodExpression>(result); |
There was a problem hiding this comment.
This just validates that the built expression is of a certain type. Should we consider using TestData or validating the written expressions to ensure they are generated as we expect?
| // (Format is private, but this is the key parity assertion) | ||
| var formatProp = typeof(FormattableStringExpression).GetProperty("Format", BindingFlags.NonPublic | BindingFlags.Instance); | ||
| Assert.IsNotNull(formatProp, "Format property should exist on FormattableStringExpression"); | ||
| Assert.AreEqual("new DefaultAzureCredential()", formatProp!.GetValue(expr)); |
There was a problem hiding this comment.
I don't think we should have azure related tests in MTG
| { | ||
| /// <summary> This sample shows how to call SayHi and parse the result. </summary> | ||
| [global::NUnit.Framework.TestAttribute] | ||
| [global::NUnit.Framework.IgnoreAttribute("Only validating compilation of examples")] |
There was a problem hiding this comment.
Hmm do we know why these attributes weren't resolved by Roslyn ? We might have to add NUnit to the metadata references for the generator.
| ApiKeyCredential credential = new ApiKeyCredential("<key>"); | ||
| SampleTypeSpecClient client = new SampleTypeSpecClient(endpoint, credential); | ||
|
|
||
| using BinaryContent content = BinaryContent.Create(BinaryData.FromObjectAsJson(new |
There was a problem hiding this comment.
I think FromObjectAsJson is not AOT safe and may cause issues in the azure repo. I'm not sure if the tests directory is setup to catch this in that repo. @JoshLove-msft @jsquire do you know ?
| { | ||
| requiredString = "<requiredString>", | ||
| requiredInt = 0, | ||
| requiredCollection = new object[] { "1" }, |
There was a problem hiding this comment.
do you think we should try to use the explicit element type of the collection instead? will that work here?
Implements M1-M4 milestones for sample generation for the C# emitter — from mock value synthesis to emitting compilable Samples_{Client}.cs files. Based on the design document.
New Components
ExampleMockValueBuilder(M1) — GeneratesInputOperationExampleinstances for operations without spec-provided examples. ProducesShortVersion(required only) andAllParameters(with optional parameters) variants. This was simplified compared to autorest versions - instead of giving concrete mock values that matches the expected format for time/duration/different number types etc., mock values were simplified to "<{parameter name/encoded type hints}>" for strings, and 0 for numbers. This is because we're expecting AI based sample generator to read from deterministic samples and come up with scenario based mock values, hence a hint would be more useful than hardcoded mock values.ExampleValueExpressionBuilder(M2) — ConvertsInputExampleValue→ C#ValueExpressionAST nodes. Covers all primitives, collections, enums, models, credentialsApiKeyCredential,DefaultAzureCredential,BinaryContentanonymous objects, streams, etc.ExampleParameterValue(M2) — Dual-mode bridge holding either rawInputExampleValueor a pre-builtValueExpressionfor known parameters (credentials, endpoints).OperationSample(M3) — Resolves client construction chains (subclient → root), parameter value mappings (with spread support), inline/out-of-line decisions, and paging response type unwrapping.ClientSampleProvider(M4) —TypeProviderthat emitstests/Generated/Samples/Samples_{Client}.cswith sync+async [Test]/[Ignore] methods. Handles normal, pageable (foreach/await foreach), and LRO invocation patterns with JSON response parsing. Note: Per design doc,BuildSamplesshould live in
ScmMethodProviderCollection.cswhich is the equivalent of autorestOperationMethodChainBuilder.cs. However, coupling sample building with method building was purely for convenience, so it has been moved out to ClientSampleProvider for more clarity.Infrastructure Changes
ScmOutputLibrary— RegistersClientSampleProviderfor all clientsScmMethodProviderCollection— ExposedMethodProvidersfor sample provider access