Call-by-name syntax migration goal

[sureshjoshi]

This PR creates a native Python built-in goal which performs the call-by-name migration for passed in files/targets (covers items 1 and 4 in #20572).

Note: This is not a completed feature, as per the "split off tickets" below.

Outstanding items in progress:

• Add goal_rule to migrations
• Support Get calls using dictionaries
• Split off to [Call-by-name] Handle functools.partial migrations #20744
• Split off to [Call-by-name] Investigate why so many intrinsic migrations are missing (e.g. Digest code) #20745
• Optimize single-parameter Get calls as per Stu's comment in Create migration tool for call-by-name syntax #20572
• Re-name MultiGet to concurrently (review what else is required here)
• Ensure explicitly excluded migrations are logged to the console
• Consolidate and cleanup code now that integration tests are present

Out of scope of this PR, but migrate-call-by-name isn't a great name. This could be a good jump off point for a generic migrate goal with options/sub-goals (ala Swift's or Svelte's native migration tools).

See #20572

[sureshjoshi]

@benjyw Two items here I'd like some direction on, if you get a chance:

Re-name MultiGet to concurrently (review what else is required here)

Haven't touched anything related to MultiGet, since I know it'll be churned with concurrently - so what's the plan here?

Is concurrently intended to be an alias for MultiGet until we eventually deprecate the Get based syntax someday? Or is/will it be a different way to collect/call?

My naive assumption is that it's still just a co-routine collector, same as MultiGet with a shiny new name.

Investigate why so many intrinsic migrations are missing (e.g. Digest code)

I'm not entirely sure where to even start here. There are a bunch of calls that just don't even register in the Rust rule graph call-by-syntax mapper. I think an example I gave was:
Get(Digest, RemovePrefix(result.output_digest, REPORT_DIR))

In the associated issue, Stu mentioned:

Benjy gave all of the native rules names: if they're not already exposed from the native engine, he might be able to help with exposing them.

Do you have any idea what is going on with this one?

[benjyw]

Re names - yes, all rules should have names now. Context: #19755

Is something not working vis-a-vis the intrinsics names?

See src/rust/engine/src/intrinsics.rs specifically for those names

[benjyw]

Re MultiGet: IIUC it should be renamed concurrently and take rule names, instead of Gets. Right?

[sureshjoshi]

Is something not working vis-a-vis the intrinsics names?

See src/rust/engine/src/intrinsics.rs specifically for those names

Yeah, so the generated rule graph isn't showing a handful of called-by-name functions. So, the example of Get(Digest, RemovePrefix(result.output_digest, REPORT_DIR)) wasn't being mapped to a function call.

But based on your comment, this set of intrinsics is where I should be looking? If so, perfect - having not dug through the rust code yet, I was at a bit of a loss.

    ...
    intrinsics.insert(
            Intrinsic::new(
                "directory_digest_to_digest_contents",
                types.digest_contents,
                types.directory_digest,
            ),
            Box::new(directory_digest_to_digest_contents),
        );
        intrinsics.insert(
            Intrinsic::new(
                "directory_digest_to_digest_entries",
                types.digest_entries,
                types.directory_digest,
            ),
            Box::new(directory_digest_to_digest_entries),
        );
    ...

[benjyw]

Yeah that is mapped to a rust function, not a python one.

[sureshjoshi]

@stuhood @benjyw @huonw @kaos

Added you all to the PR. I've got some lame tax paperwork to do for the couple days, but I wanted to start getting some eyeballs on this.

When I'm back, I'm going to work on some cleaning up and re-factoring of the code for a few things that bother me, and to see if I can find a cleaner way to satisfy the type checker other than the barrage of isinstance on AST items. Or at least consolidate them or something 🤷🏽

I've pulled off intrinsics and partials as separate tickets, as I think those could be rabbit hole investigations, and I didn't want to delay getting the 90% tool into a PR.

I'll be updating this shortly with an example of running the migrations on a Pants repo, so you can see what it looks like outside of integration tests.
Snapshot of the April 3rd YOLO branch (intentionally unformatted):
93157fd

It's worth noting there are LOTS of places that aren't migrated, as we're not doing a find/replace, but navigating through the AST. For example, code like digests.add(await Get(...)) isn't currently handled.

[sureshjoshi]

Embedded comments are wiped out by the AST. There are ways we can try to move this around at replacement time, but effort to value might not be there - especially as we ensure that the comment loses some context when we summarily move it.

I'd rather let the user know to either manually move/remove/replace the comment, or to extract the code with an embedded comment out. In the Pants repo, we have these cases specifically around Process calls with embedded Args

[stuhood]

Yea, that seems fine with me as long as the warning is rendered to remind people to look for the stripped comments in code review.

[stuhood]

Snapshot of the April 3rd YOLO branch (intentionally unformatted):
93157fd

That looks awesome: kudos!

Will review this weekend.

[stuhood]

This looks great: thanks so much @sureshjoshi!

[stuhood]

Suggested change

	The migration plan is a JSON representation of the rule graph, which is generated by the
	engine based on the active backends/rules in the project.
	Each item in the migration plan is a rule that contains the old `Get` syntax, the associated
	input/output types, and the new function to directly call. The migration plan can be dumped as
	JSON using the `--json` flag, which can be useful for debugging. For example:
	A JSON migration plan can also be emitted, which contains the old `Get` syntax, the associated
	input/output types, and the new function to directly call. The migration plan can be dumped as
	JSON using the `--json` flag, which can be useful for debugging. For example:

[stuhood]

FWIW: If the called rule doesn't take any arguments, you can skip the **implicitly(), and just call it as the_rule_to_call().

[sureshjoshi]

👍🏽

Ah, okay, I didn't know if there were any ambient arguments that could still be passed without an explicit Get arg.

[stuhood]

...oh. Hm. Good question. 😬

[stuhood]

This is a neat pattern!

[sureshjoshi]

walrus FTW

Though, the lack of AST inference is brutal - I need to extract some utility AST stuff to support our use cases - too much dupe code to make the type checker happy (but, the value/need in making the type checker happy with AST code is really high)

[stuhood]

I'm not seeing why there is a difference between map_long_form_get_to_new_syntax and map_short_form_get_to_new_syntax in terms of the output syntax: it's clear why the input is different, but the "if the called function only takes a single argument with a type that matches the argument" case applies to both of them I think.

[sureshjoshi]

Yeah, I updated the short-form syntax before pushing this PR, but didn't give much thought to the long-form at the time.

From the examples I've seen in the code, the "short-form" syntax calls the XYZRequest constructor, so replacing that is pretty simple.

But the long-form calls seem to have a wider variety of call patterns, and one of the original calls I tried to replace similar to the short-form ended up failing the type checker due to some missing context (but works when called using implicitly(dict)

I'll take a look back at this, to see if it was a runtime or just type check problem.

[stuhood]

AFAIK, any call to Get(..) should be replaceable to function_call(..): the only difference should be that before replacement you get out a Get[T], and after replacement you get out an Awaitable[T]... so it might break some type checking I suppose. But maybe it would leave you happier with this code?

[sureshjoshi]

Yeah, this is more about satisfying the type checker, as if the call is awaitable, we access a different part of the AST vs if it's just straight callable. This will be pulled out to a utility to at least make the primary code more linearly readable.

[stuhood]

Yea, that seems fine with me as long as the warning is rendered to remind people to look for the stripped comments in code review.