Allow CSharpSyntaxGenerator to be used as a source generator

[jasonmalinowski]

This takes a fairly hacky approach to updating the code by converting the CSharpSyntaxGenerator project to build twice: once as a
netcoreapp3.1 console app like it always did, and now also as a netstandard2.0 library that is a source generator. The source generator one only supports building the .cs outputs that are consumed as regular APIs, and ignores the test or grammar outputs for now.

[jasonmalinowski]

All of the code in this project was in CSharpSyntaxGenerator; I don't know why the RootNamespace was what it was originally set to.

[jasonmalinowski]

This was why I'm doing this funky build hackery: if you're producing the grammar tool, it actually builds by linking in the source files of the compiler. I wanted the assurance that the source generator form was behaving as a true 'source generator' and any information about the source was actually being read out of the inputs rather than being linked at the time of build. If it was consuming stuff from the earlier build then we could end up in some really funky situations where a change would have to be made the compiler directory, then you'd have to build the new generator, and then build the other project again to get everything into sync.

[jasonmalinowski]

@chsienki Should we have an API that lets you do AddSource and directly give it a StringBuilder? Having to write this myself seems icky.

[sharwell]

Why not use the overload that takes a string?

Suggested change

	context.AddSource(hintName, SourceText.From(new StringBuilderReader(stringBuilder), stringBuilder.Length, encoding: Encoding.UTF8));
	context.AddSource(hintName, stringBuilder.ToString());

[jasonmalinowski]

@sharwell: because that'll allocate a 2MB string. 😄

[chsienki]

@jasonmalinowski I'm 100% fine with adding at string builder overload. Do you want me to add it to this PR?

[jasonmalinowski]

I'd say no, so if we have to revert this we're also not reverting a public API change that somebody else might depend on.

[jasonmalinowski]

Filed #49082 to track it.

[333fred]

I want to make sure that we don't merge this before we've validated the experience works on vscode, or this will effectively break my ability to work.

[jasonmalinowski]

Marking back as draft as there's some toolset changes and other stuff that needs fixing.

[jasonmalinowski]

So I'm able to see the generator running fine as long as I'm running the latest Omnisharp by setting omnisharp.path to latest in my settings.json. It doesn't seem that editing the Syntax.xml results in the generators rerunning though. @JoeRobich is there some known bug where additional files don't get updated in the Workspace? I don't see any code doing updates in OmnisharpWorkspace for additional files, but it's possible I don't know what I'm looking for.

[RikkiGibson]

I want to make sure that we don't merge this before we've validated the experience works on vscode, or this will effectively break my ability to work.

I also work primarily in vscode and losing the ability to go-to-def etc. will be a significant inconvenience for me.

In addition, I'm concerned about moving forward to a model where, because the generated syntax isn't version controlled, it's difficult for us to audit changes to it over time, or for example review a change to the generator in terms of the diff of the output code.

[CyrusNajmabadi]

In addition, I'm concerned about moving forward to a model where

These are fundamental development concerns. I think if we're going to be pushing SGs as a solution to codegen, we'll need answers there. If we can't even use them for a basic case like this for us (i.e. simple model produces simple codegen'ed api), i hesitate about us stating that others should use it in production settings.

[RikkiGibson]

Thinking over it a bit. I think that it's just a matter of distinguishing the "generator user" versus "generator author" experiences. We are the latter. In our case, the output of the generator when we feed in Syntax.xml for example is a test baseline. CI needs to fail when the baseline is out of sync with what the generator actually produces. So if we can make sure that happens, then I'm good.

[jasonmalinowski]

We are the latter.

@RikkiGibson In this case, I think we're both, right? If you're adding new syntax nodes as part of feature development, do you you need to show the diff of the underlying generated file in that case? You might want to reference it while you're developing, but is that actually reviewed on it's own? I agree that if you're changing the generator then it's a different story.

In our case, the output of the generator when we feed in Syntax.xml for example is a test baseline. CI needs to fail when the baseline is out of sync with what the generator actually produces. So if we can make sure that happens, then I'm good.

@RikkiGibson can you clarify this a bit? The CI today fails if you failed to run the generator before checking in; the point of moving this to a source generator is it's not possible to be out of sync anymore. (indeed that's part of the point of generators in the first place...)

[RikkiGibson]

In this case, I think we're both, right?

Yes.

If you're adding new syntax nodes as part of feature development, do you you need to show the diff of the underlying generated file in that case?

No, that is not particularly useful to do. Perhaps if an input change exposed a bug in the generator, but that seems like something you'd examine locally, not in code review.

can you clarify this a bit?

Sorry, I stated this too much in terms of "how it works right now". When someone PRs an implementation change for a generator like CSharpSyntaxGenerator I just want to be able to easily review the diff of the generator output.

Less importantly: I can also conceive of situations where I'd want to f12 on a generated method, then run a blame to figure out when the generator was changed to produce the implementation that particular way.

[CyrusNajmabadi]

Less importantly: I can also conceive of situations where I'd want to f12 on a generated method, then run a blame to figure out when the generator was changed to produce the implementation that particular way.

This is also in teh scope of "i think everyone who uses source generators would want this". So by dogfooding this, we get a realistic understanding around requests for that sort of thing that will likely come in the future. :)

[chsienki]

@jasonmalinowski which analyzer rules are failing to be taken into account? I see a couple in the correctness leg, but they look like they're in actual files, not generated ones? I see now you made it a global config. I've written up some thoughts on #47384 about how we should handle this.

[chsienki]

The analyzer consistency problem wasn't directly caused by Jason's changes, but rather the changes are surfacing an issue we already had:

The net5.0 sdk now includes the NetAnalyzers package directly. We also reference it via NuGet, meaning we're trying to load the same analyzers twice. Previously when we disabled analyzers we did so by removing them at the MSBuild layer, meaning we never actually tried to load them. As part of this PR we're now disabling them via the new flag, so we're now loading them (even though they don't run) meaning we correctly detect the duplicate analyzers.

For now I've disabled the in-box SDK analyzers, as we want to run them on other TFM builds too (and keep them in-sync) but we might consider conditionally including/excluding depending on the TFM we're building in the future.

[jasonmalinowski]

Tagging @dotnet/roslyn-compiler, @dotnet/roslyn-infrastructure for reviews.

And in general recognition to the conversation above: it is absolutely understood that our tooling around source generators is not perfect. Our goal of enabling this is to get dogfooding (since right now we have literally no dogfooding at all) and to use our experiences to help guide which experiences to improve next. We believe we've addressed core needs that would cause outright blockage of workflows that would have no workarounds whatsoever. We know there's going to be some pain, but it's a tradeoff we're willing to make to deliver a better product to our customers.

[jasonmalinowski]

@dotnet/roslyn-compiler could I get some reviews?

[jasonmalinowski]

@jaredpar @chsienki Diagnostic handling updated. Ready for review again.

[Neme12]

Creating a TextReader from SourceText seems like this is something that should be provided by Roslyn as well. Otherwise, people are more likely to create a string out of it than make their own reader, which is less efficient.