bootstrap: Refactor and rename a bunch of path handling

[jyn514]

This PR looks really scary BUT I promise that there's not actually many changes. Almost all of them are renames and the rest are just moving code around.

See https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Path.20filters.20in.20bootstrap/with/571240765 for background on why I'm making these changes, and https://hackmd.io/9J4UKJPBQOSwHr5WXQac0Q?edit for a sketch of what I think the long-term plan should be for path handling.

I highly highly recommend reviewing this commit by commit with --ignore-all-space --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change.

[rustbot]

r? @Zalathar

rustbot has assigned @Zalathar.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jyn514]

and FWIW I'm happy to split out changes, most of these I did with LSP assistance so they won't take me long to redo.

[Zalathar]

I think some of these renames are straightforwardly good, and some will require a bit more explanation/haggling.

I'll try to find an uncontroversial subset that we can land easily, and then we can concentrate on what's left.

[Zalathar]

E.g. a thing I'm concerned about is that some of these new names are “aspirational” but not actually accurate to how things currently work, so they'd end up being misleading.

[Zalathar]

So far these are the parts I think are fairly straightforward:

• Unnecessary uses of paths for codgen backends
• cli_paths → selectors (but with updated module docs to explain that “selector” is our term for positional CLI args that tell bootstrap what parts to check/build/test etc.)
• Suite → TestSuite (but not Set → Alias)
• rust-analyzer-proc-macro-srv (but with some snapshot tests, and an explanation of what the alias is for)

[Zalathar]

I remember being reluctant to change this in the past, because I wasn't entirely sure what special behaviour it enables, and whether that behaviour is important.

I would certainly like to get rid of paths, but I would like to either be cautious about it, or acknowledge that we're going to YOLO this change and that rustdoc folks will have to tell us if they notice problems.

[Zalathar]

Also, IIRC this case is the primary reason why PathSet::Set holds a BTreeSet<TaskPath> instead of just a singular path (selector?).

So if we do get rid of it, I would ideally want to get rid of the BTreeSet at the same time.

(An empty BTreeSet is also used to represent PathSet::empty, but we can probably use a separate enum variant for that case, and in the long run that variant will hopefully disappear if we split off user-visible steps from dependency nodes.)

[jyn514]

Let me talk about paths() a bit.

The original intent @Mark-Simulacrum had when he introduced PathSet, to my knowledge, was that multiple paths could be aliases for the same step. That's what rustdoc is doing here; both paths here run exactly the same Step, regardless of whether one or both are present.

That never really caught on. To my knowledge, this is the only usage of paths() there's ever been.

Later, in #95503, I repurposed PathSet to mean "each crate in this set should be passed to Step::make_run in RunConfig". That was not the previous meaning.

Rustdoc never looks at run.paths in make_run, so it's safe to just treat it as an alias, like elsewhere in bootstrap.

[Zalathar]

I think I'd be willing to take a separate PR that does try to YOLO get rid of paths, because if we can get away with it then that's a nice win.

[Zalathar]

(There might be some subtle differences in handling of things like --skip, but we should probably consider that sort of edge-case thing a lost cause until selector/step handling is in good enough shape that we can just have a sensible and understandable implementation.)

[Mark-Simulacrum]

Yeah, I think that reasoning is accurate. It was added in f104b12 (back in 2018!).

As that commit notes:

However, this meant that some Step specifications needed to be updated, since for example rustdoc can be built by ./x.py build src/librustdoc or ./x.py build src/tools/rustdoc. A PathSet abstraction is added that handles this: now, each Step can not only list path(...) but also paths(&[a, b, ...]) which will make it so that we don't invoke it with each of the individual paths, instead invoking it with the first path in the list (though this shouldn't be depended on).

It seems fine to me to remove support for aliases.

[Zalathar]

@rustbot author

[Kobzol]

Renaming some of these things sounds like a good idea, but I'm not sure if all those changes are worth the churn, especially if we plan to actually rewrite how path handling works, during which I assume we will make large changes to it, which might cause additional renames.

But apart from the CargoCommand and Alias change the new names look reasonable to me.

View changes since this review

[jyn514]

ok, I have removed the Set -> Alias and Kind -> CargoSubcommand renames. I've also split out the paths removal, I'll put that in a separate PR.

[rust-bors]

☔ The latest upstream changes (presumably #152230) made this pull request unmergeable. Please resolve the merge conflicts.