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

"force_multiline_blocks = true" is non-idempotent #4897

Open
RalfJung opened this issue Jul 12, 2021 · 2 comments
Open

"force_multiline_blocks = true" is non-idempotent #4897

RalfJung opened this issue Jul 12, 2021 · 2 comments
Labels
a-matches match arms, patterns, blocks, etc bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 12, 2021

To reproduce, start with this code:

use std::convert::TryFrom;
use std::env;
use std::ffi::{OsStr, OsString};
use std::io::ErrorKind;

use rustc_data_structures::fx::FxHashMap;
use rustc_mir::interpret::Pointer;
use rustc_target::abi::{LayoutOf, Size};

impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
    #[allow(non_snake_case)]
    fn GetCurrentDirectoryW(
        &mut self,
        size_op: &OpTy<'tcx, Tag>, // DWORD
        buf_op: &OpTy<'tcx, Tag>,  // LPTSTR
    ) -> InterpResult<'tcx, u32> {
        let this = self.eval_context_mut();
        this.assert_target_os("windows", "GetCurrentDirectoryW");

        if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
            this.reject_in_isolation("GetCurrentDirectoryW", reject_with)?;
            this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?;
            return Ok(0);
        }

        let size = u64::from(this.read_scalar(size_op)?.to_u32()?);
        let buf = this.read_scalar(buf_op)?.check_init()?;

        // If we cannot get the current directory, we return 0
        match env::current_dir() {
            Ok(cwd) =>
                return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?)),
            Err(e) => this.set_last_error_from_io_error(e.kind())?,
        }
        Ok(0)
    }
}

and this rustfmt.toml:

version = "Two"
use_small_heuristics = "Max"
match_arm_leading_pipes = "Preserve"
force_multiline_blocks = true

Run rustfmt once. It makes the following changes:

--- env.rs      2021-07-12 14:00:23.456543583 +0200
+++ env2.rs     2021-07-12 14:00:39.268279709 +0200
@@ -29,8 +29,9 @@
 
         // If we cannot get the current directory, we return 0
         match env::current_dir() {
-            Ok(cwd) =>
-                return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?)),
+            Ok(cwd) => {
+                return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?));
+            }
             Err(e) => this.set_last_error_from_io_error(e.kind())?,
         }
         Ok(0)

Then run it again. It makes further changes:

--- env2.rs     2021-07-12 14:00:39.268279709 +0200
+++ env3.rs     2021-07-12 14:00:47.316145404 +0200
@@ -30,7 +30,9 @@
         // If we cannot get the current directory, we return 0
         match env::current_dir() {
             Ok(cwd) => {
-                return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?));
+                return Ok(windows_check_buffer_size(
+                    this.write_path_to_wide_str(&cwd, buf, size)?,
+                ));
             }
             Err(e) => this.set_last_error_from_io_error(e.kind())?,
         }

This is a bug, rustfmt should be idempotent (i.e., running it a 2nd time should not change anything).

@calebcartwright calebcartwright added bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce labels Jul 18, 2021
@ytmimi
Copy link
Contributor

ytmimi commented Jul 26, 2022

confirming this is reproducible with version rustfmt 1.5.1-nightly (a451a39d 2022-07-25)

@ytmimi
Copy link
Contributor

ytmimi commented Jul 26, 2022

linking tracking issue for force_multiline_blocks #3374

@ytmimi ytmimi added the a-matches match arms, patterns, blocks, etc label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-matches match arms, patterns, blocks, etc bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce
Projects
None yet
Development

No branches or pull requests

3 participants