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

Add APIs for using analyzer element model 2. #3775

Merged
merged 2 commits into from
Dec 14, 2024

Conversation

scheglov
Copy link
Contributor

@scheglov scheglov commented Dec 7, 2024

See https://docs.google.com/presentation/d/1Vxr-y5ljuo0Os78R4tKZg8cc0In984EQcLs06HOqS28/edit#slide=id.p for the new element model details. It is possible that you will not able access it, unless you work for Google.

This change is green in google3.

It looks that there are issues with dependencies and versions, and possible circular or chained dependencies.
And I don't know how to deal with these issues.

However google3 uses the version of analyzer that is newer than 7.0.0, so it is possible that 7.0.0 is not enough.
But because the the version issues mentioned above, I don't see, don't get to the place where it can be checked yet.
I have analyzer 7.1.0 publishing in flight, in case it is will be necessary.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

github-actions bot commented Dec 7, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

@scheglov scheglov requested a review from jakemac53 December 7, 2024 22:47
@kevmoo
Copy link
Member

kevmoo commented Dec 9, 2024

How should we deal with #3774 here

Merge the effort?

@jakemac53
Copy link
Contributor

I would really like to avoid propagating all the numbered variants of the APIs here, it implies a future breaking change to remove them, and build doesn't do many breaking changes (not since March 2021).

What is the plan for getting rid of the old APIs here? Or do we plan to keep them around for some time?

@jakemac53
Copy link
Contributor

cc @natebosch in case you have any ideas for how to mitigate the maintenance/versioning costs here...

@natebosch
Copy link
Member

I can't think of a way around a breaking version change here.

I'll spend some more time thinking of how we can most smoothly roll this out, but my initial thought is that it might bet better to separate the interface at a higher level than per-method. We should consider adding a Resolver2 (or same name but different import?) with the new APIs so that it's easier to avoid unintentionally mixing use. Having both available and neither is @deprecated could be confusing.

@scheglov
Copy link
Contributor Author

scheglov commented Dec 9, 2024

Note, this change is a split-out of https://critique.corp.google.com/cl/698854756 where I migrated many clients of package:build and package:source_gen. And alternative way to do package:build API changes will require some rework on these pending google3 changes. I had to do this, to prove myself that everything works at the end. How do you usually plan changes like this? Do you also make google3 changes together with package:build changes? Do you plan doing something similar with these changes?

The old APIs in the analyzer will be removed after all the clients migrated.

@jakemac53
Copy link
Contributor

jakemac53 commented Dec 11, 2024

In most cases, we would just do breaking changes externally (without publishing), then merge it into google3 (fixing up clients) before publishing. Externally we just expect version solving to solve this problem and don't expend a lot of effort to allow incremental adoption of new APIs.

This is harder though, when the change needs to trickle through many packages, such as the analyzer case.

However, I think it is probably feasible in this case. We still end up needing a breaking change it sounds like, but we don't need to introduce the new variant and then delete the old one, we can just do a single atomic swap of the types.

We would update the internal packages to be compatible with the new APIs and land that at the same time as rolling in the new package:build APIs. Then once that succeeds, we publish package:build.

@jakemac53
Copy link
Contributor

@scheglov let me know if you want to just have a meeting to discuss

Copy link
Contributor

@jakemac53 jakemac53 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @scheglov , we plan to have these apis temporarily just for the internal migration, but never to publish them (if we can avoid it). We will do an immediate swap to the new APIs as a breaking change, once the migration is complete.

@scheglov scheglov merged commit 1865a35 into dart-lang:master Dec 14, 2024
69 of 76 checks passed
jakemac53 added a commit that referenced this pull request Dec 16, 2024
jakemac53 added a commit that referenced this pull request Dec 16, 2024
This reverts commit fbdfb67.

I have forked the repo prior to this revert here https://github.com/dart-lang/build/tree/resolver-2-methods.

The plan is to use that fork internally until all builders are updated to the new APIs. Then we will do a breaking change in this repo to migrate to all the new APIs. We do not plan to publish ever the *2 APIs on pub.

In the short term, we will update our analyzer upper constraints across all packages but not add these new methods or expose the new APIs. Those versions will be published (soon ish) to unblock other work (such as updating to the latest dart_style).
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

Successfully merging this pull request may close these issues.

4 participants