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.99k stars 4.03k forks source link

Roslyn 4.3 API is not backward compatible with Roslyn 4.2 because of ExclamationExclamationToken #62671

Open gfraiteur opened 2 years ago

gfraiteur commented 2 years ago

The PR https://github.com/dotnet/roslyn/pull/61397 has removed null-parameter checking. I understand this was an experimental feature, but there is no concept of "experimental API", so we now have (auto-generated) code that references for instance ExclamationExclamationToken, which is now broken in VS 7.3.

It does not cause a big concrete problem for us now because our product (Metalama) is preview, however, if the Roslyn team abandons its policy of honoring commitments of API backward compatibility, this puts us, and more importantly our customers, in a difficult position.

In case of violation of the backward compatibility contract, the users of our product (Metalama) have to update it whenever any developer of the customer company decides to update to a new VS build where is an API breaking change in Visual Studio. This causes problems to our customers because :

For these reasons, it is essential for us that the Roslyn team honors backward compatibility.

I understand the challenges of experimental features and the consequence that they need to be implemented by experimental APIs. However, if the Roslyn team wants to keep the freedom to remove experimental APIs, we need the ability to know which APIs are experimental, so we can choose not to support them.

Possible solutions for us are:

It is essential for us that the Roslyn team comes with a systematic solution to the issue of API backward compatibility.

Thank you for considering this issue.

CyrusNajmabadi commented 2 years ago

I'm open to discussing this in our api review. But fundamentally this was a case of a true compat break. Something that we do do. The feature was approved and shipped for real. Then we changed our minds and pulled it out based on feedback and a long period of reassessing. We intentionally pulled it out knowing it was a break. We knew this could break things and that that's ok as 100% compatibility is not something we're signing up for Roslyn with. This falls into our actual policy of basically:

  1. Breaks should be rare.
  2. Breaks should hopefully only happen for a short period of time.
  3. Breaks should be intentional.
  4. Breaks should hopefully affect only a small portion of the ecosystem.

This change feel under all of the above. In this case, the language genuinely changed and the feature was pulled out. At a team level this break was considered acceptable and appropriate.

jaredpar commented 2 years ago

I'm definitely open to the idea of how we tag parts of the compiler API which intersect preview features. Fundamentally though we have to be able to change these APIs while feature remains in preview. Otherwise it would effectively prevent us from doing feature previews (which is highly valuable to all involved).

gfraiteur commented 2 years ago

From our point of view, we have two needs:

  1. Know which APIs are experimental so we do NOT generate code for them,
  2. Know what a SyntaxNode uses an experimental feature WITHOUT referencing the experimental API itself, so we can report a diagnostic when the user uses an unsupported feature.
gfraiteur commented 2 years ago

I worked on the issue on our side and figured out that all new syntax APIs since 4.0.1 were preview, so I dropped support for the 4.1 and 4.2 API. I am also checking LanguageVersion and forbid the Preview value so that should be enough to prevent users from using preview features.

I was wondering: is it a policy that preview features are introduced in minor versions and major versions have a clean and stable API? Is it something we can rely on?

RikkiGibson commented 2 years ago

I feel like it would be reasonable to solve this by putting [Experimental] on public API for features which are only available in preview.