Better PEP 639 from uv implementation (#32)

Since writing the initial implementation, the uv implementation got some improvements, which I'm merging back.

Author: konstin

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "pyproject-toml"
-version = "0.13.4"
+version = "0.13.5"
 description = "pyproject.toml parser in Rust"
 edition = "2021"
 license = "MIT"

Cargo.toml

@@ -1,5 +1,10 @@
 # Changelog
 
+## 0.13.5
+
+* Better PEP 639 `license-files` glob validation and error messages from uv 
+* A public `check_pep639_glob` function for using the PEP 639 support with a different glob crate.
+
 ## 0.13.4
 
 * Update pep440_rs to 0.7.2

Changelog.md

@@ -2,7 +2,7 @@
 mod pep639_glob;
 
 #[cfg(feature = "pep639-glob")]
-pub use pep639_glob::{parse_pep639_glob, Pep639GlobError};
+pub use pep639_glob::{check_pep639_glob, parse_pep639_glob, Pep639GlobError};
 
 pub mod pep735_resolve;
 

src/lib.rs

@@ -7,12 +7,26 @@ use thiserror::Error;
 pub enum Pep639GlobError {
     #[error(transparent)]
     PatternError(#[from] PatternError),
-    #[error("The parent directory operator (`..`) at position {pos} is not allowed in license file globs")]
-    ParentDirectory { pos: usize },
-    #[error("Glob contains invalid character at position {pos}: `{invalid}`")]
-    InvalidCharacter { pos: usize, invalid: char },
-    #[error("Glob contains invalid character in range at position {pos}: `{invalid}`")]
-    InvalidCharacterRange { pos: usize, invalid: char },
+    #[error(
+        "The parent directory operator (`..`) at position {pos} is not allowed in glob: `{glob}`"
+    )]
+    ParentDirectory { glob: String, pos: usize },
+    #[error("Invalid character `{invalid}` at position {pos} in glob: `{glob}`")]
+    InvalidCharacter {
+        glob: String,
+        pos: usize,
+        invalid: char,
+    },
+    #[error("Only forward slashes are allowed as path separator, invalid character at position {pos} in glob: `{glob}`")]
+    InvalidBackslash { glob: String, pos: usize },
+    #[error("Invalid character `{invalid}` in range at position {pos} in glob: `{glob}`")]
+    InvalidCharacterRange {
+        glob: String,
+        pos: usize,
+        invalid: char,
+    },
+    #[error("Too many at stars at position {pos} in glob: `{glob}`")]
+    TooManyStars { glob: String, pos: usize },
 }
 
 /// Parse a PEP 639 `license-files` glob
