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

Make deprecated APIs expire based on language version. #1075

Open
lrhn opened this issue Jul 8, 2020 · 8 comments
Open

Make deprecated APIs expire based on language version. #1075

lrhn opened this issue Jul 8, 2020 · 8 comments
Labels
feature Proposed language feature that solves one or more problems

Comments

@lrhn
Copy link
Member

lrhn commented Jul 8, 2020

Currently the Dart @Deprecated(...) or @deprecated annotations only trigger warnings.
It's possible to say when the feature will be removed in the message, but it's still going to be a hard breaking change when that happens because existing code will stop working.

Deprecation is a two-step process: 1) Mark as deprecated and 2) remove entirely.

There is no gradually increase in motivation to remove usages of the deprecated member, it goes directly from "want to avoid warnings" to "program doesn't run". That makes some projects turn deprecation warnings into errors, which means that merely marking something as deprecated will break those projects. That loses the point of deprecating before removing: to not make it a sudden breaking change

We could introduce a third, intermediate, stage, "discontinued", where the API stops working for all code running above a certain language version. This is a kind of "library versioning", but since all it does is remove access, it's much simpler than having different members for different language versions.

In short: Add a language version field to Deprecated, and refuse access to the deprecated feature from code compiled at that language version or above. Example:

@Deprecated("Doesn't work any more", discontinued: "2.11")
void foo() { ... }

Code running at language version 2.11 or above will not be allowed to call foo. Code running at language version 2.10 or below can keep using it, and will still receive deprecation warnings.

This forces users to address the deprecation warning at a point where they are also addressing other changes required by the SDK, without breaking code that stays on the older version. It allows users to gradually migrate, while still enforcing an eventual discontinuation.
The feature can then be removed finally from the SDK when we stop supporting the older language versions, so for the above, when we stop supporting language version 2.10, we also remove the feature finally. (We can remove it before using the normal breaking change process, if we think the impact will be small enough.)

The @Deprecated annotation has some intricate rules around inheritance and multiple paths to a declaration or member signature. It can roughly be summarized as: An API feature use is considered deprecated if removing all deprecated declarations/parameters would change the meaning of the use.

In the same way, an API feature use is considered discontinued if removing all declarations/parameters which are deprecated and discontinued at the current language version or below, would change the meaning of the use.

If a declaration is imported more than once, then its use is deprecated only if it's deprecated along all import paths (which it might not be if the deprecation comes from a deprecated import or export declaration, rather than the declaration itself being deprecated, and the declaration also being imported non-deprecated).
An instance member parameter is deprecated if there is not a non-deprecated occurrence of it in any superinterface of the type (and it would be bad style to deprecate a parameter in a class if it's not deprecated in a superinterface, because then it can't be removed).

In the same way, an API feature use is considered discontinued in a language version if it's removed in that language version along all imports paths/in all superinterface members.

This approach can only disallow access, it doesn't actually remove anything, which means that:

  • It's not possible to discontinue a required parameter gradually. It's still there, and still required, no matter which language version. The language version must be ignored for required parameters.
  • It's not possible to discontinue an optional positional parameter unless all later positional parameters are discontinued too. You can't not pass a value for it and still pass an argument for a later parameter.
  • It's not possible to introduce a new, different member or parameter with the same name as a discontinued member. It's still there and blocking the namespace.
  • We can't block dynamic invocations from using a deprecated member or parameter.

These are really the same rules as for any deprecation, the feature is still there, we just go from "you're not supposed to use it" to "you're not allowed to use it".

This can be made a language feature, and block access in all compilers.

Alternatively, it can be made an Analyzer-only feature, one where violations default to being an error. That would be consistent with all other @Deprecated features being Analyzer-only. It would mean that if you skip the analyzer, you can keep using deprecated members after their expiry date.

@natebosch

Ob-feature-request: We can't actually make it Deprecated("message", discontinued: "2.11") because the positional parameter is already optional. (#1076)

@lrhn lrhn added the feature Proposed language feature that solves one or more problems label Jul 8, 2020
@leafpetersen
Copy link
Member

There is a related proposal which I've made before, which is to allow APIs to be replaced based on the language version. That is, instead of/in addition to deprecating, you put in a replacement:

