Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gfraiteur opened this issue Jul 15, 2022 · 5 comments

Comments

@gfraiteur
Copy link

gfraiteur commented Jul 15, 2022

The PR #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 :

  • They need to synchronize the updates to VS and to Metalama, which is not always possible because not all customers force all their team members to use the same VS version.
  • Customers may not have an up-to-date support subscription for our products, and may be angry, for good reasons, of being forced to pay for our product just to be able to update VS.

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:

  • an [Experimental] custom attribute on the API,
  • keeping the experimental API but marking it as [Obsolete],
  • an Experimental attribute in Syntax.xml,
  • omitting this API from PublicApi.Shipped.txt.

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 15, 2022
@CyrusNajmabadi
Copy link
Member

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 jaredpar added Feature Request and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 15, 2022
@jaredpar
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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 RikkiGibson self-assigned this Aug 9, 2022
@RikkiGibson
Copy link
Contributor

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

@RikkiGibson RikkiGibson added this to the Compiler.Next milestone Aug 9, 2022
@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants