fix(lock): autodiscovery of lockfile (#16498)

This commit adds autodiscovery of lockfile. 

This only happens if Deno discovers the configuration file (either 
"deno.json" or "deno.jsonc"). In such case Deno tries to load
"deno.lock"
file that sits next to the configuration file, or creates one for user
if
the lockfile doesn't exist yet.

As a consequence, "--lock" and "--lock-write" flags had been updated.
"--lock" no longer requires a value, if one is not provided, it defaults
to "./deno.lock" resolved from the current working directory.
"--lock-write"
description was updated to say that it forces to overwrite a lockfile.

Autodiscovery is currently not handled by the LSP.

cli/args/flags.rs

@@ -458,7 +458,9 @@ impl Flags {
       Some(files.clone())
     } else if let Run(RunFlags { script }) = &self.subcommand {
       if let Ok(module_specifier) = deno_core::resolve_url_or_path(script) {
-        if module_specifier.scheme() == "file" {
+        if module_specifier.scheme() == "file"
+          || module_specifier.scheme() == "npm"
+        {
           if let Ok(p) = module_specifier.to_file_path() {
             Some(vec![p])
           } else {
@@ -2145,16 +2147,17 @@ fn lock_arg<'a>() -> Arg<'a> {
   Arg::new("lock")
     .long("lock")
     .value_name("FILE")
-    .help("Check the specified lock file")
+    .help("Check the specified lock file. If value is not provided, defaults to \"deno.lock\" in the current working directory.")
     .takes_value(true)
+    .min_values(0)
+    .max_values(1)
     .value_hint(ValueHint::FilePath)
 }
 
 fn lock_write_arg<'a>() -> Arg<'a> {
   Arg::new("lock-write")
     .long("lock-write")
-    .requires("lock")
-    .help("Write lock file (use with --lock)")
+    .help("Force overwriting the lock file.")
 }
 
 static CONFIG_HELP: Lazy<String> = Lazy::new(|| {
@@ -3098,7 +3101,11 @@ fn lock_args_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
 
 fn lock_arg_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
   if matches.is_present("lock") {
-    let lockfile = matches.value_of("lock").unwrap();
+    let lockfile = if let Some(path) = matches.value_of("lock") {
+      path
+    } else {
+      "./deno.lock"
+    };
     flags.lock = Some(PathBuf::from(lockfile));
   }
 }
@@ -5335,6 +5342,57 @@ mod tests {
         ..Flags::default()
       }
     );
+
+    let r = flags_from_vec(svec![
+      "deno",
+      "run",
+      "--lock",
+      "--lock-write",
+      "script.ts"
+    ]);
+    assert_eq!(
+      r.unwrap(),
+      Flags {
+        subcommand: DenoSubcommand::Run(RunFlags {
+          script: "script.ts".to_string(),
+        }),
+        lock_write: true,
+        lock: Some(PathBuf::from("./deno.lock")),
+        ..Flags::default()
+      }
+    );
+
+    let r = flags_from_vec(svec![
+      "deno",
+      "run",
+      "--lock-write",
+      "--lock",
+      "lock.json",
+      "script.ts"
+    ]);
+    assert_eq!(
+      r.unwrap(),
+      Flags {
+        subcommand: DenoSubcommand::Run(RunFlags {
+          script: "script.ts".to_string(),
+        }),
+        lock_write: true,
+        lock: Some(PathBuf::from("lock.json")),
+        ..Flags::default()
+      }
+    );
+
+    let r = flags_from_vec(svec!["deno", "run", "--lock-write", "script.ts"]);
+    assert_eq!(
+      r.unwrap(),
+      Flags {
+        subcommand: DenoSubcommand::Run(RunFlags {
+          script: "script.ts".to_string(),
+        }),
+        lock_write: true,
+        ..Flags::default()
+      }
+    );
   }
 
   #[test]

cli/args/mod.rs

@@ -24,6 +24,7 @@ use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
 use deno_core::normalize_path;
+use deno_core::parking_lot::Mutex;
 use deno_core::url::Url;
 use deno_runtime::colors;
 use deno_runtime::deno_tls::rustls::RootCertStore;
@@ -33,6 +34,7 @@ use std::collections::BTreeMap;
 use std::env;
 use std::net::SocketAddr;
 use std::path::PathBuf;
+use std::sync::Arc;
 
 use crate::args::config_file::JsxImportSourceConfig;
 use crate::deno_dir::DenoDir;
@@ -61,11 +63,16 @@ pub struct CliOptions {
   // application need not concern itself with, so keep these private
   flags: Flags,
   maybe_config_file: Option<ConfigFile>,
+  maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
   overrides: CliOptionOverrides,
 }
 
 impl CliOptions {
-  pub fn new(flags: Flags, maybe_config_file: Option<ConfigFile>) -> Self {
+  pub fn new(
+    flags: Flags,
+    maybe_config_file: Option<ConfigFile>,
+    maybe_lockfile: Option<Lockfile>,
+  ) -> Self {
     if let Some(insecure_allowlist) =
       flags.unsafely_ignore_certificate_errors.as_ref()
     {
@@ -80,8 +87,11 @@ impl CliOptions {
       eprintln!("{}", colors::yellow(msg));
     }
 
+    let maybe_lockfile = maybe_lockfile.map(|l| Arc::new(Mutex::new(l)));
+
     Self {
       maybe_config_file,
+      maybe_lockfile,
       flags,
       overrides: Default::default(),
     }
@@ -89,7 +99,9 @@ impl CliOptions {
 
   pub fn from_flags(flags: Flags) -> Result<Self, AnyError> {
     let maybe_config_file = ConfigFile::discover(&flags)?;
-    Ok(Self::new(flags, maybe_config_file))
+    let maybe_lock_file =
+      Lockfile::discover(&flags, maybe_config_file.as_ref())?;
+    Ok(Self::new(flags, maybe_config_file, maybe_lock_file))
   }
 
   pub fn maybe_config_file_specifier(&self) -> Option<ModuleSpecifier> {
@@ -210,13 +222,8 @@ impl CliOptions {
       .map(|host| InspectorServer::new(host, version::get_user_agent()))
   }
 
-  pub fn resolve_lock_file(&self) -> Result<Option<Lockfile>, AnyError> {
-    if let Some(filename) = &self.flags.lock {
-      let lockfile = Lockfile::new(filename.clone(), self.flags.lock_write)?;
-      Ok(Some(lockfile))
-    } else {
-      Ok(None)
-    }
+  pub fn maybe_lock_file(&self) -> Option<Arc<Mutex<Lockfile>>> {
+    self.maybe_lockfile.clone()
   }
 
   pub fn resolve_tasks_config(

cli/lockfile.rs

@@ -15,9 +15,11 @@ use std::path::PathBuf;
 use std::rc::Rc;
 use std::sync::Arc;
 
+use crate::args::ConfigFile;
 use crate::npm::NpmPackageReq;
 use crate::npm::NpmResolutionPackage;
 use crate::tools::fmt::format_json;
+use crate::Flags;
 
 #[derive(Debug)]
 pub struct LockfileError(String);
@@ -73,6 +75,16 @@ pub struct LockfileContent {
   pub npm: NpmContent,
 }
 
+impl LockfileContent {
+  fn empty() -> Self {
+    Self {
+      version: "2".to_string(),
+      remote: BTreeMap::new(),
+      npm: NpmContent::default(),
+    }
+  }
+}
+
 #[derive(Debug, Clone)]
 pub struct Lockfile {
   pub overwrite: bool,
@@ -82,36 +94,91 @@ pub struct Lockfile {
 }
 
 impl Lockfile {
+  pub fn discover(
+    flags: &Flags,
+    maybe_config_file: Option<&ConfigFile>,
+  ) -> Result<Option<Lockfile>, AnyError> {
+    let filename = match flags.lock {
+      Some(ref lock) => PathBuf::from(lock),
+      None => match maybe_config_file {
+        Some(config_file) => {
+          if config_file.specifier.scheme() == "file" {
+            let mut path = config_file.specifier.to_file_path().unwrap();
+            path.set_file_name("deno.lock");
+            path
+          } else {
+            return Ok(None);
+          }
+        }
+        None => return Ok(None),
+      },
+    };
+
+    let lockfile = Self::new(filename, flags.lock_write)?;
+    Ok(Some(lockfile))
+  }
+
   pub fn new(filename: PathBuf, overwrite: bool) -> Result<Lockfile, AnyError> {
     // Writing a lock file always uses the new format.
-    let content = if overwrite {
-      LockfileContent {
-        version: "2".to_string(),
-        remote: BTreeMap::new(),
-        npm: NpmContent::default(),
+    if overwrite {
+      return Ok(Lockfile {
+        overwrite,
+        has_content_changed: false,
+        content: LockfileContent::empty(),
+        filename,
+      });
     }
+
+    let result = match std::fs::read_to_string(&filename) {
+      Ok(content) => Ok(content),
+      Err(e) => {
+        if e.kind() == std::io::ErrorKind::NotFound {
+          return Ok(Lockfile {
+            overwrite,
+            has_content_changed: false,
+            content: LockfileContent::empty(),
+            filename,
+          });
         } else {
-      let s = std::fs::read_to_string(&filename).with_context(|| {
-        format!("Unable to read lockfile: {}", filename.display())
+          Err(e)
+        }
+      }
+    };
+
+    let s = result.with_context(|| {
+      format!("Unable to read lockfile: \"{}\"", filename.display())
+    })?;
+    let value: serde_json::Value =
+      serde_json::from_str(&s).with_context(|| {
+        format!(
+          "Unable to parse contents of the lockfile \"{}\"",
+          filename.display()
+        )
       })?;
-      let value: serde_json::Value = serde_json::from_str(&s)
-        .context("Unable to parse contents of the lockfile")?;
     let version = value.get("version").and_then(|v| v.as_str());
-      if version == Some("2") {
-        serde_json::from_value::<LockfileContent>(value)
-          .context("Unable to parse lockfile")?
+    let content = if version == Some("2") {
+      serde_json::from_value::<LockfileContent>(value).with_context(|| {
+        format!(
+          "Unable to parse contents of the lockfile \"{}\"",
+          filename.display()
+        )
+      })?
     } else {
       // If there's no version field, we assume that user is using the old
       // version of the lockfile. We'll migrate it in-place into v2 and it
       // will be writte in v2 if user uses `--lock-write` flag.
-        let remote: BTreeMap<String, String> =
-          serde_json::from_value(value).context("Unable to parse lockfile")?;
+      let remote: BTreeMap<String, String> = serde_json::from_value(value)
+        .with_context(|| {
+          format!(
+            "Unable to parse contents of the lockfile \"{}\"",
+            filename.display()
+          )
+        })?;
       LockfileContent {
         version: "2".to_string(),
         remote,
         npm: NpmContent::default(),
       }
-      }
     };
 
     Ok(Lockfile {
@@ -209,10 +276,15 @@ impl Lockfile {
         .unwrap_or(&package.dist.shasum);
       if &package_info.integrity != integrity {
         return Err(LockfileError(format!(
-          "Integrity check failed for npm package: \"{}\".
-  Cache has \"{}\" and lockfile has \"{}\".
-  Use \"--lock-write\" flag to update the lockfile.",
-          package.id, integrity, package_info.integrity
+          "Integrity check failed for npm package: \"{}\". Unable to verify that the package
+is the same as when the lockfile was generated.
+
+This could be caused by:
+  * the lock file may be corrupt
+  * the source itself may be corrupt
+
+Use \"--lock-write\" flag to regenerate the lockfile at \"{}\".",
+          package.id, self.filename.display()
         )));
       }
     } else {
@@ -338,9 +410,9 @@ mod tests {
   }
 
   #[test]
-  fn new_nonexistent_lockfile() {
+  fn create_lockfile_for_nonexistent_path() {
     let file_path = PathBuf::from("nonexistent_lock_file.json");
-    assert!(Lockfile::new(file_path, false).is_err());
+    assert!(Lockfile::new(file_path, false).is_ok());
   }
 
   #[test]

cli/lsp/language_server.rs

@@ -2916,6 +2916,8 @@ impl Inner {
         ..Default::default()
       },
       self.maybe_config_file.clone(),
+      // TODO(#16510): add support for lockfile
+      None,
     );
     cli_options.set_import_map_specifier(self.maybe_import_map_uri.clone());
 

cli/proc_state.rs

@@ -168,9 +168,7 @@ impl ProcState {
       Some(progress_bar.clone()),
     )?;
 
-    let lockfile = cli_options
-      .resolve_lock_file()?
-      .map(|f| Arc::new(Mutex::new(f)));
+    let lockfile = cli_options.maybe_lock_file();
     let maybe_import_map_specifier =
       cli_options.resolve_import_map_specifier()?;
 
@@ -296,7 +294,6 @@ impl ProcState {
         npm_package_reqs.push(package_ref.req);
       }
     }
-
     let roots = roots
       .into_iter()
       .map(|s| (s, ModuleKind::Esm))
@@ -312,6 +309,13 @@ impl ProcState {
           self.options.type_check_mode() != TypeCheckMode::None,
           false,
         ) {
+          // TODO(bartlomieju): this is strange... ideally there should be only
+          // one codepath in `prepare_module_load` so we don't forget things
+          // like writing a lockfile. Figure a way to refactor this function.
+          if let Some(ref lockfile) = self.lockfile {
+            let g = lockfile.lock();
+            g.write()?;
+          }
           return result;
         }
       }

cli/tests/integration/npm_tests.rs

@@ -998,6 +998,54 @@ fn lock_file_missing_top_level_package() {
   );
 }
 
+#[test]
+fn auto_discover_lock_file() {
+  let _server = http_server();
+
+  let deno_dir = util::new_deno_dir();
+  let temp_dir = util::TempDir::new();
+
+  // write empty config file
+  temp_dir.write("deno.json", "{}");
+
+  // write a lock file with borked integrity
+  let lock_file_content = r#"{
+    "version": "2",
+    "remote": {},
+    "npm": {
+      "specifiers": { "@denotest/bin": "@denotest/bin@1.0.0" },
+      "packages": {
+        "@denotest/bin@1.0.0": {
+          "integrity": "sha512-foobar",
+          "dependencies": {}
+        }
+      }
+    }
+  }"#;
+  temp_dir.write("deno.lock", lock_file_content);
+
+  let deno = util::deno_cmd_with_deno_dir(&deno_dir)
+    .current_dir(temp_dir.path())
+    .arg("run")
+    .arg("--unstable")
+    .arg("-A")
+    .arg("npm:@denotest/bin/cli-esm")
+    .arg("test")
+    .envs(env_vars())
+    .stdout(Stdio::piped())
+    .stderr(Stdio::piped())
+    .spawn()
+    .unwrap();
+  let output = deno.wait_with_output().unwrap();
+  assert!(!output.status.success());
+  assert_eq!(output.status.code(), Some(10));
+
+  let stderr = String::from_utf8(output.stderr).unwrap();
+  assert!(stderr.contains(
+    "Integrity check failed for npm package: \"@denotest/bin@1.0.0\""
+  ));
+}
+
 fn env_vars_no_sync_download() -> Vec<(String, String)> {
   vec![
     ("DENO_NODE_COMPAT_URL".to_string(), util::std_file_url()),

cli/tests/integration/run_tests.rs

@@ -609,12 +609,6 @@ itest!(private_field_presence_no_check {
   output: "run/private_field_presence.ts.out",
 });
 
-itest!(lock_write_requires_lock {
-  args: "run --lock-write some_file.ts",
-  output: "run/lock_write_requires_lock.out",
-  exit_code: 1,
-});
-
 // TODO(bartlomieju): remove --unstable once Deno.spawn is stabilized
 itest!(lock_write_fetch {
   args:
@@ -3634,3 +3628,10 @@ fn websocket_server_idletimeout() {
 
   assert!(child.wait().unwrap().success());
 }
+
+itest!(auto_discover_lockfile {
+  args: "run run/auto_discover_lockfile/main.ts",
+  output: "run/auto_discover_lockfile/main.out",
+  http_server: true,
+  exit_code: 10,
+});

cli/tests/testdata/jsx/deno.lock

@@ -0,0 +1,7 @@
+{
+  "version": "2",
+  "remote": {
+    "http://localhost:4545/jsx/jsx-dev-runtime": "7cac3d940791b3c8e671b24f9678ca37d87d40487ed2b3720a2a40891aa6173d",
+    "http://localhost:4545/jsx/jsx-runtime": "7cac3d940791b3c8e671b24f9678ca37d87d40487ed2b3720a2a40891aa6173d"
+  }
+}

cli/tests/testdata/npm/lock_file/main.out

@@ -1,4 +1,9 @@
 Download [WILDCARD]
-error: Integrity check failed for npm package: "@babel/parser@7.19.0".
-  Cache has "sha512-74bEXKX2h+8rrfQUfsBfuZZHzsEs6Eql4pqy/T4Nn6Y9wNPggQOqD6z6pn5Bl8ZfysKouFZT/UXEH94ummEeQw==" and lockfile has "sha512-foobar!".
-  Use "--lock-write" flag to update the lockfile.
+error: Integrity check failed for npm package: "@babel/parser@7.19.0". Unable to verify that the package
+is the same as when the lockfile was generated.
+
+This could be caused by:
+  * the lock file may be corrupt
+  * the source itself may be corrupt
+
+Use "--lock-write" flag to regenerate the lockfile at "[WILDCARD]lock.json".

cli/tests/testdata/run/auto_discover_lockfile/deno.json

@@ -0,0 +1,3 @@
+{
+  "tasks": {}
+}

cli/tests/testdata/run/auto_discover_lockfile/deno.lock

@@ -0,0 +1,7 @@
+{
+  "version": "2",
+  "remote": {
+    "http://localhost:4545/subdir/mod2.ts": "cae1d3e9f3c38cd415ff52dff854be8f3d17d35f8d7b3d285e813fb0f6393a2f",
+    "http://localhost:4545/subdir/print_hello.ts": "foobar"
+  }
+}

cli/tests/testdata/run/auto_discover_lockfile/main.out

@@ -0,0 +1,5 @@
+Download http://localhost:4545/subdir/mod2.ts
+Download http://localhost:4545/subdir/print_hello.ts
+error: The source code is invalid, as it does not match the expected hash in the lock file.
+  Specifier: http://localhost:4545/subdir/print_hello.ts
+  Lock file: [WILDCARD]auto_discover_lockfile[WILDCARD]deno.lock

cli/tests/testdata/run/auto_discover_lockfile/main.ts

@@ -0,0 +1 @@
+import "http://localhost:4545/subdir/mod2.ts";

cli/tests/testdata/run/config_types/deno.lock

@@ -0,0 +1,6 @@
+{
+  "version": "2",
+  "remote": {
+    "http://localhost:4545/run/config_types/types.d.ts": "741c39165e37de0c12acc5c081841f4362487e3f17dc4cad7017b70af72c4605"
+  }
+}

cli/tests/testdata/run/lock_write_requires_lock.out

@@ -1,3 +0,0 @@
-error: The following required arguments were not provided:
-    --lock <FILE>
-[WILDCARD]