🐛Deno.stdin.setRaw(false) does not disable ENABLE_VIRTUAL_TERMINAL_INPUT

[rivy]

@dsherret , @bartlomieju (I saw that you both collaborated on the last change to the ops code.)

Deno.stdin.setRaw(false) does not disable ENABLE_VIRTUAL_TERMINAL_INPUT. Consequently, using Deno.stdin.setRaw(true) in a script always leaves the terminal in "virtual terminal input" mode back at the command line, disabling arrow usage.

C> ver
Microsoft Windows [Version 10.0.19044.2604]
C>deno -V
deno 1.30.3

To show the bug, execute this script:

// `deno run -A --unstable $0`

// Deno.stdin.setRaw(false) *does not* turn off ENABLE_VIRTUAL_TERMINAL_INPUT (DWORD = 0x0200) for STDIN
// ... causes incorrect/odd input for arrow keys at CMD line after use

// ref: rust ops code @ <https://github.com/denoland/deno/blob/d5634164cb86771fc279468cbb93e311c1ad3089/runtime/ops/tty.rs#L46-L102>
// ref: commit @ <https://github.com/denoland/deno/commit/715f35d635bf5a4e972238e6ac8d290c0e096083>
// ref: test(s) @ <https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d126c95579fdc2dca91b14684ffe6f68>

// spell-checker: ignore (winos) getch kbhit

console.log(`stdin: isatty()=${Deno.isatty(Deno.stdin.rid)}`);

const dllKernel = (() => {
  try {
    return (Deno as any).dlopen?.("kernel32.dll", {
      "GetConsoleMode":
        /* https://learn.microsoft.com/en-us/windows/console/getconsolemode */ {
          parameters: ["pointer", "buffer"],
          result: "u32",
        },
      "GetStdHandle":
        /* https://learn.microsoft.com/en-us/windows/console/getstdhandle */ {
          parameters: ["u32"],
          result: "pointer",
        },
      "SetConsoleMode":
        /* https://learn.microsoft.com/en-us/windows/console/setconsolemode */ {
          parameters: ["pointer", "u32"],
          result: "u32",
        },
    });
  } catch {
    return undefined;
  }
})();

console.log({ dllKernel });
if (dllKernel == null) {
  console.warn(
    `Unable to load DLLs; use \`deno run -A --unstable "${Deno.mainModule}"\``,
  );
}

const ENABLE_VIRTUAL_TERMINAL_INPUT = 0x0200;
const stdinID = 4294967286; /* stdin == (DWORD(-10)) */
const stdinHandle = dllKernel.symbols.GetStdHandle(stdinID);

{
  const buffer = new Uint32Array(1).fill(0);
  const _ = dllKernel.symbols.GetConsoleMode(stdinHandle, buffer);
  const consoleMode = buffer[0];
  console.log("initial ::", {
    stdinHandle,
    buffer,
    consoleMode: "0x" + consoleMode.toString(16),
  });
}

Deno.stdin.setRaw(true);

{
  const buffer = new Uint32Array(1).fill(0);
  const _ = dllKernel.symbols.GetConsoleMode(stdinHandle, buffer);
  const consoleMode = buffer[0];
  console.log("setRaw(true)", {
    stdinHandle,
    buffer,
    consoleMode: "0x" + consoleMode.toString(16),
  });
}

Deno.stdin.setRaw(false);

{
  const buffer = new Uint32Array(1).fill(0);
  const _ = dllKernel.symbols.GetConsoleMode(stdinHandle, buffer);
  const consoleMode = buffer[0];
  console.log("setRaw(false)", {
    stdinHandle,
    buffer,
    consoleMode: "0x" + consoleMode.toString(16),
  });
}

//=== cut here

{
  // * disable ENABLE_VIRTUAL_TERMINAL_INPUT manually with FFI `SetConsoleMode()`
  const buffer = new Uint32Array(1).fill(0);
  const _0 = dllKernel.symbols.GetConsoleMode(stdinHandle, buffer);
  let consoleMode = buffer[0];
  consoleMode = consoleMode & ~ENABLE_VIRTUAL_TERMINAL_INPUT;
  const _1 = dllKernel.symbols.SetConsoleMode(stdinHandle, consoleMode);
}

{
  const buffer = new Uint32Array(1).fill(0);
  const _ = dllKernel.symbols.GetConsoleMode(stdinHandle, buffer);
  const consoleMode = buffer[0];
  console.log("after SetConsoleMode()", {
    stdinHandle,
    buffer,
    consoleMode: "0x" + consoleMode.toString(16),
  });
}

