Calculate SuppressAddComponentParameter in tooling

[ryzngard]

Fixes #10736

Chris did a good breakdown of what this value is used for #10736 (comment)

[ryzngard]

@chsienki @jjonescz does this design work moving forward? If so I'll start addressing feedback and get this ready to merge

[chsienki]

@ryzngard Good for me!

[jjonescz]

Sounds good to me as well

[DustinCampbell]

I took another look and I think I spotted a significant bug that might be the cause of the test failures.

[DustinCampbell]

🤔 Is there a better way to do this? It feels unnecessarily expensive to create a temporary CSharpCompilation with all of the metadata references just to get the value of this option. Could the compilation be passed to ComputeRazorSourceGeneratorOptions rather than the metadata references? Also, the old code checked the metadata references for Microsoft.AspNetCore.Components.dll before creating the compilation. Is that optimization no longer needed?
 
@chsienki, do you have any concerns with this compilation being created here? Note that it happens much earlier in RazorSourceGenerator.Initialize(...) than it did before. I'm definitely not an expert on the incremental generator machinery, so I'm not sure what kind of impact this will have.

An alternative approach might be to make the RazorSourceGenerationOptions.Configuration lazily computed, or just create the RazorConfiguration in GetDeclarationProjectEngine(...) and GetGenerationProjectEngine(...) and remove it from RazorSourceGenerationOptions altogether.

[chsienki]

@DustinCampbell I wrote this, lol.

Agree that we should filter to MS.ANC.Components.dll to make it faster, that's a bug on me porting it over.

It does seem wild that we do this, but as its structured it's still the fastest way to do this. The compilation is going to change continuously, which means we'd end up re-running this code extremely frequently. The metadata refs change very infrequently to be amortized to the point where this basically only runs once (especially if we're actually filtering the reference list).

Now, that being said we could probably split this up into two steps, one of which runs from the compilation, so the rest doesn't run. That would be faster for batch-compilation but I can't predict the difference in IDE without actually benchmarking it (I suspect its probably a wash though).

I have it on my TODO list to clean some of this up. Let me open an issue to track that so we don't forget about it.

[DustinCampbell]

✅ Looks much better to me! I left a couple of small suggestions, but you can totally leave them if you want.

[DustinCampbell]

💭 Since this is the source generator, would it make sense to avoid the extra allocation and array copy caused by ToImmutableArray()? Something like this would effectively be allocation free until its known that there are any referenecs.

Suggested change

	var minimalReferences = references
	.Where(r => r.Display is { } display && display.EndsWith("Microsoft.AspNetCore.Components.dll", StringComparison.Ordinal))
	.ToImmutableArray();
	var isComponentParameterSupported = minimalReferences.Length == 0
	? false
	: CSharpCompilation.Create("components", references: minimalReferences).HasAddComponentParameter();
	using var minimalReferences = new PooledArrayBuilder<MetadataReference>(capacity: references.Length);
	foreach (var reference in references)
	{
	if (reference.Display is { } display &&
	display.EndsWith("Microsoft.AspNetCore.Components.dll", StringComparison.Ordinal))
	{
	minimalReferences.Add(reference);
	}
	}
	var isComponentParameterSupported = minimalReferences.Count > 0
	? CSharpCompilation.Create("components", references: minimalReferences.DrainToImmutable()).HasAddComponentParameter();
	: false;

[DustinCampbell]

nit: This might read a little better with the boolean logic flipped.

Suggested change

	var suppressAddComponentParameter = compilation is null
	? false
	: !compilation.HasAddComponentParameter();
	var suppressAddComponentParameter = compilation is not null
	? compilation.HasAddComponentParameter()
	: true;

[chsienki]

LGTM. I don't quite follow why we need the option separately from what's in the configuration but given that we need to clean that all up anyway let's just leave it and sort it later on.

[ryzngard]

LGTM. I don't quite follow why we need the option separately from what's in the configuration but given that we need to clean that all up anyway let's just leave it and sort it later on.

Yea, it's just a matter of how the tooling side reports things. In VS Code we have to serialize across because we don't have the compilation to get data from. I agree this should be cleaned up. Likely what should happen is do a first pass to make IDE have it's own structures for getting information that it should provide and then correctly provide that to the compiler when needed. We're doing a lot of reusing of objects that doesn't make as much sense anymore

[DustinCampbell]

LGTM. I don't quite follow why we need the option separately from what's in the configuration but given that we need to clean that all up anyway let's just leave it and sort it later on.

Because the compiler's code gen checks this value from the code gen options. It doesn't have access to the configuration at that point, which I think is probably the right design. (After all, it would be fragile for the compiler's code gen to be checking for code gen options on two different objects.)

Unfortunately, the compiler's code gen options aren't always created with the configuration in hand. There's a ton of weird logic in there that I looked at briefly in #10764 and ultimately decided to deal with later. Once that's rationalized, I think the code in the source generator options can be cleaned up.