Skip to content

Commit 55a569d

Browse files
charliermarshcharliermarsh
committed
Report marker diagnostics during parsing, rather than evaluation
1 parent 82f9690 commit 55a569d

File tree

4 files changed

+80
-151
lines changed

4 files changed

+80
-151
lines changed

‎crates/uv-pep508/src/lib.rs

-11
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,6 @@ impl<T: Pep508Url> Serialize for Requirement<T> {
200200
}
201201
}
202202

203-
type MarkerWarning = (MarkerWarningKind, String);
204-
205203
impl<T: Pep508Url> Requirement<T> {
206204
/// Returns whether the markers apply for the given environment
207205
pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
@@ -223,15 +221,6 @@ impl<T: Pep508Url> Requirement<T> {
223221
.evaluate_extras_and_python_version(extras, python_versions)
224222
}
225223

226-
/// Returns whether the markers apply for the given environment.
227-
pub fn evaluate_markers_and_report(
228-
&self,
229-
env: &MarkerEnvironment,
230-
extras: &[ExtraName],
231-
) -> (bool, Vec<MarkerWarning>) {
232-
self.marker.evaluate_collect_warnings(env, extras)
233-
}
234-
235224
/// Return the requirement with an additional marker added, to require the given extra.
236225
///
237226
/// For example, given `flask >= 2.0.2`, calling `with_extra_marker("dotenv")` would return

‎crates/uv-pep508/src/marker/parse.rs

+59-12
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ use uv_pep440::{Version, VersionPattern, VersionSpecifier};
66
use crate::cursor::Cursor;
77
use crate::marker::MarkerValueExtra;
88
use crate::{
9-
ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueVersion,
10-
MarkerWarningKind, Pep508Error, Pep508ErrorSource, Pep508Url, Reporter,
9+
ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueString,
10+
MarkerValueVersion, MarkerWarningKind, Pep508Error, Pep508ErrorSource, Pep508Url, Reporter,
1111
};
1212

1313
/// ```text
@@ -72,6 +72,7 @@ fn parse_marker_operator<T: Pep508Url>(
7272
/// '`implementation_version`', 'extra'
7373
pub(crate) fn parse_marker_value<T: Pep508Url>(
7474
cursor: &mut Cursor,
75+
reporter: &mut impl Reporter,
7576
) -> Result<MarkerValue, Pep508Error<T>> {
7677
// > User supplied constants are always encoded as strings with either ' or " quote marks. Note
7778
// > that backslash escapes are not defined, but existing implementations do support them. They
@@ -101,14 +102,60 @@ pub(crate) fn parse_marker_value<T: Pep508Url>(
101102
!char.is_whitespace() && !['>', '=', '<', '!', '~', ')'].contains(&char)
102103
});
103104
let key = cursor.slice(start, len);
104-
MarkerValue::from_str(key).map_err(|_| Pep508Error {
105-
message: Pep508ErrorSource::String(format!(
106-
"Expected a quoted string or a valid marker name, found `{key}`"
107-
)),
108-
start,
109-
len,
110-
input: cursor.to_string(),
111-
})
105+
MarkerValue::from_str(key)
106+
.map_err(|_| Pep508Error {
107+
message: Pep508ErrorSource::String(format!(
108+
"Expected a quoted string or a valid marker name, found `{key}`"
109+
)),
110+
start,
111+
len,
112+
input: cursor.to_string(),
113+
})
114+
.inspect(|value| match value {
115+
MarkerValue::MarkerEnvString(MarkerValueString::OsNameDeprecated) => {
116+
reporter.report(
117+
MarkerWarningKind::DeprecatedMarkerName,
118+
"os.name is deprecated in favor of os_name".to_string(),
119+
);
120+
}
121+
MarkerValue::MarkerEnvString(MarkerValueString::PlatformMachineDeprecated) => {
122+
reporter.report(
123+
MarkerWarningKind::DeprecatedMarkerName,
124+
"platform.machine is deprecated in favor of platform_machine".to_string(),
125+
);
126+
}
127+
MarkerValue::MarkerEnvString(
128+
MarkerValueString::PlatformPythonImplementationDeprecated,
129+
) => {
130+
reporter.report(
131+
MarkerWarningKind::DeprecatedMarkerName,
132+
"platform.python_implementation is deprecated in favor of platform_python_implementation".to_string(),
133+
);
134+
}
135+
MarkerValue::MarkerEnvString(
136+
MarkerValueString::PythonImplementationDeprecated,
137+
) => {
138+
reporter.report(
139+
MarkerWarningKind::DeprecatedMarkerName,
140+
"python_implementation is deprecated in favor of platform_python_implementation"
141+
.to_string(),
142+
);
143+
}
144+
MarkerValue::MarkerEnvString(MarkerValueString::PlatformVersionDeprecated) => {
145+
reporter.report(
146+
MarkerWarningKind::DeprecatedMarkerName,
147+
"platform.version is deprecated in favor of platform_version"
148+
.to_string(),
149+
);
150+
}
151+
MarkerValue::MarkerEnvString(MarkerValueString::SysPlatformDeprecated) => {
152+
reporter.report(
153+
MarkerWarningKind::DeprecatedMarkerName,
154+
"sys.platform is deprecated in favor of sys_platform".to_string(),
155+
);
156+
}
157+
_ => {}
158+
})
112159
}
113160
}
114161
}
@@ -121,14 +168,14 @@ pub(crate) fn parse_marker_key_op_value<T: Pep508Url>(
121168
reporter: &mut impl Reporter,
122169
) -> Result<Option<MarkerExpression>, Pep508Error<T>> {
123170
cursor.eat_whitespace();
124-
let l_value = parse_marker_value(cursor)?;
171+
let l_value = parse_marker_value(cursor, reporter)?;
125172
cursor.eat_whitespace();
126173
// "not in" and "in" must be preceded by whitespace. We must already have matched a whitespace
127174
// when we're here because other `parse_marker_key` would have pulled the characters in and
128175
// errored
129176
let operator = parse_marker_operator(cursor)?;
130177
cursor.eat_whitespace();
131-
let r_value = parse_marker_value(cursor)?;
178+
let r_value = parse_marker_value(cursor, reporter)?;
132179

133180
// Convert a `<marker_value> <marker_op> <marker_value>` expression into its
134181
// typed equivalent.

‎crates/uv-pep508/src/marker/tree.rs

+18-123
Original file line numberDiff line numberDiff line change
@@ -810,7 +810,6 @@ impl MarkerTree {
810810

811811
/// Does this marker apply in the given environment?
812812
pub fn evaluate(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
813-
self.report_deprecated_options(&mut TracingReporter);
814813
self.evaluate_reporter_impl(env, extras, &mut TracingReporter)
815814
}
816815

@@ -826,7 +825,6 @@ impl MarkerTree {
826825
env: Option<&MarkerEnvironment>,
827826
extras: &[ExtraName],
828827
) -> bool {
829-
self.report_deprecated_options(&mut TracingReporter);
830828
match env {
831829
None => self.evaluate_extras(extras),
832830
Some(env) => self.evaluate_reporter_impl(env, extras, &mut TracingReporter),
@@ -841,7 +839,6 @@ impl MarkerTree {
841839
extras: &[ExtraName],
842840
reporter: &mut impl Reporter,
843841
) -> bool {
844-
self.report_deprecated_options(reporter);
845842
self.evaluate_reporter_impl(env, extras, reporter)
846843
}
847844

@@ -993,102 +990,6 @@ impl MarkerTree {
993990
}
994991
}
995992

996-
/// Same as [`Self::evaluate`], but instead of using logging to warn, you get a Vec with all
997-
/// warnings collected
998-
pub fn evaluate_collect_warnings(
999-
&self,
1000-
env: &MarkerEnvironment,
1001-
extras: &[ExtraName],
1002-
) -> (bool, Vec<(MarkerWarningKind, String)>) {
1003-
let mut warnings = Vec::new();
1004-
let mut reporter = |kind, warning| {
1005-
warnings.push((kind, warning));
1006-
};
1007-
self.report_deprecated_options(&mut reporter);
1008-
let result = self.evaluate_reporter_impl(env, extras, &mut reporter);
1009-
(result, warnings)
1010-
}
1011-
1012-
/// Report the deprecated marker from <https://peps.python.org/pep-0345/#environment-markers>
1013-
fn report_deprecated_options(&self, reporter: &mut impl Reporter) {
1014-
let string_marker = match self.kind() {
1015-
MarkerTreeKind::True | MarkerTreeKind::False => return,
1016-
MarkerTreeKind::String(marker) => marker,
1017-
MarkerTreeKind::Version(marker) => {
1018-
for (_, tree) in marker.edges() {
1019-
tree.report_deprecated_options(reporter);
1020-
}
1021-
return;
1022-
}
1023-
MarkerTreeKind::In(marker) => {
1024-
for (_, tree) in marker.children() {
1025-
tree.report_deprecated_options(reporter);
1026-
}
1027-
return;
1028-
}
1029-
MarkerTreeKind::Contains(marker) => {
1030-
for (_, tree) in marker.children() {
1031-
tree.report_deprecated_options(reporter);
1032-
}
1033-
return;
1034-
}
1035-
MarkerTreeKind::Extra(marker) => {
1036-
for (_, tree) in marker.children() {
1037-
tree.report_deprecated_options(reporter);
1038-
}
1039-
return;
1040-
}
1041-
};
1042-
1043-
match string_marker.key() {
1044-
MarkerValueString::OsNameDeprecated => {
1045-
reporter.report(
1046-
MarkerWarningKind::DeprecatedMarkerName,
1047-
"os.name is deprecated in favor of os_name".to_string(),
1048-
);
1049-
}
1050-
MarkerValueString::PlatformMachineDeprecated => {
1051-
reporter.report(
1052-
MarkerWarningKind::DeprecatedMarkerName,
1053-
"platform.machine is deprecated in favor of platform_machine".to_string(),
1054-
);
1055-
}
1056-
MarkerValueString::PlatformPythonImplementationDeprecated => {
1057-
reporter.report(
1058-
MarkerWarningKind::DeprecatedMarkerName,
1059-
"platform.python_implementation is deprecated in favor of
1060-
platform_python_implementation"
1061-
.to_string(),
1062-
);
1063-
}
1064-
MarkerValueString::PythonImplementationDeprecated => {
1065-
reporter.report(
1066-
MarkerWarningKind::DeprecatedMarkerName,
1067-
"python_implementation is deprecated in favor of
1068-
platform_python_implementation"
1069-
.to_string(),
1070-
);
1071-
}
1072-
MarkerValueString::PlatformVersionDeprecated => {
1073-
reporter.report(
1074-
MarkerWarningKind::DeprecatedMarkerName,
1075-
"platform.version is deprecated in favor of platform_version".to_string(),
1076-
);
1077-
}
1078-
MarkerValueString::SysPlatformDeprecated => {
1079-
reporter.report(
1080-
MarkerWarningKind::DeprecatedMarkerName,
1081-
"sys.platform is deprecated in favor of sys_platform".to_string(),
1082-
);
1083-
}
1084-
_ => {}
1085-
}
1086-
1087-
for (_, tree) in string_marker.children() {
1088-
tree.report_deprecated_options(reporter);
1089-
}
1090-
}
1091-
1092993
/// Find a top level `extra == "..."` expression.
1093994
///
1094995
/// ASSUMPTION: There is one `extra = "..."`, and it's either the only marker or part of the
@@ -2010,14 +1911,15 @@ mod test {
20101911
let expected = [
20111912
"WARN warnings4: uv_pep508: os.name is deprecated in favor of os_name",
20121913
"WARN warnings4: uv_pep508: platform.machine is deprecated in favor of platform_machine",
2013-
"WARN warnings4: uv_pep508: platform.python_implementation is deprecated in favor of",
2014-
"WARN warnings4: uv_pep508: sys.platform is deprecated in favor of sys_platform",
1914+
"WARN warnings4: uv_pep508: platform.python_implementation is deprecated in favor of platform_python_implementation",
1915+
"WARN warnings4: uv_pep508: platform.version is deprecated in favor of platform_version",
1916+
"WARN warnings4: uv_pep508: sys.platform is deprecated in favor of sys_platform",
20151917
"WARN warnings4: uv_pep508: Comparing linux and posix lexicographically"
20161918
];
20171919
if lines == expected {
20181920
Ok(())
20191921
} else {
2020-
Err(format!("{lines:?}"))
1922+
Err(format!("{lines:#?}"))
20211923
}
20221924
});
20231925
}
@@ -2030,59 +1932,52 @@ mod test {
20301932
#[test]
20311933
fn test_marker_version_inverted() {
20321934
let env37 = env37();
2033-
let (result, warnings) = MarkerTree::from_str("python_version > '3.6'")
1935+
let result = MarkerTree::from_str("python_version > '3.6'")
20341936
.unwrap()
2035-
.evaluate_collect_warnings(&env37, &[]);
2036-
assert_eq!(warnings, &[]);
1937+
.evaluate(&env37, &[]);
20371938
assert!(result);
20381939

2039-
let (result, warnings) = MarkerTree::from_str("'3.6' > python_version")
1940+
let result = MarkerTree::from_str("'3.6' > python_version")
20401941
.unwrap()
2041-
.evaluate_collect_warnings(&env37, &[]);
2042-
assert_eq!(warnings, &[]);
1942+
.evaluate(&env37, &[]);
20431943
assert!(!result);
20441944

20451945
// Meaningless expressions are ignored, so this is always true.
2046-
let (result, warnings) = MarkerTree::from_str("'3.*' == python_version")
1946+
let result = MarkerTree::from_str("'3.*' == python_version")
20471947
.unwrap()
2048-
.evaluate_collect_warnings(&env37, &[]);
2049-
assert_eq!(warnings, &[]);
1948+
.evaluate(&env37, &[]);
20501949
assert!(result);
20511950
}
20521951

20531952
#[test]
20541953
fn test_marker_string_inverted() {
20551954
let env37 = env37();
2056-
let (result, warnings) = MarkerTree::from_str("'nux' in sys_platform")
1955+
let result = MarkerTree::from_str("'nux' in sys_platform")
20571956
.unwrap()
2058-
.evaluate_collect_warnings(&env37, &[]);
2059-
assert_eq!(warnings, &[]);
1957+
.evaluate(&env37, &[]);
20601958
assert!(result);
20611959

2062-
let (result, warnings) = MarkerTree::from_str("sys_platform in 'nux'")
1960+
let result = MarkerTree::from_str("sys_platform in 'nux'")
20631961
.unwrap()
2064-
.evaluate_collect_warnings(&env37, &[]);
2065-
assert_eq!(warnings, &[]);
1962+
.evaluate(&env37, &[]);
20661963
assert!(!result);
20671964
}
20681965

20691966
#[test]
20701967
fn test_marker_version_star() {
20711968
let env37 = env37();
2072-
let (result, warnings) = MarkerTree::from_str("python_version == '3.7.*'")
1969+
let result = MarkerTree::from_str("python_version == '3.7.*'")
20731970
.unwrap()
2074-
.evaluate_collect_warnings(&env37, &[]);
2075-
assert_eq!(warnings, &[]);
1971+
.evaluate(&env37, &[]);
20761972
assert!(result);
20771973
}
20781974

20791975
#[test]
20801976
fn test_tilde_equal() {
20811977
let env37 = env37();
2082-
let (result, warnings) = MarkerTree::from_str("python_version ~= '3.7'")
1978+
let result = MarkerTree::from_str("python_version ~= '3.7'")
20831979
.unwrap()
2084-
.evaluate_collect_warnings(&env37, &[]);
2085-
assert_eq!(warnings, &[]);
1980+
.evaluate(&env37, &[]);
20861981
assert!(result);
20871982
}
20881983

‎crates/uv-pep508/src/verbatim_url.rs

+3-5
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ use std::sync::LazyLock;
99
use thiserror::Error;
1010
use url::{ParseError, Url};
1111

12-
use uv_fs::{normalize_absolute_path, normalize_url_path};
13-
1412
use crate::Pep508Url;
1513

1614
/// A wrapper around [`Url`] that preserves the original string.
@@ -62,7 +60,7 @@ impl VerbatimUrl {
6260
Cow::Owned(base_dir.as_ref().join(path))
6361
};
6462

65-
let path = normalize_absolute_path(&path)
63+
let path = uv_fs::normalize_absolute_path(&path)
6664
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;
6765

6866
// Extract the fragment, if it exists.
@@ -92,7 +90,7 @@ impl VerbatimUrl {
9290
};
9391

9492
// Normalize the path.
95-
let path = normalize_absolute_path(path)
93+
let path = uv_fs::normalize_absolute_path(path)
9694
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;
9795

9896
// Extract the fragment, if it exists.
@@ -229,7 +227,7 @@ impl Pep508Url for VerbatimUrl {
229227
{
230228
let path = strip_host(path);
231229

232-
let path = normalize_url_path(path);
230+
let path = uv_fs::normalize_url_path(path);
233231

234232
if let Some(working_dir) = working_dir {
235233
return Ok(VerbatimUrl::from_path(path.as_ref(), working_dir)?

0 commit comments

Comments
 (0)