pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.29k stars 628 forks source link

[Call-by-name] Replaced `ast` with `libcst` in migration code #21089

Closed sureshjoshi closed 3 months ago

sureshjoshi commented 3 months ago

This PR addresses several ast-centric implementation problems. Using a concrete syntax tree allows us to do fancier migrations, and not have to write code to handle silly things like trailing commas or nested comments.

High level, libcst is amazing. All of the most complex code in the migration was replaced by a better transformer.

The implicitly work done in this PR will eventually get pulled out into a custom linter/type-checker, as outside of this migration - we'll want to flag when users add new code with unnecessary implicitly usage.

Closes #21040

sureshjoshi commented 3 months ago

In subsequent PRs (or, as part of subsequent migrations), I'll add some more implicitly rules and clean up some of the code. This is a visually a big change, that I didn't want to get much bigger.

Overall, less code, more functionality

sureshjoshi commented 3 months ago

I read through it, but my understanding of the call by name conversion is superficial, so someone else needs to review this in more depth. From what I can tell, LGTM.

FWIW the end result should be "same as before, except with some bugs fixed". Most of the re-factoring was what was required to switch from AST to libcst. The only "novelty" was one implicitly optimization (removed when the called function requires args)

sureshjoshi commented 3 months ago

Pulling these from source comments to main:

do we want to guard this from leaking into pants runtime (beyond the migration code)?

I think libcst is an excellent utility for migrations, of which this will not be the last. Allowing broader use of it seems like a plus to me. So I don't think we should restrict this dependency with dependency visibility rules.

I agree :)

so it would be good to have a clear record on the motivation for each dependency and it's intended use

I also agree :)

The motivation here is that libcst will be used in some implicitly checks and in the upcoming migrate goal I'm writing (or migrate fix-er`), so it'll end up in source code regardless.

I agree, overall, that we would like to reduce dependencies - or visibility them out.

I wonder if pex's new PEP723 support (or __visibility__) will help for those places where we have one-off build files, if they're not already hidden away from the rest of the code.