Skip to content

Implement "experiment release versions" #43031

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

Closed
7 tasks done
jakemac53 opened this issue Aug 12, 2020 · 21 comments
Closed
7 tasks done

Implement "experiment release versions" #43031

jakemac53 opened this issue Aug 12, 2020 · 21 comments
Assignees
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Aug 12, 2020

General overview

Today, experiments are only enabled for a library if it is on the "current language version". This causes problems when the SDK depends on packages which enable experiments, as it creates a chicken and egg situation.

The planned fix is to add a feature which enables experiments to optionally be given a “release version” which becomes the language version which serves as the opt-in for that feature, instead of the current language version.

We will use this feature to mark 2.10 (or possibly 2.9 cc @leafpetersen ?) as the experiment release version for the null safety experiment, and prevent a repeat of what happened in the 2.9->2.10 transition.

Implementation details

Add an experimentLanguageVersion field to experiment configuration

An additional optional field is to be added to the entries in the features section of the experimental_features.yaml file. This field will be called experimentLanguageVersion, and will accept a language version string.

If present, then the specified language version will be the required version to enable the feature, instead of the "current" version which is the default. This only takes effect in the presence of the --enable-experiment=<experiment> flag, or an allow list entry that enables the experiment for the library.

Note that the feature must continue to be enabled with a combination of the experiment flag and corresponding experimental release version even after the feature ships. This allows us to transition smoothly from the experiment to a stable release. In some subsequent stable sdk the experimentLanguageVersion field may however be removed, at which point the experiment flag will no longer have any effect, and packages will have to migrate to the language version in which the feature was released.

Both the analyzer and cfe will need to support this new configuration.

See go/experiments-crossing-stable-releases for the full discussion (internal only doc).

Ship the experiments file with the sdk

See #43040, some external tools will need access to this now.

We decided we don't need to do this.

Language tests

@jakemac53 will add some language tests to cover the behavior change (to whatever extent is feasible)

Publish a new analyzer version with the changes

Once the feature has been implemented, we need to publish a version of the analyzer, ideally well in advance of the first 2.11 dev release of the sdk.

Other tool updates

Some other tools such as pub, pana, and build_resolvers also have knowledge of experiments and what language versions are required for them to be enabled. These will have to be updated to support the new release version option.

Checklist

@jakemac53 jakemac53 added legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures labels Aug 12, 2020
@jakemac53
Copy link
Contributor Author

Marking as a P1 because this will be a blocker for any 2.11 release

@jakemac53
Copy link
Contributor Author

Note also that we had not discussed concretely the naming of the new field, I called it experimentLanguageVersion in the plan above, if somebody feels strongly about that please bring up those concerns/ideas sooner rather than later :D.

@jakemac53 jakemac53 removed legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. labels Aug 12, 2020
@jakemac53 jakemac53 added this to the September Release 2020 milestone Aug 12, 2020
@mit-mit mit-mit added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Aug 13, 2020
@jakemac53
Copy link
Contributor Author

Note that as far as I can tell today we do not ship the experimental_features.yaml file with the sdk, so I believe we will also need to do that for tools outside of the sdk to consume (and the analyzer may also want to read this instead of using its generated dart file).

cc @jonasfj @scheglov @sortie

@pcsosinski
Copy link

does this work need to land prior to the 2.10 build going to stable, or just before we bump to 2.11?

@jakemac53
Copy link
Contributor Author

This needs to land before we bump to 2.11 which generally happens well before the previous stable release nowadays afaik.

@pcsosinski
Copy link

ah sorry I should have said "before 2.10 branches," which would mean we need all code submitted prior to that point (week of sept 7). it sounds like that is not the case, but pretty close. do you have any concerns with getting this work landed in the next 2 weeks?

@leafpetersen
Copy link
Member

Expanding on the comment from @pcsosinski , I believe it is the case that we do not need this to be actually in the 2.10 release, since 2.10 will treat 2.10 as the language version for null safety anyway. That is, if necessary, we could branch 2.10 before this lands, but delay bumping the minor version in master (e.g. by bumping to 2.10.10-dev.0 instead). @jakemac53 do you agree or do we need to make this a release blocker for 2.10?

@jakemac53
Copy link
Contributor Author

Yes I agree @leafpetersen - this only has to land before the minor version switch, whenever that is.

@franklinyow franklinyow added the type-enhancement A request for a change that isn't a bug label Aug 26, 2020
@franklinyow
Copy link
Contributor

Are we target to finish everything on Sept. Release? If not I will remove the P1 label and the Sept milestone

@jakemac53
Copy link
Contributor Author

Well, I have no idea what Sept. Release means. This must get done before any 2.11 releases though or else it is all kind of pointless.

@pcsosinski
Copy link

last we discussed w/ @leafpetersen the hope was to have this done before the Sept branch cut (meaning done by end of next week, Sept 3rd). @jakemac53 do you think we're on track for that?

@jakemac53
Copy link
Contributor Author

Yes, I think we are on track. It sounds like the core work is done. The remaining work is the language test which I can do asap, and the package related things which don't have to be done before the branch is cut (but also should not take long).

@scheglov any idea when this would land in an analyzer release? (note I am not asking for a release this moment)

@scheglov
Copy link
Contributor

I can release 0.40.1 when necessary.
Hopefully after the language tests are done, and we checked that the analyzer passes them.

There is a refactoring going on in the analyzer, but I guess I could release a version before this refactoring started if necessary.

@jakemac53
Copy link
Contributor Author

Hopefully after the language tests are done, and we checked that the analyzer passes them.

Note that the language tests can't really do anything useful here until 2.11 - but I was planning on manually doing the 2.11 switch locally and running them.

There is a refactoring going on in the analyzer, but I guess I could release a version before this refactoring started if necessary.

This is not super urgent - the first external users who will need it will be the 2.11 dev sdk users which is likely still a ways off.

@leafpetersen
Copy link
Member

So am I correct in understanding that all of the work that needs to happen in the SDK (that is, that would block the branch cut) has been completed?

@jakemac53
Copy link
Contributor Author

jakemac53 commented Sep 1, 2020

I have a test cl out here https://dart-review.googlesource.com/c/sdk/+/161342 to run on the bots as a way of confirming we will be good to switch to 2.11.

It does look to me like we might have one outstanding bug there though where the experiment release version didn't get properly copied over in the generated code for the cfe. actually I take it back I was just confused about the expectation here I believe.

@jakemac53
Copy link
Contributor Author

@johnniwinther
Copy link
Member

I'll take a look at the CFE failures on https://dart-review.googlesource.com/c/sdk/+/161342

@johnniwinther
Copy link
Member

A fix for the (unintended) CFE failures is in progress: https://dart-review.googlesource.com/c/sdk/+/161503

@jakemac53
Copy link
Contributor Author

I have confirmed that cl (https://dart-review.googlesource.com/c/sdk/+/161503) does in fact resolve the cfe issues - there are only 2 minor failures when switching the version now which look unrelated.

@franklinyow
Copy link
Contributor

All necessary work done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants