fix(npm): support dist tags specified in npm package dependencies (#16652)

Closes #16321

cli/npm/registry.rs

@@ -518,6 +518,12 @@ impl TestNpmRegistryApi {
       .insert(package_to.0.to_string(), package_to.1.to_string());
   }
 
+  pub fn add_dist_tag(&self, package_name: &str, tag: &str, version: &str) {
+    let mut infos = self.package_infos.lock();
+    let info = infos.get_mut(package_name).unwrap();
+    info.dist_tags.insert(tag.to_string(), version.to_string());
+  }
+
   pub fn add_peer_dependency(
     &self,
     package_from: (&str, &str),

cli/npm/resolution/graph.rs

@@ -411,19 +411,19 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
     }
   }
 
-  fn resolve_best_package_version_and_info(
+  fn resolve_best_package_version_and_info<'info>(
     &self,
     name: &str,
     version_matcher: &impl NpmVersionMatcher,
-    package_info: &NpmPackageInfo,
+    package_info: &'info NpmPackageInfo,
-  ) -> Result<VersionAndInfo, AnyError> {
+  ) -> Result<VersionAndInfo<'info>, AnyError> {
     if let Some(version) =
       self.resolve_best_package_version(name, version_matcher)
     {
       match package_info.versions.get(&version.to_string()) {
         Some(version_info) => Ok(VersionAndInfo {
           version,
-          info: version_info.clone(),
+          info: version_info,
         }),
         None => {
           bail!("could not find version '{}' for '{}'", version, name)
@@ -670,16 +670,21 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
   ) -> Result<Option<NpmPackageId>, AnyError> {
     fn find_matching_child<'a>(
       peer_dep: &NpmDependencyEntry,
+      peer_package_info: &NpmPackageInfo,
       children: impl Iterator<Item = &'a NpmPackageId>,
-    ) -> Option<NpmPackageId> {
+    ) -> Result<Option<NpmPackageId>, AnyError> {
       for child_id in children {
         if child_id.name == peer_dep.name
-          && peer_dep.version_req.satisfies(&child_id.version)
+          && version_req_satisfies(
+            &peer_dep.version_req,
+            &child_id.version,
+            peer_package_info,
+          )?
         {
-          return Some(child_id.clone());
+          return Ok(Some(child_id.clone()));
         }
       }
-      None
+      Ok(None)
     }
 
     // Peer dependencies are resolved based on its ancestors' siblings.
@@ -712,8 +717,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
       match &ancestor {
         NodeParent::Node(ancestor_node_id) => {
           let maybe_peer_dep_id = if ancestor_node_id.name == peer_dep.name
-            && peer_dep.version_req.satisfies(&ancestor_node_id.version)
+            && version_req_satisfies(
-          {
+              &peer_dep.version_req,
+              &ancestor_node_id.version,
+              peer_package_info,
+            )? {
             Some(ancestor_node_id.clone())
           } else {
             let ancestor = self.graph.borrow_node(ancestor_node_id);
@@ -731,7 +739,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
                 }
               }
             }
-            find_matching_child(peer_dep, ancestor.children.values())
+            find_matching_child(
+              peer_dep,
+              peer_package_info,
+              ancestor.children.values(),
+            )?
           };
           if let Some(peer_dep_id) = maybe_peer_dep_id {
             if existing_dep_id == Some(&peer_dep_id) {
@@ -751,9 +763,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
         }
         NodeParent::Req => {
           // in this case, the parent is the root so the children are all the package requirements
-          if let Some(child_id) =
+          if let Some(child_id) = find_matching_child(
-            find_matching_child(peer_dep, self.graph.package_reqs.values())
+            peer_dep,
-          {
+            peer_package_info,
+            self.graph.package_reqs.values(),
+          )? {
             if existing_dep_id == Some(&child_id) {
               return Ok(None); // do nothing, there's already an existing child dep id for this
             }
@@ -778,7 +792,7 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
     // to resolve based on the package info and will treat this just like any
     // other dependency when not optional
     if !peer_dep.kind.is_optional()
-    // only
+      // prefer the existing dep id if it exists
       && existing_dep_id.is_none()
     {
       self.analyze_dependency(
@@ -917,18 +931,17 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi>
 }
 
 #[derive(Clone)]
-struct VersionAndInfo {
+struct VersionAndInfo<'a> {
   version: NpmVersion,
-  info: NpmPackageVersionInfo,
+  info: &'a NpmPackageVersionInfo,
 }
 
-fn get_resolved_package_version_and_info(
+fn get_resolved_package_version_and_info<'a>(
   pkg_name: &str,
   version_matcher: &impl NpmVersionMatcher,
-  info: &NpmPackageInfo,
+  info: &'a NpmPackageInfo,
   parent: Option<&NpmPackageId>,
-) -> Result<VersionAndInfo, AnyError> {
+) -> Result<VersionAndInfo<'a>, AnyError> {
-  let mut maybe_best_version: Option<VersionAndInfo> = None;
   if let Some(tag) = version_matcher.tag() {
     // For when someone just specifies @types/node, we want to pull in a
     // "known good" version of @types/node that works well with Deno and
@@ -946,26 +959,9 @@ fn get_resolved_package_version_and_info(
       );
     }
 
-    if let Some(version) = info.dist_tags.get(tag) {
+    tag_to_version_info(info, tag)
-      match info.versions.get(version) {
-        Some(info) => {
-          return Ok(VersionAndInfo {
-            version: NpmVersion::parse(version)?,
-            info: info.clone(),
-          });
-        }
-        None => {
-          bail!(
-            "Could not find version '{}' referenced in dist-tag '{}'.",
-            version,
-            tag,
-          )
-        }
-      }
-    } else {
-      bail!("Could not find dist-tag '{}'.", tag)
-    }
   } else {
+    let mut maybe_best_version: Option<VersionAndInfo> = None;
     for version_info in info.versions.values() {
       let version = NpmVersion::parse(&version_info.version)?;
       if version_matcher.matches(&version) {
@@ -976,36 +972,73 @@ fn get_resolved_package_version_and_info(
         if is_best_version {
           maybe_best_version = Some(VersionAndInfo {
             version,
-            info: version_info.clone(),
+            info: version_info,
           });
         }
       }
     }
-  }
 
-  match maybe_best_version {
+    match maybe_best_version {
-    Some(v) => Ok(v),
+      Some(v) => Ok(v),
-    // If the package isn't found, it likely means that the user needs to use
+      // If the package isn't found, it likely means that the user needs to use
-    // `--reload` to get the latest npm package information. Although it seems
+      // `--reload` to get the latest npm package information. Although it seems
-    // like we could make this smart by fetching the latest information for
+      // like we could make this smart by fetching the latest information for
-    // this package here, we really need a full restart. There could be very
+      // this package here, we really need a full restart. There could be very
-    // interesting bugs that occur if this package's version was resolved by
+      // interesting bugs that occur if this package's version was resolved by
-    // something previous using the old information, then now being smart here
+      // something previous using the old information, then now being smart here
-    // causes a new fetch of the package information, meaning this time the
+      // causes a new fetch of the package information, meaning this time the
-    // previous resolution of this package's version resolved to an older
+      // previous resolution of this package's version resolved to an older
-    // version, but next time to a different version because it has new information.
+      // version, but next time to a different version because it has new information.
-    None => bail!(
+      None => bail!(
-      concat!(
+        concat!(
-        "Could not find npm package '{}' matching {}{}. ",
+          "Could not find npm package '{}' matching {}{}. ",
-        "Try retrieving the latest npm package information by running with --reload",
+          "Try retrieving the latest npm package information by running with --reload",
+        ),
+        pkg_name,
+        version_matcher.version_text(),
+        match parent {
+          Some(id) => format!(" as specified in {}", id.display()),
+          None => String::new(),
+        }
       ),
-      pkg_name,
+    }
-      version_matcher.version_text(),
+  }
-      match parent {
+}
-        Some(id) => format!(" as specified in {}", id.display()),
+
-        None => String::new(),
+fn version_req_satisfies(
+  version_req: &NpmVersionReq,
+  version: &NpmVersion,
+  package_info: &NpmPackageInfo,
+) -> Result<bool, AnyError> {
+  match version_req.tag() {
+    Some(tag) => {
+      let tag_version = tag_to_version_info(package_info, tag)?.version;
+      Ok(tag_version == *version)
+    }
+    None => Ok(version_req.matches(version)),
+  }
+}
+
+fn tag_to_version_info<'a>(
+  info: &'a NpmPackageInfo,
+  tag: &str,
+) -> Result<VersionAndInfo<'a>, AnyError> {
+  if let Some(version) = info.dist_tags.get(tag) {
+    match info.versions.get(version) {
+      Some(info) => Ok(VersionAndInfo {
+        version: NpmVersion::parse(version)?,
+        info,
+      }),
+      None => {
+        bail!(
+          "Could not find version '{}' referenced in dist-tag '{}'.",
+          version,
+          tag,
+        )
       }
-    ),
+    }
+  } else {
+    bail!("Could not find dist-tag '{}'.", tag)
   }
 }
 
@@ -1022,17 +1055,18 @@ mod test {
   fn test_get_resolved_package_version_and_info() {
     // dist tag where version doesn't exist
     let package_ref = NpmPackageReference::from_str("npm:test").unwrap();
+    let package_info = NpmPackageInfo {
+      name: "test".to_string(),
+      versions: HashMap::new(),
+      dist_tags: HashMap::from([(
+        "latest".to_string(),
+        "1.0.0-alpha".to_string(),
+      )]),
+    };
     let result = get_resolved_package_version_and_info(
       "test",
       &package_ref.req,
-      &NpmPackageInfo {
+      &package_info,
-        name: "test".to_string(),
-        versions: HashMap::new(),
-        dist_tags: HashMap::from([(
-          "latest".to_string(),
-          "1.0.0-alpha".to_string(),
-        )]),
-      },
       None,
     );
     assert_eq!(
@@ -1042,26 +1076,27 @@ mod test {
 
     // dist tag where version is a pre-release
     let package_ref = NpmPackageReference::from_str("npm:test").unwrap();
+    let package_info = NpmPackageInfo {
+      name: "test".to_string(),
+      versions: HashMap::from([
+        ("0.1.0".to_string(), NpmPackageVersionInfo::default()),
+        (
+          "1.0.0-alpha".to_string(),
+          NpmPackageVersionInfo {
+            version: "0.1.0-alpha".to_string(),
+            ..Default::default()
+          },
+        ),
+      ]),
+      dist_tags: HashMap::from([(
+        "latest".to_string(),
+        "1.0.0-alpha".to_string(),
+      )]),
+    };
     let result = get_resolved_package_version_and_info(
       "test",
       &package_ref.req,
-      &NpmPackageInfo {
+      &package_info,
-        name: "test".to_string(),
-        versions: HashMap::from([
-          ("0.1.0".to_string(), NpmPackageVersionInfo::default()),
-          (
-            "1.0.0-alpha".to_string(),
-            NpmPackageVersionInfo {
-              version: "0.1.0-alpha".to_string(),
-              ..Default::default()
-            },
-          ),
-        ]),
-        dist_tags: HashMap::from([(
-          "latest".to_string(),
-          "1.0.0-alpha".to_string(),
-        )]),
-      },
       None,
     );
     assert_eq!(result.unwrap().version.to_string(), "1.0.0-alpha");
@@ -2226,6 +2261,80 @@ mod test {
     );
   }
 
+  #[tokio::test]
+  async fn resolve_dep_and_peer_dist_tag() {
+    let api = TestNpmRegistryApi::default();
+    api.ensure_package_version("package-a", "1.0.0");
+    api.ensure_package_version("package-b", "2.0.0");
+    api.ensure_package_version("package-b", "3.0.0");
+    api.ensure_package_version("package-c", "1.0.0");
+    api.ensure_package_version("package-d", "1.0.0");
+    api.add_dependency(("package-a", "1.0.0"), ("package-b", "some-tag"));
+    api.add_dependency(("package-a", "1.0.0"), ("package-d", "1.0.0"));
+    api.add_dependency(("package-a", "1.0.0"), ("package-c", "1.0.0"));
+    api.add_peer_dependency(("package-c", "1.0.0"), ("package-d", "other-tag"));
+    api.add_dist_tag("package-b", "some-tag", "2.0.0");
+    api.add_dist_tag("package-d", "other-tag", "1.0.0");
+
+    let (packages, package_reqs) =
+      run_resolver_and_get_output(api, vec!["npm:package-a@1.0"]).await;
+    assert_eq!(
+      packages,
+      vec![
+        NpmResolutionPackage {
+          id: NpmPackageId::from_serialized("package-a@1.0.0_package-d@1.0.0")
+            .unwrap(),
+          copy_index: 0,
+          dependencies: HashMap::from([
+            (
+              "package-b".to_string(),
+              NpmPackageId::from_serialized("package-b@2.0.0").unwrap(),
+            ),
+            (
+              "package-c".to_string(),
+              NpmPackageId::from_serialized("package-c@1.0.0_package-d@1.0.0")
+                .unwrap(),
+            ),
+            (
+              "package-d".to_string(),
+              NpmPackageId::from_serialized("package-d@1.0.0").unwrap(),
+            ),
+          ]),
+          dist: Default::default(),
+        },
+        NpmResolutionPackage {
+          id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(),
+          copy_index: 0,
+          dependencies: HashMap::new(),
+          dist: Default::default(),
+        },
+        NpmResolutionPackage {
+          id: NpmPackageId::from_serialized("package-c@1.0.0_package-d@1.0.0")
+            .unwrap(),
+          copy_index: 0,
+          dependencies: HashMap::from([(
+            "package-d".to_string(),
+            NpmPackageId::from_serialized("package-d@1.0.0").unwrap(),
+          ),]),
+          dist: Default::default(),
+        },
+        NpmResolutionPackage {
+          id: NpmPackageId::from_serialized("package-d@1.0.0").unwrap(),
+          copy_index: 0,
+          dependencies: HashMap::new(),
+          dist: Default::default(),
+        },
+      ]
+    );
+    assert_eq!(
+      package_reqs,
+      vec![(
+        "package-a@1.0".to_string(),
+        "package-a@1.0.0_package-d@1.0.0".to_string()
+      ),]
+    );
+  }
+
   async fn run_resolver_and_get_output(
     api: TestNpmRegistryApi,
     reqs: Vec<&str>,

cli/npm/semver/mod.rs

@@ -25,6 +25,14 @@ mod specifier;
 // A lot of the below is a re-implementation of parts of https://github.com/npm/node-semver
 // which is Copyright (c) Isaac Z. Schlueter and Contributors (ISC License)
 
+pub fn is_valid_npm_tag(value: &str) -> bool {
+  // a valid tag is anything that doesn't get url encoded
+  // https://github.com/npm/npm-package-arg/blob/103c0fda8ed8185733919c7c6c73937cfb2baf3a/lib/npa.js#L399-L401
+  value
+    .chars()
+    .all(|c| c.is_alphanumeric() || matches!(c, '-' | '_' | '.' | '~'))
+}
+
 #[derive(
   Clone, Debug, PartialEq, Eq, Default, Hash, Serialize, Deserialize,
 )]
@@ -164,20 +172,35 @@ fn parse_npm_version(input: &str) -> ParseResult<NpmVersion> {
   ))
 }
 
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+enum NpmVersionReqInner {
+  RangeSet(VersionRangeSet),
+  Tag(String),
+}
+
 /// A version requirement found in an npm package's dependencies.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct NpmVersionReq {
   raw_text: String,
-  range_set: VersionRangeSet,
+  inner: NpmVersionReqInner,
 }
 
 impl NpmVersionMatcher for NpmVersionReq {
   fn tag(&self) -> Option<&str> {
-    None
+    match &self.inner {
+      NpmVersionReqInner::RangeSet(_) => None,
+      NpmVersionReqInner::Tag(tag) => Some(tag.as_str()),
+    }
   }
 
   fn matches(&self, version: &NpmVersion) -> bool {
-    self.satisfies(version)
+    match &self.inner {
+      NpmVersionReqInner::RangeSet(range_set) => range_set.satisfies(version),
+      NpmVersionReqInner::Tag(_) => panic!(
+        "programming error: cannot use matches with a tag: {}",
+        self.raw_text
+      ),
+    }
   }
 
   fn version_text(&self) -> String {
@@ -197,16 +220,12 @@ impl NpmVersionReq {
     with_failure_handling(parse_npm_version_req)(text)
       .with_context(|| format!("Invalid npm version requirement '{}'.", text))
   }
-
-  pub fn satisfies(&self, version: &NpmVersion) -> bool {
-    self.range_set.satisfies(version)
-  }
 }
 
 fn parse_npm_version_req(input: &str) -> ParseResult<NpmVersionReq> {
-  map(range_set, |set| NpmVersionReq {
+  map(inner, |inner| NpmVersionReq {
     raw_text: input.to_string(),
-    range_set: set,
+    inner,
   })(input)
 }
 
@@ -229,14 +248,97 @@ fn parse_npm_version_req(input: &str) -> ParseResult<NpmVersionReq> {
 // part       ::= nr | [-0-9A-Za-z]+
 
 // range-set ::= range ( logical-or range ) *
-fn range_set(input: &str) -> ParseResult<VersionRangeSet> {
+fn inner(input: &str) -> ParseResult<NpmVersionReqInner> {
   if input.is_empty() {
-    return Ok((input, VersionRangeSet(vec![VersionRange::all()])));
+    return Ok((
+      input,
+      NpmVersionReqInner::RangeSet(VersionRangeSet(vec![VersionRange::all()])),
+    ));
   }
-  map(if_not_empty(separated_list(range, logical_or)), |ranges| {
+
-    // filter out the ranges that won't match anything for the tests
+  let (input, mut ranges) =
-    VersionRangeSet(ranges.into_iter().filter(|r| !r.is_none()).collect())
+    separated_list(range_or_invalid, logical_or)(input)?;
-  })(input)
+
+  if ranges.len() == 1 {
+    match ranges.remove(0) {
+      RangeOrInvalid::Invalid(invalid) => {
+        if is_valid_npm_tag(invalid.text) {
+          return Ok((
+            input,
+            NpmVersionReqInner::Tag(invalid.text.to_string()),
+          ));
+        } else {
+          return Err(invalid.failure);
+        }
+      }
+      RangeOrInvalid::Range(range) => {
+        // add it back
+        ranges.push(RangeOrInvalid::Range(range));
+      }
+    }
+  }
+
+  let ranges = ranges
+    .into_iter()
+    .filter_map(|r| r.into_range())
+    .collect::<Vec<_>>();
+  Ok((input, NpmVersionReqInner::RangeSet(VersionRangeSet(ranges))))
+}
+
+enum RangeOrInvalid<'a> {
+  Range(VersionRange),
+  Invalid(InvalidRange<'a>),
+}
+
+impl<'a> RangeOrInvalid<'a> {
+  pub fn into_range(self) -> Option<VersionRange> {
+    match self {
+      RangeOrInvalid::Range(r) => {
+        if r.is_none() {
+          None
+        } else {
+          Some(r)
+        }
+      }
+      RangeOrInvalid::Invalid(_) => None,
+    }
+  }
+}
+
+struct InvalidRange<'a> {
+  failure: ParseError<'a>,
+  text: &'a str,
+}
+
+fn range_or_invalid(input: &str) -> ParseResult<RangeOrInvalid> {
+  let range_result =
+    map_res(map(range, RangeOrInvalid::Range), |result| match result {
+      Ok((input, range)) => {
+        let is_end = input.is_empty() || logical_or(input).is_ok();
+        if is_end {
+          Ok((input, range))
+        } else {
+          ParseError::backtrace()
+        }
+      }
+      Err(err) => Err(err),
+    })(input);
+  match range_result {
+    Ok(result) => Ok(result),
+    Err(failure) => {
+      let (input, text) = invalid_range(input)?;
+      Ok((
+        input,
+        RangeOrInvalid::Invalid(InvalidRange { failure, text }),
+      ))
+    }
+  }
+}
+
+fn invalid_range(input: &str) -> ParseResult<&str> {
+  let end_index = input.find("||").unwrap_or(input.len());
+  let text = input[..end_index].trim();
+  Ok((&input[end_index..], text))
 }
 
 // range ::= hyphen | simple ( ' ' simple ) * | ''
@@ -505,7 +607,9 @@ mod tests {
 
   #[test]
   pub fn npm_version_req_ranges() {
-    let tester = NpmVersionReqTester::new(">= 2.1.2 < 3.0.0 || 5.x");
+    let tester = NpmVersionReqTester::new(
+      ">= 2.1.2 < 3.0.0 || 5.x || ignored-invalid-range || $#$%^#$^#$^%@#$%SDF|||",
+    );
     assert!(!tester.matches("2.1.1"));
     assert!(tester.matches("2.1.2"));
     assert!(tester.matches("2.9.9"));
@@ -515,6 +619,12 @@ mod tests {
     assert!(!tester.matches("6.1.0"));
   }
 
+  #[test]
+  pub fn npm_version_req_with_tag() {
+    let req = NpmVersionReq::parse("latest").unwrap();
+    assert_eq!(req.inner, NpmVersionReqInner::Tag("latest".to_string()));
+  }
+
   macro_rules! assert_cmp {
     ($a:expr, $b:expr, $expected:expr) => {
       assert_eq!(
@@ -729,7 +839,7 @@ mod tests {
       let range = NpmVersionReq::parse(range_text).unwrap();
       let expected_range = NpmVersionReq::parse(expected).unwrap();
       assert_eq!(
-        range.range_set, expected_range.range_set,
+        range.inner, expected_range.inner,
         "failed for {} and {}",
         range_text, expected
       );
@@ -853,7 +963,7 @@ mod tests {
       let req = NpmVersionReq::parse(req_text).unwrap();
       let version = NpmVersion::parse(version_text).unwrap();
       assert!(
-        req.satisfies(&version),
+        req.matches(&version),
         "Checking {} satisfies {}",
         req_text,
         version_text
@@ -952,7 +1062,7 @@ mod tests {
       let req = NpmVersionReq::parse(req_text).unwrap();
       let version = NpmVersion::parse(version_text).unwrap();
       assert!(
-        !req.satisfies(&version),
+        !req.matches(&version),
         "Checking {} not satisfies {}",
         req_text,
         version_text

cli/npm/semver/specifier.rs

@@ -6,6 +6,7 @@ use monch::*;
 use serde::Deserialize;
 use serde::Serialize;
 
+use super::is_valid_npm_tag;
 use super::range::Partial;
 use super::range::VersionRange;
 use super::range::XRange;
@@ -55,7 +56,7 @@ fn parse_npm_specifier(input: &str) -> ParseResult<SpecifierVersionReq> {
   map_res(version_range, |result| {
     let (new_input, range_result) = match result {
       Ok((input, range)) => (input, Ok(range)),
-      // use an empty string because we'll consider the tag
+      // use an empty string because we'll consider it a tag
       Err(err) => ("", Err(err)),
     };
     Ok((
@@ -65,9 +66,7 @@ fn parse_npm_specifier(input: &str) -> ParseResult<SpecifierVersionReq> {
         inner: match range_result {
           Ok(range) => SpecifierVersionReqInner::Range(range),
           Err(err) => {
-            // npm seems to be extremely lax on what it supports for a dist-tag (any non-valid semver range),
+            if !is_valid_npm_tag(input) {
-            // so just make any error here be a dist tag unless it starts or ends with whitespace
-            if input.trim() != input {
               return Err(err);
             } else {
               SpecifierVersionReqInner::Tag(input.to_string())