From 5b19079a9f6b36194db30688d374d400951a217c Mon Sep 17 00:00:00 2001 From: cfredric Date: Mon, 29 Nov 2021 12:36:15 -0500 Subject: [PATCH 1/4] Add `is_absolute` and implicit target support to label.rs. --- util/label/label.rs | 83 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 13 deletions(-) diff --git a/util/label/label.rs b/util/label/label.rs index e25770d5db..9c617fd217 100644 --- a/util/label/label.rs +++ b/util/label/label.rs @@ -11,9 +11,15 @@ use label_error::LabelError; pub fn analyze(input: &'_ str) -> Result> { let label = input; let (input, repository_name) = consume_repository_name(input, label)?; + let is_absolute = input.starts_with("//"); let (input, package_name) = consume_package_name(input, label)?; let name = consume_name(input, label)?; - Ok(Label::new(repository_name, package_name, name)) + let name = match (package_name, name) { + (None, None) => return Err(LabelError(err(label, "labels must have a package and/or a target."))), + (Some(package_name), None) => name_from_package(package_name), + (_, Some(name)) => name, + }; + Ok(Label::new(repository_name, package_name, name, is_absolute)) } #[derive(Debug, PartialEq)] @@ -21,6 +27,7 @@ pub struct Label<'s> { pub repository_name: Option<&'s str>, pub package_name: Option<&'s str>, pub name: &'s str, + is_absolute: bool, } type Result = core::result::Result; @@ -30,11 +37,13 @@ impl<'s> Label<'s> { repository_name: Option<&'s str>, package_name: Option<&'s str>, name: &'s str, + is_absolute: bool, ) -> Label<'s> { Label { repository_name, package_name, name, + is_absolute, } } @@ -44,6 +53,10 @@ impl<'s> Label<'s> { None => vec![], } } + + pub fn is_absolute(&self) -> bool { + self.is_absolute + } } fn err<'s>(label: &'s str, msg: &'s str) -> String { @@ -108,7 +121,7 @@ fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, "'//' cannot appear in the middle of the label.", ))); } - return Ok((input, None)); + return Ok((input.rsplit_once('/').map(|t| t.1).unwrap_or(input), None)) } }; @@ -165,26 +178,25 @@ fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, Ok((rest, Some(package_name))) } -fn consume_name<'s>(input: &'s str, label: &'s str) -> Result<&'s str> { +fn consume_name<'s>(input: &'s str, label: &'s str) -> Result> { if input.is_empty() { - return Err(LabelError(err(label, "empty target name."))); + return Ok(None); } - let name = if let Some(stripped) = input.strip_prefix(':') { - stripped - } else { - input - }; - if name.is_empty() { + if input == ":" { return Err(LabelError(err(label, "empty target name."))); } + let name = input.strip_prefix(':').or_else(|| input.strip_prefix('/')).unwrap_or(input); if name.starts_with('/') { return Err(LabelError(err( label, "target names may not start with '/'.", ))); } + Ok(Some(name)) +} - Ok(name) +fn name_from_package(package_name: &str) -> &str { + package_name.rsplit_once('/').map(|tup| tup.1).unwrap_or(package_name) } #[cfg(test)] @@ -194,11 +206,12 @@ mod tests { #[test] fn test_new() { assert_eq!( - Label::new(Some("repo"), Some("foo/bar"), "baz"), + Label::new(Some("repo"), Some("foo/bar"), "baz", true), Label { repository_name: Some("repo"), package_name: Some("foo/bar"), - name: "baz" + name: "baz", + is_absolute: true, } ); } @@ -209,6 +222,17 @@ mod tests { assert_eq!(analyze("@//:foo")?.repository_name, None); assert_eq!(analyze("//:foo")?.repository_name, None); assert_eq!(analyze(":foo")?.repository_name, None); + + assert_eq!(analyze("@repo//foo/bar")?.repository_name, Some("repo")); + assert_eq!(analyze("@//foo/bar")?.repository_name, None); + assert_eq!(analyze("//foo/bar")?.repository_name, None); + assert_eq!(analyze("foo/bar")?.repository_name, None); + + assert_eq!(analyze("@repo//foo")?.repository_name, Some("repo")); + assert_eq!(analyze("@//foo")?.repository_name, None); + assert_eq!(analyze("//foo")?.repository_name, None); + assert_eq!(analyze("foo")?.repository_name, None); + assert_eq!( analyze("@foo:bar"), Err(LabelError( @@ -286,6 +310,14 @@ mod tests { )) ); + assert_eq!(analyze("@repo//foo/bar")?.package_name, Some("foo/bar")); + assert_eq!(analyze("//foo/bar")?.package_name, Some("foo/bar")); + assert_eq!(analyze("foo/bar")?.package_name, None); + + assert_eq!(analyze("@repo//foo")?.package_name, Some("foo")); + assert_eq!(analyze("//foo")?.package_name, Some("foo")); + assert_eq!(analyze("foo")?.package_name, None); + Ok(()) } @@ -310,6 +342,14 @@ mod tests { )) ); + assert_eq!(analyze("@repo//foo/bar")?.name, "bar"); + assert_eq!(analyze("//foo/bar")?.name, "bar"); + assert_eq!(analyze("foo/bar")?.name, "bar"); + + assert_eq!(analyze("@repo//foo")?.name, "foo"); + assert_eq!(analyze("//foo")?.name, "foo"); + assert_eq!(analyze("foo")?.name, "foo"); + Ok(()) } @@ -324,4 +364,21 @@ mod tests { Ok(()) } + + #[test] + fn test_is_absolute() -> Result<()> { + assert!(analyze("@repo//foo/bar:baz")?.is_absolute()); + assert!(analyze("//foo/bar:baz")?.is_absolute()); + assert!(!analyze("foo/bar:baz")?.is_absolute()); + + assert!(analyze("@repo//foo/bar")?.is_absolute()); + assert!(analyze("//foo/bar")?.is_absolute()); + assert!(!analyze("foo/bar")?.is_absolute()); + + assert!(analyze("@repo//foo")?.is_absolute()); + assert!(analyze("//foo")?.is_absolute()); + assert!(!analyze("foo")?.is_absolute()); + + Ok(()) + } } From ec6766a0887f24b74923c7336a45c25da2799405 Mon Sep 17 00:00:00 2001 From: cfredric Date: Mon, 29 Nov 2021 13:15:29 -0500 Subject: [PATCH 2/4] Refactor package_name splitting. --- util/label/label.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/util/label/label.rs b/util/label/label.rs index 9c617fd217..13f6875268 100644 --- a/util/label/label.rs +++ b/util/label/label.rs @@ -106,15 +106,10 @@ fn consume_repository_name<'s>( } fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, Option<&'s str>)> { - let colon_pos = input.find(':'); - let start_pos; - let mut is_absolute = false; - if input.starts_with("//") { - start_pos = 2; - is_absolute = true; - } else { - start_pos = 0; - if colon_pos.is_none() { + let is_absolute = input.starts_with("//"); + + let (package_name, rest) = match (is_absolute, input.find(':')) { + (false, None) => { if input.contains("//") { return Err(LabelError(err( label, @@ -123,12 +118,19 @@ fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, } return Ok((input.rsplit_once('/').map(|t| t.1).unwrap_or(input), None)) } + (_, colon_pos) => { + let (input, colon_pos) = if is_absolute { + (&input[2..], colon_pos.map(|cp| cp - 2)) + } else { + (input, colon_pos) + }; + match colon_pos { + Some(colon_pos) => (&input[0..colon_pos], &input[colon_pos..]), + None => (input, ""), + } + } }; - let (package_name, rest) = match colon_pos { - Some(colon_pos) => (&input[start_pos..colon_pos], &input[colon_pos..]), - None => (&input[start_pos..], ""), - }; if package_name.is_empty() { return Ok((rest, None)); } From d80e10efbd5a622a333ba628a37948da025b0f40 Mon Sep 17 00:00:00 2001 From: cfredric Date: Tue, 30 Nov 2021 16:54:23 -0500 Subject: [PATCH 3/4] Forbid relative package references. Run rustfmt. --- util/label/label.rs | 150 ++++++++++++++++++++++++++++---------------- 1 file changed, 95 insertions(+), 55 deletions(-) diff --git a/util/label/label.rs b/util/label/label.rs index 13f6875268..2b73a6a8af 100644 --- a/util/label/label.rs +++ b/util/label/label.rs @@ -11,15 +11,19 @@ use label_error::LabelError; pub fn analyze(input: &'_ str) -> Result> { let label = input; let (input, repository_name) = consume_repository_name(input, label)?; - let is_absolute = input.starts_with("//"); let (input, package_name) = consume_package_name(input, label)?; let name = consume_name(input, label)?; let name = match (package_name, name) { - (None, None) => return Err(LabelError(err(label, "labels must have a package and/or a target."))), + (None, None) => { + return Err(LabelError(err( + label, + "labels must have a package and/or a target.", + ))) + } (Some(package_name), None) => name_from_package(package_name), (_, Some(name)) => name, }; - Ok(Label::new(repository_name, package_name, name, is_absolute)) + Ok(Label::new(repository_name, package_name, name)) } #[derive(Debug, PartialEq)] @@ -27,7 +31,6 @@ pub struct Label<'s> { pub repository_name: Option<&'s str>, pub package_name: Option<&'s str>, pub name: &'s str, - is_absolute: bool, } type Result = core::result::Result; @@ -37,13 +40,11 @@ impl<'s> Label<'s> { repository_name: Option<&'s str>, package_name: Option<&'s str>, name: &'s str, - is_absolute: bool, ) -> Label<'s> { Label { repository_name, package_name, name, - is_absolute, } } @@ -53,10 +54,6 @@ impl<'s> Label<'s> { None => vec![], } } - - pub fn is_absolute(&self) -> bool { - self.is_absolute - } } fn err<'s>(label: &'s str, msg: &'s str) -> String { @@ -106,17 +103,23 @@ fn consume_repository_name<'s>( } fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, Option<&'s str>)> { - let is_absolute = input.starts_with("//"); + let is_absolute = match input.rfind("//") { + None => false, + Some(0) => true, + Some(_) => { + return Err(LabelError(err( + label, + "'//' cannot appear in the middle of the label.", + ))); + } + }; let (package_name, rest) = match (is_absolute, input.find(':')) { - (false, None) => { - if input.contains("//") { - return Err(LabelError(err( - label, - "'//' cannot appear in the middle of the label.", - ))); - } - return Ok((input.rsplit_once('/').map(|t| t.1).unwrap_or(input), None)) + (false, colon_pos) if colon_pos.map_or(true, |pos| pos != 0) => { + return Err(LabelError(err( + label, + "relative packages are not permitted.", + ))); } (_, colon_pos) => { let (input, colon_pos) = if is_absolute { @@ -134,12 +137,6 @@ fn consume_package_name<'s>(input: &'s str, label: &'s str) -> Result<(&'s str, if package_name.is_empty() { return Ok((rest, None)); } - if package_name.contains("//") { - return Err(LabelError(err( - label, - "'//' cannot appear in the middle of the label.", - ))); - } if !package_name.chars().all(|c| { c.is_ascii_alphanumeric() @@ -187,7 +184,10 @@ fn consume_name<'s>(input: &'s str, label: &'s str) -> Result> { if input == ":" { return Err(LabelError(err(label, "empty target name."))); } - let name = input.strip_prefix(':').or_else(|| input.strip_prefix('/')).unwrap_or(input); + let name = input + .strip_prefix(':') + .or_else(|| input.strip_prefix('/')) + .unwrap_or(input); if name.starts_with('/') { return Err(LabelError(err( label, @@ -198,7 +198,10 @@ fn consume_name<'s>(input: &'s str, label: &'s str) -> Result> { } fn name_from_package(package_name: &str) -> &str { - package_name.rsplit_once('/').map(|tup| tup.1).unwrap_or(package_name) + package_name + .rsplit_once('/') + .map(|tup| tup.1) + .unwrap_or(package_name) } #[cfg(test)] @@ -208,12 +211,11 @@ mod tests { #[test] fn test_new() { assert_eq!( - Label::new(Some("repo"), Some("foo/bar"), "baz", true), + Label::new(Some("repo"), Some("foo/bar"), "baz"), Label { repository_name: Some("repo"), package_name: Some("foo/bar"), name: "baz", - is_absolute: true, } ); } @@ -228,12 +230,22 @@ mod tests { assert_eq!(analyze("@repo//foo/bar")?.repository_name, Some("repo")); assert_eq!(analyze("@//foo/bar")?.repository_name, None); assert_eq!(analyze("//foo/bar")?.repository_name, None); - assert_eq!(analyze("foo/bar")?.repository_name, None); + assert_eq!( + analyze("foo/bar"), + Err(LabelError( + "foo/bar must be a legal label; relative packages are not permitted.".to_string() + )) + ); assert_eq!(analyze("@repo//foo")?.repository_name, Some("repo")); assert_eq!(analyze("@//foo")?.repository_name, None); assert_eq!(analyze("//foo")?.repository_name, None); - assert_eq!(analyze("foo")?.repository_name, None); + assert_eq!( + analyze("foo"), + Err(LabelError( + "foo must be a legal label; relative packages are not permitted.".to_string() + )) + ); assert_eq!( analyze("@foo:bar"), @@ -272,10 +284,21 @@ mod tests { assert_eq!(analyze("//foo:baz/qux")?.package_name, Some("foo")); assert_eq!(analyze("//foo/bar:baz/qux")?.package_name, Some("foo/bar")); - assert_eq!(analyze("foo:baz/qux")?.package_name, Some("foo")); - assert_eq!(analyze("foo/bar:baz/qux")?.package_name, Some("foo/bar")); + assert_eq!( + analyze("foo:baz/qux"), + Err(LabelError( + "foo:baz/qux must be a legal label; relative packages are not permitted." + .to_string() + )) + ); + assert_eq!( + analyze("foo/bar:baz/qux"), + Err(LabelError( + "foo/bar:baz/qux must be a legal label; relative packages are not permitted." + .to_string() + )) + ); - assert_eq!(analyze("foo")?.package_name, None); assert_eq!(analyze("//foo")?.package_name, Some("foo")); assert_eq!( @@ -285,6 +308,13 @@ mod tests { .to_string() )) ); + assert_eq!( + analyze("//foo//bar"), + Err(LabelError( + "//foo//bar must be a legal label; '//' cannot appear in the middle of the label." + .to_string() + )) + ); assert_eq!( analyze("foo//bar:baz"), Err(LabelError( @@ -292,6 +322,13 @@ mod tests { .to_string() )) ); + assert_eq!( + analyze("//foo//bar:baz"), + Err(LabelError( + "//foo//bar:baz must be a legal label; '//' cannot appear in the middle of the label." + .to_string() + )) + ); assert_eq!( analyze("//azAZ09/-. $()_:baz")?.package_name, @@ -314,11 +351,21 @@ mod tests { assert_eq!(analyze("@repo//foo/bar")?.package_name, Some("foo/bar")); assert_eq!(analyze("//foo/bar")?.package_name, Some("foo/bar")); - assert_eq!(analyze("foo/bar")?.package_name, None); + assert_eq!( + analyze("foo/bar"), + Err(LabelError( + "foo/bar must be a legal label; relative packages are not permitted.".to_string() + )) + ); assert_eq!(analyze("@repo//foo")?.package_name, Some("foo")); assert_eq!(analyze("//foo")?.package_name, Some("foo")); - assert_eq!(analyze("foo")?.package_name, None); + assert_eq!( + analyze("foo"), + Err(LabelError( + "foo must be a legal label; relative packages are not permitted.".to_string() + )) + ); Ok(()) } @@ -346,11 +393,21 @@ mod tests { assert_eq!(analyze("@repo//foo/bar")?.name, "bar"); assert_eq!(analyze("//foo/bar")?.name, "bar"); - assert_eq!(analyze("foo/bar")?.name, "bar"); + assert_eq!( + analyze("foo/bar"), + Err(LabelError( + "foo/bar must be a legal label; relative packages are not permitted.".to_string() + )) + ); assert_eq!(analyze("@repo//foo")?.name, "foo"); assert_eq!(analyze("//foo")?.name, "foo"); - assert_eq!(analyze("foo")?.name, "foo"); + assert_eq!( + analyze("foo"), + Err(LabelError( + "foo must be a legal label; relative packages are not permitted.".to_string() + )) + ); Ok(()) } @@ -366,21 +423,4 @@ mod tests { Ok(()) } - - #[test] - fn test_is_absolute() -> Result<()> { - assert!(analyze("@repo//foo/bar:baz")?.is_absolute()); - assert!(analyze("//foo/bar:baz")?.is_absolute()); - assert!(!analyze("foo/bar:baz")?.is_absolute()); - - assert!(analyze("@repo//foo/bar")?.is_absolute()); - assert!(analyze("//foo/bar")?.is_absolute()); - assert!(!analyze("foo/bar")?.is_absolute()); - - assert!(analyze("@repo//foo")?.is_absolute()); - assert!(analyze("//foo")?.is_absolute()); - assert!(!analyze("foo")?.is_absolute()); - - Ok(()) - } } From ec9ff543888716de4917cf4fe457d1694a0e8998 Mon Sep 17 00:00:00 2001 From: Chris Fredrickson Date: Wed, 1 Dec 2021 10:49:43 -0500 Subject: [PATCH 4/4] s/target/name/ --- util/label/label.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/label/label.rs b/util/label/label.rs index 2b73a6a8af..22d2bb4302 100644 --- a/util/label/label.rs +++ b/util/label/label.rs @@ -17,7 +17,7 @@ pub fn analyze(input: &'_ str) -> Result> { (None, None) => { return Err(LabelError(err( label, - "labels must have a package and/or a target.", + "labels must have a package and/or a name.", ))) } (Some(package_name), None) => name_from_package(package_name),