Repository: denoland/deno
Pull request: feat(inspector): accept bare host and bare port for --inspect flag

This commit changes arg parsing for `--inspect`, `--inspect-brk` and `--inspect-wait` flag
to allow passing values as follows:
- `127.0.0.1` -> `127.0.0.1:9229`
- `192.168.0.1` -> `192.168.0.1:9229`
- `:10000` -> `127.0.0.1:10000`
- `10000` -> `127.0.0.1:10000`
- `:0` -> `127.0.0.1:<OS assigned port>`
- `0` -> `127.0.0.1:<OS assigned port>`
This matches what Node.js is doing and will help to solve test compatiblity discovered in https://github.com/denoland/deno/pull/31863.

Changed files:
filename: cli/args/flags.rs
patch: @@ -4,6 +4,7 @@ use std::borrow::Cow;
 use std::collections::HashSet;
 use std::env;
 use std::ffi::OsString;
+use std::net::IpAddr;
 use std::net::SocketAddr;
 use std::num::NonZeroU8;
 use std::num::NonZeroU32;
@@ -4921,43 +4922,106 @@ fn deny_import_arg() -> Arg {
   .value_parser(flags_net::validator)
 }
 
+pub fn inspect_value_parser(host_and_port: &str) -> Result<SocketAddr, String> {
+  const DEFAULT_HOST: &str = "127.0.0.1";
+  const DEFAULT_PORT: u16 = 9229;
+
+  fn parse_port(port: &str) -> Result<u16, String> {
+    port
+      .parse::<u16>()
+      .map_err(|_| format!("Invalid inspector port '{port}'"))
+  }
+
+  let default_host: IpAddr = DEFAULT_HOST.parse().unwrap();
+
+  if host_and_port.is_empty() {
+    return Err("Inspector address cannot be empty".to_string());
+  }
+
+  if let Some(port_part) = host_and_port.strip_prefix(':') {
+    let port = if port_part.is_empty() {
+      DEFAULT_PORT
+    } else {
+      parse_port(port_part)?
+    };
+    return Ok(SocketAddr::new(default_host, port));
+  }
+
+  if host_and_port.contains(':') {
+    if let Ok(addr) = host_and_port.parse::<SocketAddr>() {
+      return Ok(addr);
+    }
+
+    if let Ok(host_ip) = host_and_port.parse::<IpAddr>() {
+      return Ok(SocketAddr::new(host_ip, DEFAULT_PORT));
+    }
+
+    let (host_part, port_part) = host_and_port
+      .rsplit_once(':')
+      .ok_or_else(|| format!("Invalid inspector address '{host_and_port}'"))?;
+
+    let port = if port_part.is_empty() {
+      DEFAULT_PORT
+    } else {
+      parse_port(port_part)?
+    };
+
+    let host_ip = host_part
+      .parse::<IpAddr>()
+      .map_err(|e| format!("Invalid inspector host '{host_part}': {:?}", e))?;
+
+    return Ok(SocketAddr::new(host_ip, port));
+  }
+
+  if host_and_port.chars().all(|c| c.is_ascii_digit()) {
+    let port = parse_port(host_and_port)?;
+    return Ok(SocketAddr::new(default_host, port));
+  }
+
+  let host_ip = host_and_port.parse::<IpAddr>().map_err(|e| {
+    format!("Invalid inspector host '{host_and_port}': {:?}", e)
+  })?;
+
+  Ok(SocketAddr::new(host_ip, DEFAULT_PORT))
+}
+
 fn inspect_args(app: Command) -> Command {
   app
     .arg(
       Arg::new("inspect")
         .long("inspect")
-        .value_name("HOST_AND_PORT")
+        .value_name("HOST_PORT")
         .default_missing_value("127.0.0.1:9229")
-        .help(cstr!("Activate inspector on host:port <p(245)>[default: 127.0.0.1:9229]</>"))
+        .help(cstr!("Activate inspector on host:port <p(245)>[default: 127.0.0.1:9229]</>. Host and port are optional. Using port 0 will assign a random free port."))
         .num_args(0..=1)
         .require_equals(true)
-        .value_parser(value_parser!(SocketAddr))
+        .value_parser(inspect_value_parser)
         .help_heading(DEBUGGING_HEADING),
     )
     .arg(
       Arg::new("inspect-brk")
         .long("inspect-brk")
-        .value_name("HOST_AND_PORT")
+        .value_name("HOST_PORT")
         .default_missing_value("127.0.0.1:9229")
         .help(
           "Activate inspector on host:port, wait for debugger to connect and break at the start of user script",
         )
         .num_args(0..=1)
         .require_equals(true)
-        .value_parser(value_parser!(SocketAddr))
+        .value_parser(inspect_value_parser)
         .help_heading(DEBUGGING_HEADING),
     )
     .arg(
       Arg::new("inspect-wait")
         .long("inspect-wait")
-        .value_name("HOST_AND_PORT")
+        .value_name("HOST_PORT")
         .default_missing_value("127.0.0.1:9229")
         .help(
           "Activate inspector on host:port and wait for debugger to connect before running user code",
         )
         .num_args(0..=1)
         .require_equals(true)
-        .value_parser(value_parser!(SocketAddr))
+        .value_parser(inspect_value_parser)
         .help_heading(DEBUGGING_HEADING),
     )
 }
