Document the best practice for getting text from a syntax in source generators

[Neme12]

When writing source generators, many users choose to generate the code using the syntax APIs because they find this approach useful for their scenario, even though AddSource only takes a SourceText, not syntax nodes. To get the SourceText from the syntax, the best way is to use AddSource(syntaxNode.GetText(Encoding.UTF8)) - this is more efficient than using syntaxNode.ToString() or syntaxNode.ToFullString() (which is the naive approach that most people would use) because it avoids the string allocation, which could be arbitrarily large. It would be helpful to have this documented in some of the source generator walkthroughs as a recommended/best practice for getting the text from a syntax in case the users choose to generate the code using the syntax APIs, to guide users to do the right thing.

Extracted from dotnet/roslyn#49082 (comment)

The best way to get a SourceText from a CompilationUnit, or any SyntaxNode in general, is to use syntaxNode.GetText(Encoding.UTF8) - that way, no huge string should be allocated.

IMO, it would be a good idea to mention this somewhere in the docs for source generators, so that people do the right thing, as opposed to calling syntaxNode.ToString().

...

Also, getting a SourceText from the syntax is already easy to do in a pretty good way, by using syntaxNode.GetText(Encoding.UTF8). This just needs to be documented so that source generator authors know that this is the recommended practice for getting the text from a syntax, in case they're using the syntax APIs to generate the source.

@jasonmalinowski @chsienki @AArnott

[CyrusNajmabadi]

This makes a lot of sense. We could go even further and even provide an overload that takes a SyntaxNode, where we will do the most efficient thing (whatever that is). We would need to document that the node will be serialized back to text and only that content will be used (not the actual 'value' of the node).

[Neme12]

@CyrusNajmabadi That makes sense too, although a small concern I have about that is that users could then assume that calling that overload would use the syntax directly and avoid any parsing overhead, thinking that it is more efficient than passing a SourceText and that building up the text by creating syntax nodes is the best way to do this, and users could do it that way even when they otherwise wouldn't have and when it doesn't provide any benefit.

[CyrusNajmabadi]

and that building up the text by creating syntax nodes is the best way to do this,

It may be hte best way to do things. There's no uniform 'best' for all scenarios. Having the option there for those who are generating using normal syntax nodes would make a lot of sense.

that users could then assume that calling that overload would use the syntax directly and avoid any parsing overhead, t

I mean, if they make assumptions in direct contradiction to what we doc, that's on them :)

and when it doesn't provide any benefit.

I'll push back on that. I think it would provide a lot of benefit. Building using syntax is extremely natural and sensible. It means you can reuse portions of existing trees and manipulate them accordingly. It means you can compose things nicely. It means you can do things like perform your own analysis of this code using our own APIs, etc. etc.

[Neme12]

I'll push back on that. I think it would provide a lot of benefit. Building using syntax is extremely natural and sensible. It means you can reuse portions of existing trees and manipulate them accordingly. It means you can compose things nicely. It means you can do things like perform your own analysis of this code using our own APIs, etc. etc.

Yes, in some cases, it does provide a lot of benefit, but in other cases, it doesn't because the source that's generated isn't based on existing source etc. For example, in Roslyn's CSharpSyntaxGenerator it wouldn't provide any benefit and generating it as text is the simplest. most readable and most efficient thing to do, especially given how large the generated text is. There's no existing syntax to reuse etc.

[CyrusNajmabadi]

Roslyn's generator is the outlier here, especially as it needs to generate the very syntax nodes it would be trying to create. We specifically did source originally to avoid that circularity issue. Now that we have an actual sdk it could reference, I would definitely move it to just producing nodes. It would clean things up majorly :-)

It's also just extremely low on my priority list :-D

[Neme12]

I would definitely move it to just producing nodes. It would clean things up majorly :-)

My point is that I don't think it would clean anything up. Writing to a StringBuilder line by line as it does today is very clear and readable and you can easily see what it will generate. Using syntax for no purpose will just make it less readable and obfuscate the output a bit. Also, then you'll have to additionally format the code or normalize whitespace, only making it much slower. I'm trying to say that using syntax can often be useful and provide a lot of benefit, but not always - sometimes it's just additional overhead for no benefit whatsoever and it would obfuscate things and make the generator less readable. There are cases when writing to the text directly is the simplest and best thing to do.

[CyrusNajmabadi]

Then use text in that case. Supplying options doesn't mean users don't have access to existing viable solutions.

[sharwell]

I'd be fine with just creating an analyzer to detect the bad patterns listed above. We already have an analyzer package that's enabled by default for analyzer authoring, and can put it there.

[Neme12]

@sharwell That's an awesome idea. I think it's still worth mentioning in the docs though.