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

Add support for -Zunpretty=hir #683

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Add support for -Zunpretty=hir #683

merged 2 commits into from
Mar 19, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 21, 2021

This is the first half of #502. Adding support for the AST requires first adding -Zunpretty=ast to the rust compiler (rust-lang/rust#82304).

I want to run rustfmt on the generated code, but I'm not sure how to do that.. This is what I tried so far:

diff --git a/ui/src/sandbox.rs b/ui/src/sandbox.rs
index 838d7a0..5fe4887 100644
--- a/ui/src/sandbox.rs
+++ b/ui/src/sandbox.rs
@@ -150,7 +150,10 @@ impl Sandbox {
             Some(file) => {
                 if CompileTarget::Hir == req.target {
                     // Run rustfmt on the generated HIR.
-                    format_command(req).arg(file)
+                    let mut rustfmt = self.format_command(req, true);
+                    rustfmt.arg(dbg!(&file));
+                    log::debug!("formatting generated HIR: {:?}", rustfmt);
+                    run_command_with_timeout(rustfmt)?;
                 }
                 read(&file)?.unwrap_or_else(String::new)
             }
@@ -200,7 +203,7 @@ impl Sandbox {
 
     pub fn format(&self, req: &FormatRequest) -> Result<FormatResponse> {
         self.write_source_code(&req.code)?;
-        let command = self.format_command(req);
+        let command = self.format_command(req, false);
 
         let output = run_command_with_timeout(command)?;
 
@@ -356,14 +359,19 @@ impl Sandbox {
         cmd
     }
 
-    fn format_command(&self, req: impl EditionRequest) -> Command {
+    fn format_command(&self, req: impl EditionRequest, single_file: bool) -> Command {
         let crate_type = CrateType::Binary;
 
         let mut cmd = self.docker_command(Some(crate_type));
 
         cmd.apply_edition(req);
 
-        cmd.arg("rustfmt").args(&["cargo", "fmt"]);
+        cmd.arg("rustfmt");
+        if single_file {
+            cmd.arg("rustfmt");
+        } else {
+            cmd.args(&["cargo", "fmt"]);
+        }
 
         log::debug!("Formatting command is {:?}", cmd);

But it didn't actually format the code, I'm not sure what's going wrong. AFAIK rustfmt formats files in places, not to stdout.

Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to run rustfmt on the generated code, but I'm not sure how to do that

You won't be able to; not the way you are trying to, at least. Rustfmt is in a completely separate container (there are 6: stable, beta, nightly, miri, clippy, rustfmt). This allows updating the compiler when miri/clippy/rustfmt are broken.

The same general idea is wanted for expanding macros, so any solution may want to attempt to encompass that, but I expect that the easiest way is to make a second request from the frontend.

ui/frontend/selectors/index.ts Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Feb 21, 2021

You won't be able to; not the way you are trying to, at least. Rustfmt is in a completely separate container (there are 6: stable, beta, nightly, miri, clippy, rustfmt). This allows updating the compiler when miri/clippy/rustfmt are broken.

Hmm, maybe I could do it by feeding the input to rustfmt on stdin instead of trying to use the same files between containers? I don't think the run_command_with_timeout API allows for that though, to write to stdin I need to have a Child handle: https://doc.rust-lang.org/std/process/struct.Stdio.html#method.piped

@jyn514
Copy link
Member Author

jyn514 commented Feb 23, 2021

FWIW, I think this would be useful to land as is, and I can try to add rustfmt in a follow-up PR.

ui/frontend/BuildMenu.tsx Outdated Show resolved Hide resolved
ui/frontend/BuildMenu.tsx Outdated Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2021

@shepmaster this is ready for re-review.

@shepmaster shepmaster temporarily deployed to CI March 13, 2021 18:54 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 18:54 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 18:54 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:24 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:24 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:24 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:29 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:29 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 19:29 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 20:40 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 20:40 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 20:40 Inactive
@shepmaster shepmaster temporarily deployed to CI March 13, 2021 20:40 Inactive
@shepmaster shepmaster added the CI: approved Allowed access to CI secrets label Mar 14, 2021
@shepmaster shepmaster removed the CI: approved Allowed access to CI secrets label Mar 14, 2021
@shepmaster shepmaster changed the base branch from master to hack March 14, 2021 19:35
@shepmaster shepmaster added the CI: approved Allowed access to CI secrets label Mar 14, 2021
@shepmaster shepmaster added CI: approved Allowed access to CI secrets and removed CI: approved Allowed access to CI secrets labels Mar 15, 2021
@shepmaster shepmaster added CI: approved Allowed access to CI secrets and removed CI: approved Allowed access to CI secrets labels Mar 15, 2021
@shepmaster shepmaster added CI: approved Allowed access to CI secrets and removed CI: approved Allowed access to CI secrets labels Mar 15, 2021
jyn514 and others added 2 commits March 18, 2021 14:26
@shepmaster shepmaster changed the base branch from hack to master March 18, 2021 18:26
@shepmaster shepmaster added CI: approved Allowed access to CI secrets and removed CI: approved Allowed access to CI secrets labels Mar 18, 2021
@shepmaster shepmaster merged commit d03009c into rust-lang:master Mar 19, 2021
@jyn514 jyn514 deleted the hir branch March 19, 2021 15:09
@jyn514
Copy link
Member Author

jyn514 commented Mar 19, 2021

🎉

@shepmaster shepmaster added the enhancement Something new the playground could do label Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: approved Allowed access to CI secrets enhancement Something new the playground could do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants