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

Should language versioning require opting in to non-breaking changes? #286

Open
leafpetersen opened this issue Mar 22, 2019 · 7 comments
Open
Assignees

Comments

@leafpetersen
Copy link
Member

leafpetersen commented Mar 22, 2019

The strawman proposal for per library versioning says

Each package available to a program will have its own language version level associated (most likely specified in the .packages file). All libraries in that package will use that associated language level unless it declares a different level explicitly in the library.

The pub tool configures this default language level for a package based on its SDK dependency. If a package requires an SDK of ^2.2.0, it will default to the 2.2 language level.

This means that a package cannot use features of a new SDK release without depending on that SDK release.

Is this the right choice for non-breaking changes?

On the plus side, if a package uses syntax which is not supported by the min SDK constraint, it should in fact get an error/warning.

On the minus side, this means that a package which does not use any new syntax will nonetheless start getting deprecation warnings when the supported lifespan of old language versions approaches, and then eventually will be rejected. The fix is easy: just bump the min SDK constraint forward. But it seems bad that code that is perfectly fine suffers from unnecessary procedural bit rot.

Perhaps we need to distinguish between "I am pinning myself to language version X" vs "I am compatible with everything from X to Y", where Y is the next breaking version release after X?

cc @lrhn @eernstg @munificent

@lrhn
Copy link
Member

lrhn commented Mar 25, 2019

If a package uses syntax which is not supported by the minimal required SDK version, then it should definitely get an error/warning. We have already seen problems with packages that use non-breaking features without updating the SDK requirement. It "works" while devloping on a new SDK, but when run on an older SDK, things just break.
So, yes, you should most definitely need to opt in to non-breaking changes by requiring an SDK/language version supporting those changes.

If a package uses no new features, and no features which has changed meaning (which is unlikely past a large breaking change like NNBD), then there is no need to show deprecation warnings for it.
The deprecation warnings could perhaps just be shown for features that are going out of support, not versions.

Staying on Dart 2.2 and not using control-flow collections should not be an issue. We just won't be deprecating version 2.2 separately from version 2.3. At some point, long past NNBD (let's callt that version 2.6), we will deprecate all versions prior to 2.6. That's when we stop supporting the nullable-by-default semantics entirely.

In short: There is no need to deprecate 2.2 before 2.3 when the only change between them is a non-breaking incremental change, and if we don't,there won't be any unnecessary deprecation warnings. You can stay on 2.2 until we force you to NNBD.

@leafpetersen
Copy link
Member Author

If a package uses syntax which is not supported by the minimal required SDK version, then it should definitely get an error/warning.

Agreed, but this doesn't have to be done via language versioning.

So, yes, you should most definitely need to opt in to non-breaking changes by requiring an SDK/language version supporting those changes.

I read what you're saying as the following. We have two problems that we would like to solve:

  • Packages using features that are not supported by their min SDK
  • A desire to ship opt in breaking changes

This feature definitely needs to address the second. You are proposing that it also address the first. This seems broadly reasonable, but I am concerned about the implementation cost. We can address the first issue relatively cheaply by building support for it into just one of the tools (e.g. pub, or the analyzer). The implications of doing this via language versioning is that every tool needs to support every incremental version of the language, even when changes between versions are not breaking. So every time we add a feature or behavior, every tool needs to be support a mode in which all features up to that one are supported, but not that one or later. Mostly this means the two front ends, but in principle there could be runtime behaviors as well.

We should be sure we're prepared for this if that's what we want to do.

@munificent
Copy link
Member

The implications of doing this via language versioning is that every tool needs to support every incremental version of the language, even when changes between versions are not breaking.

One option is to state that any version of the Dart SDK that does not contain any opt-in language changes should be shipped as a point release (1.2.3) and not a minor version (1.2.3). Then, since the language versioning stuff here only covers minor versions, point releases would slide through automatically with no opt-in support needed.

@leafpetersen
Copy link
Member Author

Then, since the language versioning stuff here only covers minor versions, point releases would slide through automatically with no opt-in support needed.

