run-make-support: use a CommandWrapper trait instead of impl_helper_methods macro

[jieyouxu]

impl_helper_methods worked reasonably well while prototyping, but now I realized a trait with default provided methods is a better fit for making it easier and less verbose to implement command wrappers. This is because command_output can be a required method checked by rustc, while we can provide helper methods but still allow individual command wrapper instances to override as desired.

pub trait CommandWrapper {
    // required method
    fn command_output(self) -> std::command::Output;

    // provided helper methods
    fn run(self) -> std::command::Output { ... }
    fn run_fail(self, expected_exit_code: i32) -> std::command::Output { ... }
}

then implementors can write

pub struct Rustc { cmd: Command }

impl CommandWrapper for Rustc {
    fn command_output(self) -> Output { ... }
}

impl Rustc {
    // ... other helper methods
}

Unresolved questions

• If these helper methods are moved on to a trait, then test writers will have to bring the trait in scope in order to use the helper methods. Is it worth the extra typing?

[jieyouxu]

Drawback: test writer would need to bring the trait into scope to use the helper methods...

[ChrisDenton]

There could be inherent methods that simply forward to the trait methods. That's more boilerplate though.

[Kobzol]

We don't need command_output on each wrapper struct anymore as of #126121. I don't think that a trait pulls its weight here, due the tests having to import it (it's a bit annoying since run-make tests are typically not very friendly with IDEs). It would be nice to remove the macro, but we can do that also with other approaches than a trait (e.g. Deref).

[Kobzol]

FWIW, I tried to remove the macro by implementing Deref/DerefMut<Target=Command> for the wrappers instead. It would have worked.. were it not for the builder pattern :) Because if we implement the helper functions (like env) on Command directly, then they have to return &mut Self, which means that you would have to use all of the top-level command wrapper methods before using any of the Command methods, which is annoying. For example, rustc().crate_type().env() would work, but rustc().env().crate_type() wouldn't work, because env() would return Command, and not Rustc.

I'm not aware of any way of resolving this without using a macro. That being said, I think that the current macro is fine, it just adds a bunch of methods, and does not have any specific requirements on the wrapper apart from having a cmd field. The documentation needs to be updated though (#126188).

[jieyouxu]

I'm going to close this issue, because I think using a trait here unfortunately just makes the API worse.