@@ -10907,7 +10971,7 @@ mod tests {
       .filter(|arg| arg.get_id() == "inspect")
       .collect::<Vec<_>>();
     // The value_name cannot have a : otherwise it breaks shell completions for zsh.
-    let value_name = "HOST_AND_PORT";
+    let value_name = "HOST_PORT";
     let arg = inspect_args
       .iter()
       .any(|v| v.get_value_names().unwrap() == [value_name]);
@@ -14173,4 +14237,59 @@ Usage: deno repl [OPTIONS] [-- [ARGS]...]\n"
       }
     );
   }
+
+  #[test]
+  fn inspect_flag_parsing() {
+    use std::net::IpAddr;
+    use std::net::Ipv4Addr;
+
+    let cases = vec![
+      (
+        "127.0.0.1:9229",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 9229),
+      ),
+      (
+        "192.168.0.1",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)), 9229),
+      ),
+      (
+        "10000",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 10000),
+      ),
+      (
+        ":10000",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 10000),
+      ),
+      (
+        ":0",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 0),
+      ),
+      (
+        "0",
+        SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 0),
+      ),
+    ];
+
+    for case in cases {
+      let flags = flags_from_vec(svec![
+        "deno",
+        "run",
+        &format!("--inspect={}", case.0),
+        "script.ts",
+      ])
+      .unwrap();
+      assert_eq!(
+        flags,
+        Flags {
+          subcommand: DenoSubcommand::Run(RunFlags {
+            script: "script.ts".to_string(),
+            ..Default::default()
+          }),
+          inspect: Some(case.1),
+          code_cache_enabled: true,
+          ..Flags::default()
+        }
+      );
+    }
+  }
 }

filename: cli/args/mod.rs
patch: @@ -6,7 +6,6 @@ mod flags_net;
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::env;
-use std::net::SocketAddr;
 use std::num::NonZeroUsize;
 use std::path::Path;
 use std::path::PathBuf;
@@ -1023,12 +1022,12 @@ impl CliOptions {
       || self.flags.inspect_wait.is_some()
   }
 