@@ -44,37 +58,91 @@ pub enum Pep639GlobError {
 /// > invalid. Projects MUST NOT use such values.
 /// > Tools consuming this field MAY reject invalid values with an error.
 pub fn parse_pep639_glob(glob: &str) -> Result<Pattern, Pep639GlobError> {
+    check_pep639_glob(glob)?;
+    Ok(Pattern::new(glob)?)
+}
+
+/// Check if a glob pattern is valid according to PEP 639 rules.
+///
+/// See [parse_pep639_glob].
+pub fn check_pep639_glob(glob: &str) -> Result<(), Pep639GlobError> {
     let mut chars = glob.chars().enumerate().peekable();
     // A `..` is on a parent directory indicator at the start of the string or after a directory
     // separator.
     let mut start_or_slash = true;
     while let Some((pos, c)) = chars.next() {
-        if c.is_alphanumeric() || matches!(c, '_' | '-' | '*' | '?') {
+        // `***` or `**literals` can be correctly represented with less stars. They are banned by
+        // `glob`, they are allowed by `globset` and PEP 639 is ambiguous, so we're filtering them
+        // out.
+        if c == '*' {
+            let mut star_run = 1;
+            while let Some((_, c)) = chars.peek() {
+                if *c == '*' {
+                    star_run += 1;
+                    chars.next();
+                } else {
+                    break;
+                }
+            }
+            if star_run >= 3 {
+                return Err(Pep639GlobError::TooManyStars {
+                    glob: glob.to_string(),
+                    // We don't update pos for the stars.
+                    pos,
+                });
+            } else if star_run == 2 {
+                if let Some((_, c)) = chars.peek() {
+                    if *c != '/' {
+                        return Err(Pep639GlobError::TooManyStars {
+                            glob: glob.to_string(),
+                            // We don't update pos for the stars.
+                            pos,
+                        });
+                    }
+                }
+            }
+            start_or_slash = false;
+        } else if c.is_alphanumeric() || matches!(c, '_' | '-' | '?') {
             start_or_slash = false;
         } else if c == '.' {
             if start_or_slash && matches!(chars.peek(), Some((_, '.'))) {
-                return Err(Pep639GlobError::ParentDirectory { pos });
+                return Err(Pep639GlobError::ParentDirectory {
+                    pos,
+                    glob: glob.to_string(),
+                });
             }
             start_or_slash = false;
         } else if c == '/' {
             start_or_slash = true;
         } else if c == '[' {
             for (pos, c) in chars.by_ref() {
-                // TODO: https://discuss.python.org/t/pep-639-round-3-improving-license-clarity-with-better-package-metadata/53020/98
                 if c.is_alphanumeric() || matches!(c, '_' | '-' | '.') {
                     // Allowed.
                 } else if c == ']' {
                     break;
                 } else {
-                    return Err(Pep639GlobError::InvalidCharacterRange { pos, invalid: c });
+                    return Err(Pep639GlobError::InvalidCharacterRange {
+                        glob: glob.to_string(),
+                        pos,
+                        invalid: c,
+                    });
                 }
             }
             start_or_slash = false;
+        } else if c == '\\' {
+            return Err(Pep639GlobError::InvalidBackslash {
+                glob: glob.to_string(),
+                pos,
+            });
         } else {
-            return Err(Pep639GlobError::InvalidCharacter { pos, invalid: c });
+            return Err(Pep639GlobError::InvalidCharacter {
+                glob: glob.to_string(),
+                pos,
+                invalid: c,
+            });
         }
     }
-    Ok(Pattern::new(glob)?)
+    Ok(())
 }
 
 #[cfg(test)]
@@ -87,28 +155,48 @@ mod tests {
         let parse_err = |glob| parse_pep639_glob(glob).unwrap_err().to_string();
         assert_snapshot!(
             parse_err(".."),
-            @"The parent directory operator (`..`) at position 0 is not allowed in license file globs"
+            @"The parent directory operator (`..`) at position 0 is not allowed in glob: `..`"
         );
         assert_snapshot!(
             parse_err("licenses/.."),
-            @"The parent directory operator (`..`) at position 9 is not allowed in license file globs"
+            @"The parent directory operator (`..`) at position 9 is not allowed in glob: `licenses/..`"
         );
         assert_snapshot!(
             parse_err("licenses/LICEN!E.txt"),
-            @"Glob contains invalid character at position 14: `!`"
+            @"Invalid character `!` at position 14 in glob: `licenses/LICEN!E.txt`"
         );
         assert_snapshot!(
             parse_err("licenses/LICEN[!C]E.txt"),
-            @"Glob contains invalid character in range at position 15: `!`"
+            @"Invalid character `!` in range at position 15 in glob: `licenses/LICEN[!C]E.txt`"
         );
         assert_snapshot!(
             parse_err("licenses/LICEN[C?]E.txt"),
-            @"Glob contains invalid character in range at position 16: `?`"
+            @"Invalid character `?` in range at position 16 in glob: `licenses/LICEN[C?]E.txt`"
+        );
+        assert_snapshot!(
+            parse_err("******"),
+            @"Too many at stars at position 0 in glob: `******`"
+        );
+        assert_snapshot!(
+            parse_err("licenses/**license"),
+            @"Too many at stars at position 9 in glob: `licenses/**license`"
+        );
+        assert_snapshot!(
+            parse_err("licenses/***/licenses.csv"),
+            @"Too many at stars at position 9 in glob: `licenses/***/licenses.csv`"
         );
-        assert_snapshot!(parse_err("******"), @"Pattern syntax error near position 2: wildcards are either regular `*` or recursive `**`");
         assert_snapshot!(
             parse_err(r"licenses\eula.txt"),
-            @r"Glob contains invalid character at position 8: `\`"
+            @r"Only forward slashes are allowed as path separator, invalid character at position 8 in glob: `licenses\eula.txt`"
+        );
+        assert_snapshot!(
+            parse_err(r"**/@test"),
+            @"Invalid character `@` at position 3 in glob: `**/@test`"
+        );
+        // Backslashes are not allowed
+        assert_snapshot!(
+            parse_err(r"public domain/Gulliver\\'s Travels.txt"),
+            @r"Invalid character ` ` at position 6 in glob: `public domain/Gulliver\\'s Travels.txt`"
         );
     }
 
@@ -128,6 +216,7 @@ mod tests {
             "licenses/라이센스*.txt",
             "licenses/ライセンス*.txt",
             "licenses/执照*.txt",
+            "src/**",
         ];
         for case in cases {
             parse_pep639_glob(case).unwrap();