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 @@ -901,6 +901,15 @@ private bool EmitBindImplForMember(
}
case ComplexTypeSpec complexType:
{
// Early detection of types we cannot bind to and skip it.
if (!_typeIndex.HasBindableMembers(complexType) &&
!_typeIndex.GetEffectiveTypeSpec(complexType).IsValueType &&
complexType is not CollectionSpec &&
((ObjectSpec)complexType).InstantiationStrategy == ObjectInstantiationStrategy.ParameterizedConstructor)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain all these conditions? I only understand the first, why the others?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complexType is not CollectionSpec we have special handling to the collections so we should not change how we do it especially it should work in most cases. look at

for more details.

((ObjectSpec)complexType).InstantiationStrategy == ObjectInstantiationStrategy.ParameterizedConstructor) is needed because if it is different ObjectInstantiationStrategy value means we can still create and handle the object. look at

if (strategy is ObjectInstantiationStrategy.ParameterlessConstructor)
to see that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_typeIndex.GetEffectiveTypeSpec(complexType).IsValueType can be handled too,

shows what we do with that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to scope the change to exact failure cases to reduce any risk of breaking anything.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I was surprised we'd even get this far with a type that has no bindable members and we're trying to construct with a parameterized constructor. I would have expected us to discover that in parsing and use a different spec type (not supported or something) - why do we miss this in parsing?

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because we want use _typeIndex which is created and filled in the emitter. If we want to do the detection earlier, we'll need to do more involving refactoring which will make the change risky for 9.0. Note, the parser fills some data structures which expect to see the type specs supported by _typeIndex later during the emission. so refactoring will not be simple IMO.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - given that it seems like the parser isn't even capable of completely deciding which types will be handled. It can only decide which may be handled and we filter further during emit. That feels like something we'd need to fix to address other issues.

{
return false;
}

string sectionValidationCall = $"{MethodsToGen_CoreBindingHelper.AsConfigWithChildren}({sectionParseExpr})";
string sectionIdentifier = GetIncrementalIdentifier(Identifier.section);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,39 @@ async Task Test(bool expectOutput)
}
}

/// <summary>
/// We binding the type "SslClientAuthenticationOptions" which has a property "CipherSuitesPolicy" of type "CipherSuitesPolicy". We can't bind this type.
/// This test is to ensure not including the property "CipherSuitesPolicy" in the generated code caused a build break.
/// </summary>
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
public async Task IgnoredUnBindablePropertiesTest()
{
string source = """
using System;
using System.Net.Security;
using Microsoft.Extensions.Configuration;
using System.Collections.Immutable;
using System.Text;
using System.Text.Json;

public class Program
{
public static void Main()
{
ConfigurationBuilder configurationBuilder = new();
IConfiguration config = configurationBuilder.Build();

var obj = config.Get<SslClientAuthenticationOptions>();
}
}
""";

ConfigBindingGenRunResult result = await RunGeneratorAndUpdateCompilation(source, assemblyReferences: GetAssemblyRefsWithAdditional(typeof(ImmutableArray<>), typeof(Encoding), typeof(JsonSerializer), typeof(System.Net.Security.AuthenticatedStream)));
Assert.NotNull(result.GeneratedSource);

Assert.DoesNotContain("CipherSuitesPolicy = ", result.GeneratedSource.Value.SourceText.ToString());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
[ActiveIssue("Work out why we aren't getting all the expected diagnostics.")]
public async Task IssueDiagnosticsForAllOffendingCallsites()
Expand Down