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

Create a RunCompiler builder to replace the run_compiler function #77286

Closed
bjorn3 opened this issue Sep 28, 2020 · 7 comments · Fixed by #77649
Closed

Create a RunCompiler builder to replace the run_compiler function #77286

bjorn3 opened this issue Sep 28, 2020 · 7 comments · Fixed by #77649
Labels
A-driver Area: rustc_driver that ties everything together into the `rustc` compiler C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Sep 28, 2020

This argument list is slowly getting out of hand. Maybe we should create a RunCompiler builder pattern struct that doesn't require most calls to use None, None, None?

Anyway, pre-existing, if you rather open an issue about it than fix it here, I'm fine with merging this PR

Originally posted by @oli-obk in #76474 (comment)

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 28, 2020

@rustbot modify labels: +A-driver +C-cleanup +T-compiler

@rustbot rustbot added A-driver Area: rustc_driver that ties everything together into the `rustc` compiler C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 28, 2020
@oli-obk oli-obk added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Sep 28, 2020
@tsurai
Copy link
Contributor

tsurai commented Sep 28, 2020

I would like to give this a try if mentoring is available

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2020

I'll be happy to mentor, but we should wait for #76474 to get merged, because any change you do will conflict with that PR. Though you can start by basing your change on top of that PR.

You will need to modify each place that is modified in that PR, so keep it bookmarked and look at it whenever you end up wondering what you may need to touch.

The root change to do is to make

pub fn run_compiler(
private and add a new struct right next to it. That new struct should have a field for every parameter of the function and an inherent method (run) that just forwards all these fields to run_compiler. You'll need a new method that takes only the non-Option fields as parameters and creates a new instance of your builder struct with those fileds initialized and the rest set to None. Then you'll need a method for each Option field to be able to set it. After adjusting all the run_compiler callsites and everything compiles again, you should make a commit and put all follow-up changes in separate commits.

As a follow up change you can make run_compiler take the struct as an argument instead of all the fields individually. You can then destructure the struct in the body of run_compiler to get all the fields out.

There may be other things that you may notice while implementing this change that could be optimized, idk, but we can close this issue after the above changes.

@lakshya-sky
Copy link
Contributor

Hi @oli-obk, I was trying to get my hands on this. But since Optional fields can't be copied to run_compile, how can I pass those fields to run_compile?. Here is code change that you suggested.

pub struct RunCompiler<'a, 'b> {
    at_args: &'a [String],
    callbacks: &'b mut (dyn Callbacks + Send),
    file_loader: Option<Box<dyn FileLoader + Send + Sync>>,
    emitter: Option<Box<dyn Write + Send>>,
    make_codegen_backend:
        Option<Box<dyn FnOnce(&config::Options) -> Box<dyn CodegenBackend> + Send>>,
}

impl<'a, 'b> RunCompiler<'a, 'b> {
    pub fn new(at_args: &'a [String], callbacks: &'b mut (dyn Callbacks + Send)) -> Self {
        Self { at_args, callbacks, file_loader: None, emitter: None, make_codegen_backend: None }
    }
    pub fn set_make_codegen_backend(
        &mut self,
        make_codegen_backend: Option<
            Box<dyn FnOnce(&config::Options) -> Box<dyn CodegenBackend> + Send>,
        >,
    ) -> &mut Self {
        self.make_codegen_backend = make_codegen_backend;
        self
    }
    pub fn set_emitter(&mut self, emitter: Option<Box<dyn Write + Send>>) -> &mut Self {
        self.emitter = emitter;
        self
    }
    pub fn set_file_loader(
        &mut self,
        file_loader: Option<Box<dyn FileLoader + Send + Sync>>,
    ) -> &mut Self {
        self.file_loader = file_loader;
        self
    }
    pub fn run(&mut self) -> interface::Result<()> {
        run_compiler(
            self.at_args,
            self.callbacks,
           /* These fields are behind mutable reference hence can't be moved. */
            self.file_loader,  
            self.emitter,
            self.make_codegen_backend,
        )
    }
}

I could technically replace run_compile() to accept references instead of values but that will require the entire function to refactor, and I am not sure I can do that.

Am I doing it wrong? What alternative I can take to get this issue fixed?

@bjorn3
Copy link
Member Author

bjorn3 commented Oct 7, 2020

run can take self instead of &mut self.

@lakshya-sky
Copy link
Contributor

Thanks @bjorn3, just been away from rust for like 15 days and completely forgot I can do that.

one more thing. run_compiler was also used in miri and rls submodules. Do I need to make a pull request there for the changes.

Thanks.

@bjorn3
Copy link
Member Author

bjorn3 commented Oct 7, 2020

Once #77635 (beta branching) is merged it is fine for miri and rls to break. Submitting a PR there is nice, but not necessary. The respective maintainers are normally the ones to fix them again.

@bors bors closed this as completed in 25d2d09 Oct 11, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 24, 2020
…hewjasper

Replace run_compiler with RunCompiler builder pattern

Fixes rust-lang#77286. Replaces rustc_driver:run_compiler with RunCompiler builder pattern.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-driver Area: rustc_driver that ties everything together into the `rustc` compiler C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants