dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

STJ generator with external (generated) context class #108317

Open alrz opened 6 days ago

alrz commented 6 days ago

Currently, you cannot stack Roslyn source generators to operate on generated code. As a workaround I opted into running my own generators in a separate project. That is, [JsonSerializable(...)] class MyContext : JsonSerializerContext is generated elsewhere, however this requires the STJ generator to recognize external context classes.

Project1:

// use a custom generator to generate the context, 
// note this is only a "blueprint" since STJ generator is not yet run.

// <auto-genreated/>
[JsonSerializable(...)] 
[JsonSerializable(...)] 
public class GeneratedContext /* : JsonSerializerContex; */

Project2 references Project1:

// STJ generator won't recognize GeneratedContext in scope
// but perhaps we can use something like this to hint on that:

[JsonSerializerExternalContext(typeof(GeneratedContext))] // use as attribute source
partial class MyContext : JsonSerializerContex;

This way we can stack generators using separate projects but the workaround only works if the final generator can work with external types.

(PS: this is not a problem specific to STJ though, every time you want to stack generators this can be a blocker, so maybe a more generic solution can be implemented instead)

dotnet-policy-service[bot] commented 6 days ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

elgonzo commented 6 days ago

Have you already tried using JsonTypeInfoResolver.Combine to combine the MyContext.Default and GeneratedContext.Default resolvers?

alrz commented 6 days ago

GeneratedContext.Default

That doesn't exist, note when you generate the context, STJ generator won't see it. I did elaborate why this is the case in OP.

eiriktsarpalis commented 5 days ago

It's something we had considered, but it didn't quite seem to justify the added complexity. What motivates this use case? Size concerns? Design time performance?

alrz commented 5 days ago

What motivates this use case?

It's a workaround for stacking generators that roslyn doesn't support. The scenario is being able to generate the context itself.

It's something we had considered, but it didn't quite seem to justify the added complexity

Since something like JsonSerializerExternalContext (or maybe MetadataTypeAttribute since the usage is similar) is a direct hint for the generator to know where to look at, I don't think it's necessarily adds too much complexity or perf penalty for this to be considered?

eiriktsarpalis commented 5 days ago

Oh I see, the context on the first project is generated by a custom generator which you want to take into account in the second project. It seems like a niche scenario if I'm honest, it's very unlikely we would be able to prioritize it in the near future.

alrz commented 5 days ago

@eiriktsarpalis If the API is acceptable I can take a stab at it.

The following is roughly the only change needed to cover this:

private void ParseJsonSerializerContextAttributes(
        INamedTypeSymbol contextClassSymbol,
        out List<TypeToGenerate>? rootSerializableTypes,
        out SourceGenerationOptionsSpec? options)
{
    Debug.Assert(_knownSymbols.JsonSerializableAttributeType != null);
    Debug.Assert(_knownSymbols.JsonSourceGenerationOptionsAttributeType != null);

    rootSerializableTypes = null;
    options = null;

    foreach (AttributeData attributeData in contextClassSymbol.GetAttributes())
    {
        INamedTypeSymbol? attributeClass = attributeData.AttributeClass;

        if (SymbolEqualityComparer.Default.Equals(attributeClass, _knownSymbols.JsonSerializableAttributeType))
        {
            /* ... */
        }
        else if (SymbolEqualityComparer.Default.Equals(attributeClass, _knownSymbols.JsonSourceGenerationOptionsAttributeType))
        {
             /* ... */
        }
+       else if (SymbolEqualityComparer.Default.Equals(attributeClass, _knownSymbols.MetadataTypeAttributeType))
+       {
+           var typeSymbol = (INamedTypeSymbol?)attributeData.ConstructorArguments[0].Value;
+           ParseJsonSerializerContextAttributes(typeSymbol, out var externalTypes, out var externalOptions);
+           if (externalTypes is not null) 
+               rootSerializableTypes.AddRange(externalTypes);
+           if (externalOptions is not null) 
+               options = externalOptions;
+       }
    }
}
eiriktsarpalis commented 5 days ago

It's not that simple. You need to API propose a new attribute for including contexts. There's also design challenges. What happens if you specify two contexts and they have conflicting contracts for a given type? What happens if you have a ][sonSerializable(typeof(Foo))] in the last context and the transitive closure of either of the earlier contexts also includes a conflicting definition for Foo? This is not an easy problem.

alrz commented 5 days ago

conflicting definition

Isn't all that already possible to happen? We're just adding another source for attributes (the "metadata class"), the semantics of the sum of the applied attributes remain intact.

This definitely needs a design review pass but in any case I can start a prototype for that to happen at some point. For one, I'd like this attribute to be sufficient for the generator to find the type so there's practically two entries, JsonSerializableAttribute and MetadataTypeAttribute (or similar).