I don't think this changes anything. We don't need to change the versioning to accomplish this, and doing this is exactly contrary to what @lrhn intends here. He wants this to apply to non-breaking changes, for the reasons he outlines above.

@lrhn
Copy link
Member

lrhn commented Mar 26, 2019

I want to prevent you from using a feature that your minimum required SDK does not support.
I want this independently of breaking changes. I admit I have probably taken that as the base behavior of our tool-chain. It's independent of breaking changes, and not tightly coupled to this feature.

We can say that non-breaking features are enabled by the SDK constraint alone, and are unaffected by libraries opting down to a lower language level.
If you, say, require SDK 2.6, which enables underscores in number literals, and one library opts down to 2.4 (down past 2.5 which is the breaking NNBD change), then we can allow that library to use underscores in number literals too. After all, the entire program will never run on an SDK without that feature.

I worry about the combinatoric implementation complexity of that approach, having a later feature enabled while an earlier breaking feature is still disabled. We'll basically have to combine all new non-breaking features with all previous supported breaking-change language levels, and ensure that they work.
If instead we only allow new features to work on top of all earlier breaking changes, that reduces the combinatoric complexity to single chain of incremental changes. Maybe it's not important, maybe the breaking/non-breaking changes are sufficiently orthogonal to make this a non-issue, and maybe our implementors can handle it, but I'd prefer to play this safe.

And it encourages people to upgrade to newer SDKs.

An option is to only have specific levels that you can opt down to, mainly "just before breaking change X". If you require SDK 2.6, which only includes non-breaking changes, there is no reason to opt
back to SDK 2.5. We should probably even prevent you from trying because it's likely a mistake.

(There is also potentially a situation where a change is non-breaking in relation to a previous breaking change, say something that is non-breaking on top of NNBD, but which would be breaking without NNBD. Such a change is not independent of the breaking change, and shouldn't be available without opting in to NNBD. In other words "non-breaking" is a relative term, not an absolute.)

@lrhn
Copy link
Member

lrhn commented Jun 19, 2019

Decision: Non breaking changes should be guarded by language versioning.

A non-breaking language feature introduces new syntax, or new behavior for existing syntax which would otherwise be a compile- or run-time error.
It does not change the semantics of any existing valid syntax, or any existing valid execution.

Since the language feature is non-breaking, compilers need not disable the parsing, they can parse and understand the syntax as if the feature was enabled, then report a single compile-time error to the user for using the feature.

Non-breaking language features introduced after version 2.0.0 and prior to the language versioning feature itself are not required to be recongized. Compilers may assume that all of these features were available since version 2.0.0. They are allowed to recognize the correct version as well, and reject code using features not supported by their SDK constraint.

  • 2.1: double literals, super-mixins
  • 2.2: set literals
  • 2.3: control flow collections
  • 2.4: no new feature

Specifying a downgrade marker, like // @dart=2.2, to a lower version than where language versioning feature was introduced (possibly 2.5.0) need not be supported. That version may be taken to mean 2.5 instead since it requires 2.5.0 to even use the feature.

@jakemac53
Copy link
Contributor

jakemac53 commented Aug 20, 2020

Note that suggested versions listed above for at least some of these features are all breaking changes, previously they required a language version one minor version lower.

The logic for something released in x.y.z would give it a language version of x.y previously.

We should leave these old features at their previous language versions imo, it isn't technically correct but it does retain the previous behavior and avoid breaking packages.

See flutter/flutter#64220 as an example where flutter packages are broken by this.

If we do move forward this needs to be carefully evaluated and go through the full breaking change process.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 19, 2020
…able since 2.0.0

See dart-lang/language#286 (comment)
and #43462

Change-Id: Ib8c37a2b5e3ce6c737e496bf285d946baf3a41d3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163401
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 21, 2020
See dart-lang/language#286 (comment)
and #43462

Change-Id: Id844473e0cb371ffc4470a18993c66c8ad0427e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163503
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants