Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ protected override FormattableString BuildDescription()
private List<PropertyProvider>? _additionalPropertyProperties;
private ModelProvider? _baseModelProvider;
private ConstructorProvider? _fullConstructor;
private (string Name, string Namespace)? _fullConstructorIdentity;
internal PropertyProvider? DiscriminatorProperty { get; private set; }

private readonly bool _isDiscriminatedBaseType;
Expand Down Expand Up @@ -189,6 +190,7 @@ public override void Reset()
_additionalPropertyFields = null;
_additionalPropertyProperties = null;
_fullConstructor = null;
_fullConstructorIdentity = null;
_isMultiLevelDiscriminator = null;
}

Expand Down Expand Up @@ -230,7 +232,29 @@ protected FieldProvider? RawDataField
protected internal bool SupportsBinaryDataAdditionalProperties => AdditionalPropertyProperties.Any(p =>
p.Type.ElementType.Equals(_additionalPropsUnknownType) ||
(p.Type.ElementType.IsFrameworkType && p.Type.ElementType.FrameworkType == typeof(object)));
public ConstructorProvider FullConstructor => _fullConstructor ??= BuildFullConstructor();
/// <summary>
/// The constructor that takes every serializable property.
/// </summary>
/// <remarks>
/// This instance is also returned as part of <see cref="TypeProvider.Constructors"/>, and callers are free to
/// mutate the constructors they receive. An identity change invalidates the constructor list, so the cached
/// instance is rebuilt alongside it; otherwise a rebuild would reuse the same instance and re-apply any
/// mutation, producing duplicated members.
/// </remarks>
public ConstructorProvider FullConstructor
{
get
{
var identity = (Type.Name, Type.Namespace);
if (_fullConstructor is null || _fullConstructorIdentity != identity)
{
_fullConstructor = BuildFullConstructor();
_fullConstructorIdentity = identity;
}

return _fullConstructor;
}
}

protected override string BuildNamespace() => string.IsNullOrEmpty(_inputModel.Namespace) ?
// TODO remove null check once https://github.com/Azure/typespec-azure/issues/2209 is fixed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.TypeSpec.Generator.Input;
using Microsoft.TypeSpec.Generator.Primitives;
using Microsoft.TypeSpec.Generator.Providers;
using Microsoft.TypeSpec.Generator.Snippets;
using Microsoft.TypeSpec.Generator.Statements;
using Microsoft.TypeSpec.Generator.Tests.Common;
using Microsoft.TypeSpec.Generator.Utilities;
Expand Down Expand Up @@ -2357,6 +2358,76 @@ public void TestUpdate_ResetsSerializationProviders()
Assert.AreEqual(1, newerSerializationProviders.Count);
}

// BuildConstructors returns the cached FullConstructor instance, so invalidating the constructor
// list on an identity change without invalidating FullConstructor left the stale instance in the
// rebuilt list. Derived generators mutate the constructors they get back from BuildConstructors,
// and reusing the same instance caused those mutations to be applied more than once.
[Test]
public void TestUpdate_ResetsFullConstructor()
{
MockHelpers.LoadMockGenerator();
var inputModel = InputFactory.Model("TestModel", properties: [InputFactory.Property("prop1", InputPrimitiveType.String)]);
var modelProvider = new ModelProvider(inputModel);

var fullConstructor = modelProvider.FullConstructor;
Assert.IsTrue(modelProvider.Constructors.Contains(fullConstructor));

// Change name
modelProvider.Update(name: "NewName");
var newFullConstructor = modelProvider.FullConstructor;
Assert.AreNotSame(fullConstructor, newFullConstructor);
// The rebuilt constructor list must contain the rebuilt full constructor, not the stale one
Assert.IsTrue(modelProvider.Constructors.Contains(newFullConstructor));
Assert.IsFalse(modelProvider.Constructors.Contains(fullConstructor));

// Change namespace
modelProvider.Update(@namespace: "NewNamespace");
var newerFullConstructor = modelProvider.FullConstructor;
Assert.AreNotSame(newFullConstructor, newerFullConstructor);
Assert.IsTrue(modelProvider.Constructors.Contains(newerFullConstructor));
}

// Regression coverage for the duplication that the stale FullConstructor caused: a generator that
// adds a suppression to the full constructor every time it builds the constructor list must not see
// its additions accumulate when an identity change triggers a rebuild.
[Test]
public void TestUpdate_DoesNotReapplyConstructorMutationsAfterIdentityChange()
{
MockHelpers.LoadMockGenerator();
var inputModel = InputFactory.Model("TestModel", properties: [InputFactory.Property("prop1", InputPrimitiveType.String)]);
var modelProvider = new MutatingModelProvider(inputModel);

_ = modelProvider.Constructors;
Assert.AreEqual(1, modelProvider.FullConstructor.Suppressions.Count);

modelProvider.Update(name: "NewName");
_ = modelProvider.Constructors;

Assert.AreEqual(1, modelProvider.FullConstructor.Suppressions.Count);
}

// Mimics how ScmModelProvider post-processes the constructors returned from the base implementation.
private class MutatingModelProvider : ModelProvider
{
public MutatingModelProvider(InputModelType inputModel) : base(inputModel)
{
}

protected internal override ConstructorProvider[] BuildConstructors()
{
var constructors = base.BuildConstructors();
foreach (var constructor in constructors)
{
if (ReferenceEquals(constructor, FullConstructor))
{
var suppression = new SuppressionStatement(null, Snippet.Literal("TEST0001"), "Test suppression.");
constructor.Update(suppressions: [suppression, .. constructor.Suppressions]);
}
}
return constructors;
}
}

private class TestModelProvider : ModelProvider
{
private readonly string? _name;
Expand Down
Loading