For the usual CMD shell terminal, the output is ...

stdin: isatty()=true
{
  dllKernel: DynamicLibrary {
    symbols: {
      SetConsoleMode: [Function],
      GetConsoleMode: [Function],
      GetStdHandle: [Function]
    }
  }
}
initial :: { stdinHandle: 88, buffer: Uint32Array(1) [ 247 ], consoleMode: "0xf7" }
setRaw(true) { stdinHandle: 88, buffer: Uint32Array(1) [ 752 ], consoleMode: "0x2f0" }
setRaw(false) { stdinHandle: 88, buffer: Uint32Array(1) [ 759 ], consoleMode: "0x2f7" }
after SetConsoleMode() { stdinHandle: 88, buffer: Uint32Array(1) [ 247 ], consoleMode: "0xf7" }

For Windows Terminal, the output is ...

stdin: isatty()=true
{
  dllKernel: DynamicLibrary {
    symbols: {
      SetConsoleMode: [Function],
      GetConsoleMode: [Function],
      GetStdHandle: [Function]
    }
  }
}
initial :: { stdinHandle: 76, buffer: Uint32Array(1) [ 503 ], consoleMode: "0x1f7" }
setRaw(true) { stdinHandle: 76, buffer: Uint32Array(1) [ 1008 ], consoleMode: "0x3f0" }
setRaw(false) { stdinHandle: 76, buffer: Uint32Array(1) [ 1015 ], consoleMode: "0x3f7" }
after SetConsoleMode() { stdinHandle: 76, buffer: Uint32Array(1) [ 503 ], consoleMode: "0x1f7" }

Disabling the code below //=== cut here will leave the command line with disabled arrow keys (displaying character sequences instead of interpreting the arrows).

EDIT: I'm not sure where the bug is occurring; the deno ops code looks correct. On second look, the problem is in the ops code...

deno/runtime/ops/tty.rs

Lines 89 to 93 in d563416

	let new_mode = if is_raw {
	original_mode & !RAW_MODE_MASK | wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
	} else {
	original_mode | RAW_MODE_MASK & !wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
	};

The precedence of & is higher than |, so parentheses are needed and this code should be...

      let new_mode = if is_raw {
        original_mode & !RAW_MODE_MASK | wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
      } else {
        ( original_mode | RAW_MODE_MASK ) & !wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
      };

[dsherret]

@rivy it seems you diagnosed the problem. Would you like to open a PR that fixes it? It would be good to add a unit test for this specific functionality.

[rivy]

@dsherret , 👋🏻

@rivy it seems you diagnosed the problem. Would you like to open a PR that fixes it? It would be good to add a unit test for this specific functionality.

I'd be happy to... the fix is dead simple. But I'm not fully familiar with the code base, and I don't see where any op_... functions are being tested. Since testing would involve calling WinOS functions, adding a test to the tty.rs would likely be simplest, but I don't know how state: &mut OpState would be setup and whether StdFileResource::with_file(state, rid, ... would be needed for isolation within the test function.

If you can offer some guidance, I'm happy to submit a PR. On the other hand, if you feel like it's easier to complete yourself, that's reasonable for me as well.

[dsherret]

@rivy I meant just a unit test for this specific functionality, so that could be done by extracting out to current code to a function like:

#[cfg(windows)]
fn new_mode(original_mode: DWORD, is_raw: boolean) -> DWORD {
  const RAW_MODE_MASK: DWORD = wincon::ENABLE_LINE_INPUT
    | wincon::ENABLE_ECHO_INPUT
    | wincon::ENABLE_PROCESSED_INPUT;
  if is_raw {
    original_mode & !RAW_MODE_MASK | wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
  } else {
    original_mode | RAW_MODE_MASK & !wincon::ENABLE_VIRTUAL_TERMINAL_INPUT
  }
}

Then adding some tests:

#[cfg(test)]
mod test{
  use super::*;

  #[cfg(windows)]
  #[test]
  fn test_new_mode() {
    assert_eq!(new_mode(..., ...), ....);
    // more assertions here...
  }
}

Then updating the code to the correct behaviour.

[rivy]

@rivy I meant just a unit test for this specific functionality, so that could be done by extracting out to current code to a function like: ...

Ah! Sure, I'm happy to do that... I'll have a PR for you by Wednesday.