-  pub fn inspect_brk(&self) -> Option<SocketAddr> {
-    self.flags.inspect_brk
+  pub fn inspect_brk(&self) -> bool {
+    self.flags.inspect_brk.is_some()
   }
 
-  pub fn inspect_wait(&self) -> Option<SocketAddr> {
-    self.flags.inspect_wait
+  pub fn inspect_wait(&self) -> bool {
+    self.flags.inspect_wait.is_some()
   }
 
   pub fn log_level(&self) -> Option<log::Level> {

filename: cli/factory.rs
patch: @@ -1124,8 +1124,8 @@ impl CliFactory {
       has_node_modules_dir: workspace_factory
         .node_modules_dir_path()?
         .is_some(),
-      inspect_brk: cli_options.inspect_brk().is_some(),
-      inspect_wait: cli_options.inspect_wait().is_some(),
+      inspect_brk: cli_options.inspect_brk(),
+      inspect_wait: cli_options.inspect_wait(),
       trace_ops: cli_options.trace_ops().clone(),
       is_standalone: false,
       auto_serve: std::env::var("DENO_AUTO_SERVE").is_ok(),

filename: runtime/inspector_server.rs
patch: @@ -62,6 +62,14 @@ pub enum InspectorServerError {
     #[inherit]
     source: std::io::Error,
   },
+  #[class(inherit)]
+  #[error("Failed to get inspector server's assigned address")]
+  LocalAddr {
+    host: SocketAddr,
+    #[source]
+    #[inherit]
+    source: std::io::Error,
+  },
 }
 
 impl InspectorServer {
@@ -77,6 +85,10 @@ impl InspectorServer {
     let tcp_listener = std::net::TcpListener::bind(host)
       .map_err(|source| InspectorServerError::Connect { host, source })?;
     tcp_listener.set_nonblocking(true)?;
+    // TODO(bartlomieju): update process wide inspector server host
+    let host = tcp_listener
+      .local_addr()
+      .map_err(|source| InspectorServerError::LocalAddr { host, source })?;
 
     let thread_handle = thread::spawn(move || {
       let rt = crate::tokio_util::create_basic_runtime();

filename: tests/unit/inspector_test.ts
patch: @@ -22,12 +22,6 @@ interface InspectorTesterOptions {
 
 const DEFAULT_CDP_TIMEOUT = 60_000;
 
-let nextPort = 9229;
-function inspectFlagWithUniquePort(flagPrefix: string): string {
-  const port = nextPort++;
-  return `${flagPrefix}=127.0.0.1:${port}`;
-}
-
 function ignoreScriptParsed(msg: CDPMessage): boolean {
   return msg.method !== "Debugger.scriptParsed";
 }
@@ -415,7 +409,7 @@ class InspectorTester {
 Deno.test("inspector_connect", async () => {
   const script = `${testdataPath}/inspector1.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect"), script],
+    ["run", "-A", "--inspect=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -432,7 +426,7 @@ Deno.test("inspector_connect", async () => {
 Deno.test("inspector_break_on_first_line", async () => {
   const script = `${testdataPath}/inspector2.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -484,7 +478,7 @@ Deno.test("inspector_break_on_first_line", async () => {
 Deno.test("inspector_pause", async () => {
   const script = `${testdataPath}/inspector1.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect"), script],
+    ["run", "-A", "--inspect=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -503,65 +497,10 @@ Deno.test("inspector_pause", async () => {
   }
 });
 
-Deno.test("inspector_port_collision", async () => {
-  if (Deno.build.os === "windows") return;
-
-  const script = `${testdataPath}/inspector1.js`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect");
-
-  const command1 = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
-    stderr: "piped",
-  });
-  const child1 = command1.spawn();
-  const stderr1 = child1.stderr.pipeThrough(new TextDecoderStream())
-    .getReader();
-
-  let buffer1 = "";
-  while (true) {
-    const { value, done } = await stderr1.read();
-    if (done) break;
-    buffer1 += value;
-    if (buffer1.includes("Debugger listening on ")) break;
-  }
-
-  const command2 = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
-    stderr: "piped",
-  });
-  const child2 = command2.spawn();
-  const stderr2 = child2.stderr.pipeThrough(new TextDecoderStream())
-    .getReader();
-
-  let buffer2 = "";
-  while (true) {
-    const { value, done } = await stderr2.read();
-    if (done) break;
-    buffer2 += value;
-    if (
-      buffer2.includes("Failed to start inspector server") ||
-      buffer2.includes("error")
-    ) {
-      break;
-    }
-  }
-
-  assert(
-    !buffer2.includes("Debugger listening"),
-    "Second process should not listen successfully",
-  );
-
-  await stderr1.cancel();
-  await stderr2.cancel();
-  child1.kill();
-  await child1.status;
-  await child2.status;
-});
-
 Deno.test("inspector_does_not_hang", async () => {
   const script = `${testdataPath}/inspector3.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     {
       notificationFilter: ignoreScriptParsed,
       env: { NO_COLOR: "1" },
@@ -617,10 +556,9 @@ Deno.test("inspector_does_not_hang", async () => {
 
 Deno.test("inspector_without_brk_runs_code", async () => {
   const script = `${testdataPath}/inspector4.js`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect");
 
   const command = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
+    args: ["run", "-A", "--inspect=0", script],
     stdout: "piped",
     stderr: "piped",
   });
@@ -649,10 +587,9 @@ Deno.test("inspector_without_brk_runs_code", async () => {
 
 Deno.test("inspector_json", async () => {
   const script = `${testdataPath}/inspector1.js`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect");
 
   const command = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
+    args: ["run", "-A", "--inspect=0", script],
     stderr: "piped",
   });
 
@@ -683,10 +620,9 @@ Deno.test("inspector_json", async () => {
 
 Deno.test("inspector_connect_non_ws", async () => {
   const script = `${testdataPath}/inspector1.js`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect");
 
   const command = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
+    args: ["run", "-A", "--inspect=0", script],
     stderr: "piped",
   });
 
@@ -709,7 +645,7 @@ Deno.test("inspector_connect_non_ws", async () => {
 Deno.test("inspector_memory", async () => {
   const script = `${testdataPath}/memory.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     {
       notificationFilter: ignoreScriptParsed,
       env: { RUST_BACKTRACE: "1" },
@@ -759,7 +695,7 @@ Deno.test("inspector_memory", async () => {
 Deno.test("inspector_profile", async () => {
   const script = `${testdataPath}/memory.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -813,7 +749,7 @@ Deno.test("inspector_profile", async () => {
 Deno.test("inspector_multiple_workers", async () => {
   const script = `${testdataPath}/multi_worker_main.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -862,7 +798,7 @@ Deno.test("inspector_multiple_workers", async () => {
 Deno.test("inspector_worker_target_discovery", async () => {
   const script = `${testdataPath}/worker_main.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -922,7 +858,7 @@ Deno.test("inspector_worker_target_discovery", async () => {
 Deno.test("inspector_node_worker_enable", async () => {
   const script = `${testdataPath}/worker_main.js`;
   const tester = await InspectorTester.create(
-    ["run", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["run", "-A", "--inspect-brk=0", script],
     { notificationFilter: ignoreScriptParsed },
   );
 
@@ -962,7 +898,7 @@ Deno.test("inspector_node_worker_enable", async () => {
 
 Deno.test("inspector_runtime_evaluate_does_not_crash", async () => {
   const tester = await InspectorTester.create(
-    ["repl", "-A", inspectFlagWithUniquePort("--inspect")],
+    ["repl", "-A", "--inspect=0"],
     {
       notificationFilter: ignoreScriptParsed,
       env: { RUST_BACKTRACE: "1" },
@@ -1060,7 +996,7 @@ Deno.test("inspector_break_on_first_line_in_test", async () => {
 
   const script = `${testdataPath}/inspector_test.js`;
   const tester = await InspectorTester.create(
-    ["test", "-A", inspectFlagWithUniquePort("--inspect-brk"), script],
+    ["test", "-A", "--inspect-brk=0", script],
     {
       notificationFilter: ignoreScriptParsed,
       env: { NO_COLOR: "1" },
@@ -1134,7 +1070,7 @@ Deno.test("inspector_with_ts_files", async () => {
       "run",
       "-A",
       "--check",
-      inspectFlagWithUniquePort("--inspect-brk"),
+      "--inspect-brk=0",
       script,
     ],
     { notificationFilter },
@@ -1258,10 +1194,9 @@ Deno.test("inspector_wait", async () => {
   const script = `${testdataPath}/inspect_wait.js`;
   const tempDir = await Deno.makeTempDir();
   const helloPath = `${tempDir}/hello.txt`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect-wait");
 
   const command = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
+    args: ["run", "-A", "--inspect-wait=0", script],
     stdin: "piped",
     stdout: "piped",
     stderr: "piped",
@@ -1345,10 +1280,9 @@ Deno.test("inspector_wait", async () => {
 
 Deno.test("inspector_node_runtime_api_url", async () => {
   const script = `${testdataPath}/node/url.js`;
-  const inspectFlag = inspectFlagWithUniquePort("--inspect");
 
   const command = new Deno.Command(Deno.execPath(), {
-    args: ["run", "-A", inspectFlag, script],
+    args: ["run", "-A", "--inspect=0", script],
     stdout: "piped",
     stderr: "piped",
   });