Skip to content

Commit

Permalink
Show dedicated error for trailing ; on URL and path requirements (#8835)
Browse files Browse the repository at this point in the history
## Summary

I think we lost this error in a prior refactor. I've added an end-to-end
test too.
  • Loading branch information
charliermarsh authored Nov 5, 2024
1 parent fae03a7 commit eeb8736
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 36 deletions.
32 changes: 13 additions & 19 deletions crates/uv-pep508/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,19 @@ fn parse_url<T: Pep508Url>(
});
}

for c in [';', '#'] {
if url.ends_with(c) {
return Err(Pep508Error {
message: Pep508ErrorSource::String(format!(
"Missing space before '{c}', the end of the URL is ambiguous"
)),
start: start + len - 1,
len: 1,
input: cursor.to_string(),
});
}
}

let url = T::parse_url(url, working_dir).map_err(|err| Pep508Error {
message: Pep508ErrorSource::UrlError(err),
start,
Expand Down Expand Up @@ -927,8 +940,6 @@ fn parse_pep508_requirement<T: Pep508Url>(
}
};

let requirement_end = cursor.pos();

// If the requirement consists solely of a package name, and that name appears to be an archive,
// treat it as a URL requirement, for consistency and security. (E.g., `requests-2.26.0.tar.gz`
// is a valid Python package name, but we should treat it as a reference to a file.)
Expand Down Expand Up @@ -960,23 +971,6 @@ fn parse_pep508_requirement<T: Pep508Url>(
// wsp*
cursor.eat_whitespace();
if let Some((pos, char)) = cursor.next() {
if marker.is_none() {
if let Some(VersionOrUrl::Url(url)) = requirement_kind {
let url = url.to_string();
for c in [';', '#'] {
if url.ends_with(c) {
return Err(Pep508Error {
message: Pep508ErrorSource::String(format!(
"Missing space before '{c}', the end of the URL is ambiguous"
)),
start: requirement_end - c.len_utf8(),
len: c.len_utf8(),
input: cursor.to_string(),
});
}
}
}
}
let message = if marker.is_none() {
format!(r#"Expected end of input or `;`, found `{char}`"#)
} else {
Expand Down
24 changes: 24 additions & 0 deletions crates/uv-pep508/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,30 @@ fn error_no_space_after_url() {
);
}

#[test]
fn error_no_space_after_file_url() {
assert_snapshot!(
parse_pep508_err(r"name @ file:///test.whl; extra == 'example'"),
@r###"
Missing space before ';', the end of the URL is ambiguous
name @ file:///test.whl; extra == 'example'
^
"###
);
}

#[test]
fn error_no_space_after_file_path() {
assert_snapshot!(
parse_pep508_err(r"name @ ./test.whl; extra == 'example'"),
@r###"
Missing space before ';', the end of the URL is ambiguous
name @ ./test.whl; extra == 'example'
^
"###
);
}

#[test]
fn error_name_at_nothing() {
assert_snapshot!(
Expand Down
30 changes: 13 additions & 17 deletions crates/uv-pep508/src/unnamed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ fn parse_unnamed_requirement<Url: UnnamedRequirementUrl>(

// Parse the URL itself, along with any extras.
let (url, extras) = parse_unnamed_url::<Url>(cursor, working_dir)?;
let requirement_end = cursor.pos();

// wsp*
cursor.eat_whitespace();
Expand All @@ -175,22 +174,6 @@ fn parse_unnamed_requirement<Url: UnnamedRequirementUrl>(
// wsp*
cursor.eat_whitespace();
if let Some((pos, char)) = cursor.next() {
if marker.is_none() {
if let Some(given) = url.given() {
for c in [';', '#'] {
if given.ends_with(c) {
return Err(Pep508Error {
message: Pep508ErrorSource::String(format!(
"Missing space before '{c}', the end of the URL is ambiguous"
)),
start: requirement_end - c.len_utf8(),
len: c.len_utf8(),
input: cursor.to_string(),
});
}
}
}
}
let message = if marker.is_none() {
format!(r#"Expected end of input or `;`, found `{char}`"#)
} else {
Expand Down Expand Up @@ -430,6 +413,19 @@ fn parse_unnamed_url<Url: UnnamedRequirementUrl>(
});
}

for c in [';', '#'] {
if url.ends_with(c) {
return Err(Pep508Error {
message: Pep508ErrorSource::String(format!(
"Missing space before '{c}', the end of the URL is ambiguous"
)),
start: start + len - 1,
len: 1,
input: cursor.to_string(),
});
}
}

let url = preprocess_unnamed_url(url, working_dir, cursor, start, len)?;

Ok(url)
Expand Down
25 changes: 25 additions & 0 deletions crates/uv/tests/it/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,31 @@ dependencies = ["flask==1.0.x"]
Ok(())
}

#[test]
fn trailing_semicolon() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.write_str("./flask.whl; sys_platform == 'win32'")?;

uv_snapshot!(context.pip_install()
.arg("-r")
.arg("requirements.txt")
.arg("--strict"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Couldn't parse requirement in `requirements.txt` at position 0
Caused by: Missing space before ';', the end of the URL is ambiguous
./flask.whl; sys_platform == 'win32'
^
"###
);

Ok(())
}

#[test]
fn missing_pip() {
uv_snapshot!(Command::new(get_bin()).arg("install"), @r###"
Expand Down

0 comments on commit eeb8736

Please sign in to comment.