SyntaxProvider.ForAttributeWithMetadataName returns duplicate values when there are multiple partial classes.

[m-nash]

Version Used: 4.3.0

Steps to Reproduce:

If you compile this code

using System;
using System.ClientModel.Primitives;
using System.Text.Json;

namespace TestProject
{
    [ModelReaderWriterBuildable(typeof(JsonModel))]
    public partial class LocalContext : ModelReaderWriterContext { }

    public class JsonModel : IJsonModel<JsonModel>
    {
        public JsonModel Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options) => new JsonModel();
        public JsonModel Create(BinaryData data, ModelReaderWriterOptions options) => new JsonModel();
        public string GetFormatFromOptions(ModelReaderWriterOptions options) => "J";
        public void Write(Utf8JsonWriter writer, ModelReaderWriterOptions options) { }
        public BinaryData Write(ModelReaderWriterOptions options) => BinaryData.Empty;
    }

    [ModelReaderWriterBuildable(typeof(JsonModel2))]
    public partial class LocalContext : ModelReaderWriterContext { }

    public class JsonModel2 : IJsonModel<JsonModel2>
    {
        public JsonModel2 Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options) => new JsonModel2();
        public JsonModel2 Create(BinaryData data, ModelReaderWriterOptions options) => new JsonModel2();
        public string GetFormatFromOptions(ModelReaderWriterOptions options) => "J";
        public void Write(Utf8JsonWriter writer, ModelReaderWriterOptions options) { }
        public BinaryData Write(ModelReaderWriterOptions options) => BinaryData.Empty;
    }
}

Into a source generator that registers this increment values provider

        var typesFromAttribute = context.SyntaxProvider.ForAttributeWithMetadataName(BuildableAttributeName,
                predicate: (node, _) => true,
                transform: (ctx, _) => (ctx.TargetSymbol as INamedTypeSymbol, ctx.Attributes))
            .Collect();

You will get 2 entries both have the exact same symbol and both of those symbols 2 attributes. This means I have to dedupe either in the transform or the register source callback.

Expected Behavior:

I should get 1 symbol with 2 attributes.

Actual Behavior:

I get 2 duplicate symbols with 2 attributes each.

[333fred]

@CyrusNajmabadi @chsienki, is this by design? Or a bug?

[CyrusNajmabadi]

By design. As per the docs, we callback with the nodes taht have the corresponding attribute on them. In this case, there are two nodes. We also give the symbol back. For convenience, we give the entire list of matching attributes.

Deduping seems like a totally reasonable way to go.

--

Note: we could consider potentially giving a partial set of attributes in the callback. However, i'm wary about that breaking existing clients.

[m-nash]

Not sure I would advocate for sending a partial attribute list. If we were dealing with Syntax nodes that would make sense but since we are dealing with symbols I would expect all attributes on the symbol to be in the list.

If it is by design that the consumer will need to dedupe the symbols that come back then feel free to close this issue.

For what its worth this was surprising behavior to me but I don't fully understand the inner workings so I might be an outlier.

[RikkiGibson]

roslyn/src/Compilers/Core/Portable/SourceGeneration/IncrementalGeneratorInitializationContext_ForAttributeWithMetadataName.cs

Lines 143 to 147 in 9ea52f2

	if (attribute.ApplicationSyntaxReference?.SyntaxTree == targetSyntaxTree &&
	attribute.AttributeClass?.ToDisplayString(s_metadataDisplayFormat) == fullyQualifiedMetadataName)
	{
	result.Add(attribute);
	}

If I am understanding right: it looks like, when the partial declarations with attributes happen to be in different files, the host is already doing a sort of "deduping" of the attributes.

I am wondering if, generators are more likely to have bugs from the current behavior, due to assuming deduping is being done for them, which are simply not observed due to "multiple partials with attributes in one file" being a relatively uncommon case. And if we made it so the attributes list in each callback only consisted of the attributes on that particular declaration, those generators would actually start behaving correctly.

[CyrusNajmabadi]

Ah, good point rikki.

Given that, I feel more amenable to changing this to always limit to the results in the particular syntax node.

This would only affect this narrow case, as you mentioned.

[m-nash]

For more context here it seems that if you separate the 4 classes into 4 files instead of one you get different behavior from roslyn. We actually get what @RikkiGibson was expected which is each partial only has the attributes applied to that syntax node.

I am not sure if this is a separate bug or not, but it was very unexpected that 4 classes in one file would behave differently than the exact same 4 classes in different files. If you think this is a separate bug let me know I can open a different issue since that might be affecting more APIs than just ForAttributeWithMetadataName.