dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.9k stars 4.01k forks source link

Add a guarding code which prevents putting Compilation into ISG pipeline. #71629

Open lsoft opened 8 months ago

lsoft commented 8 months ago

Due to widespread problem with having Compilation inside of ISG pipeline:

I would suggest to add into ISG pipeline builder a special checks (guard) for incorrect types and etc. If checks fired, then exception has been fired and the ISG must be disabled.

In general, any ISG developer should see a bug during development and well before publishing the ISG. But for ISGs that are already published, a forced shutdown will have benefits in the long run.

If a "general" guard (against all incompatible types) is not possible, a special guard against Compilation will be useful too.

jaredpar commented 7 months ago

I would suggest to add into ISG pipeline builder a special checks (guard) for incorrect types and etc

I'm unsure what you mean by this. Particularly unsure what incorrect types mean here. Can you try elaborating a bit?

lsoft commented 7 months ago

@jaredpar

I'm not an expert in ISG building, so I may say something stupid, but let's take a look to the following code snippet:

IncrementalValuesProvider<MyClass> myClasses = context.SyntaxProvider.CreateSyntaxProvider(...);

...

public class MyClass
{
     public string FoundClassName;
}

MyClass is not overrides GetHashCode and Equals and will be checked by reference. If I understand the ISG pipeline correctly, it means caching will be powerless for this step of ISG pipeline. So, is it possible for types like IncrementalValuesProvider<T> detect if T do not override (implement) GetHashCode and Equals?

I understand there is no silver bullet to exclude every "incorrect" uses of ISG pipelines, but any additional guard (even against overridden GetHashCode + Equals, or against predefined forbidden types like Compilation) will be good for ecosystem.

did I answer to your question?

jaredpar commented 7 months ago

Essentially warning when an incremental generator is not using equitable types in value providers. That is something we have discussed a few times. Usually though our emphasis is on record types used here which are not properly equitable (generally because they included an array field).

This is very much on our mind for future SG areas.