class C {
  @Versioned(until: "2.11");
  void foo(int x) {}
  @Versioned(startingFrom: "2.11");
  void foo(String x) {}
}

The semantics is essentially given by saying that this introduces two symbols, mangled uniquely by language version, which are members of C. The static and dynamic version of a symbol that you see is entirely governed by what your current language version is. So if you are in a library that has language version 2.10, then static analysis sees the first version, and a dynamic call will reach the first version. If you are in a library that has language version 2.11, then similarly for the second version.

Just something to think about.

@leafpetersen
Copy link
Member

Is tying discontinuation in packages to the language version still the right thing to do? Should it be instead possible to tie the discontinuation to the package version?

@lrhn
Copy link
Member Author

lrhn commented Jul 9, 2020

If we allow the feature in packages, then it's definitely going to be an analyzer feature, not a language feature.
The language has no idea what a "package version" is. To the language a (Dart) package only has a name, and whatever code it gets is what that Dart package is. It's a pub issue to figure out which version of a Pub package to include in the program as the package of that name. (IOW: Dart packages don't have versions, Pub packages do, and Pub's job is to find a Pub package version to make available as a Dart package).

For the @Versioned API, we'd need different approaches for static and virtual accesses. Static accesses are simple, we just resolve to the correct version based on the requesting code. Virtual accesses are more complicated, especially because we have dynamic invocations. For those to work, we'd have to mangle all names that are versioned anywhere, even in classes where they are not versioned. A dynamicValue.foo needs to work against both versioned and non-versioned foo members, so even the non-versioned ones would have to exist in (at least) two versions. Global analysis could probably reduce the necessary duplication.
If we use V-tables, we'd probably just have a V-table per language version which has code of that version occurring in the program. If a class is not versioned, all the V-tables of the class can reuse the same table.

It's not impossible, but I fear the details will be draconic.

@leafpetersen
Copy link
Member

The language has no idea what a "package version" is.

This is a thing which could be changed. I'm quite interested in getting a primitive notion of compilation unit into the language, should this be something we think about as part of it?

@lrhn
Copy link
Member Author

lrhn commented Jul 9, 2020

We could easily assign a "version" to any package, by putting "version": "1.2.3" entries into package_config.json.
It hasn't had any useful effect so far, so we didn't consider it.

It's not something I expect to reify at run-time either (the entire package_config.json file is a compile-time artifact, except when running on the VM where Platform.packageConfig and Isolate.resolvePackageUri leak the source layout. Those probably won't work when compiling AoT anyway.

At compile-time it could be used for deprecation warnings, like described here, or it could be used for API versioning, if we are very, very bold and allow API versioning in complete generality. I'm not sure the general user will be able to bear the complexity of having multiple different APIs at the same time.

Not sure how versioning is related to compilation units, though.

@leafpetersen
Copy link
Member

If we use the language version for versioning, is it going to be confusing to package authors? I don't believe our current model introduces a new language version for every SDK version, so (for example) 2.9 and 2.8 may be the same language version. So a feature marked as discontinued in 2.8 might still be visible since 2.8 and 2.9 are the same language version?

@lrhn
Copy link
Member Author

lrhn commented Jul 10, 2020

I'd expect 2.8 and 2.9 to be different language versions, even if they are the same language. That is, there are not new language features enabled in 2.9.
I don't think we want to define an official set of language versions that are the only ones you can write. Every dotted value of the form x.y is a language version.
What we define is instead for each feature, in which language version was it introduced. You can write any x.y, and if it's below the introduction point of a feature, you can't use it.

So you would get no benefit from writing //@dart=2.8 if your default version is 2.9 (except perhaps opting out of experiments while 2.9 is the newest version), but trying to make a fuzz out of someone writing 2.8 is not worth it.

Using the language version for other API/SDK changes really highlights that it actually is an SDK version, and that we have just promised to only change the language in minor-version SDK increments. (And the SDK APIs too)

@natebosch
Copy link
Member

I'm also nervous about tying this in to pub package versions.

If we go with something like a @Versioned annotation we could keep it in the SDK and not have to worry about what it would mean for it to get applied on packages at all. The @Deprecated annotation could remain unchanged, and we'd know during code review (or we could potentially enforce it mechanically) that a @Versioned(until:) should only be used on @Deprecated elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems
Projects
None yet
Development

No branches or pull requests

3 participants