Skip to content

Commit

Permalink
Report marker diagnostics during parsing, rather than evaluation
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Nov 23, 2024
1 parent 1343b16 commit 5eca3e9
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 151 deletions.
11 changes: 0 additions & 11 deletions crates/uv-pep508/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,6 @@ impl<T: Pep508Url> Serialize for Requirement<T> {
}
}

type MarkerWarning = (MarkerWarningKind, String);

impl<T: Pep508Url> Requirement<T> {
/// Returns whether the markers apply for the given environment
pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
Expand All @@ -223,15 +221,6 @@ impl<T: Pep508Url> Requirement<T> {
.evaluate_extras_and_python_version(extras, python_versions)
}

/// Returns whether the markers apply for the given environment.
pub fn evaluate_markers_and_report(
&self,
env: &MarkerEnvironment,
extras: &[ExtraName],
) -> (bool, Vec<MarkerWarning>) {
self.marker.evaluate_collect_warnings(env, extras)
}

/// Return the requirement with an additional marker added, to require the given extra.
///
/// For example, given `flask >= 2.0.2`, calling `with_extra_marker("dotenv")` would return
Expand Down
71 changes: 59 additions & 12 deletions crates/uv-pep508/src/marker/parse.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use uv_pep440::{Version, VersionPattern, VersionSpecifier};
use crate::cursor::Cursor;
use crate::marker::MarkerValueExtra;
use crate::{
ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueVersion,
MarkerWarningKind, Pep508Error, Pep508ErrorSource, Pep508Url, Reporter,
ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueString,
MarkerValueVersion, MarkerWarningKind, Pep508Error, Pep508ErrorSource, Pep508Url, Reporter,
};

/// ```text
Expand Down Expand Up @@ -72,6 +72,7 @@ fn parse_marker_operator<T: Pep508Url>(
/// '`implementation_version`', 'extra'
pub(crate) fn parse_marker_value<T: Pep508Url>(
cursor: &mut Cursor,
reporter: &mut impl Reporter,
) -> Result<MarkerValue, Pep508Error<T>> {
// > User supplied constants are always encoded as strings with either ' or " quote marks. Note
// > that backslash escapes are not defined, but existing implementations do support them. They
Expand Down Expand Up @@ -101,14 +102,60 @@ pub(crate) fn parse_marker_value<T: Pep508Url>(
!char.is_whitespace() && !['>', '=', '<', '!', '~', ')'].contains(&char)
});
let key = cursor.slice(start, len);
MarkerValue::from_str(key).map_err(|_| Pep508Error {
message: Pep508ErrorSource::String(format!(
"Expected a quoted string or a valid marker name, found `{key}`"
)),
start,
len,
input: cursor.to_string(),
})
MarkerValue::from_str(key)
.map_err(|_| Pep508Error {
message: Pep508ErrorSource::String(format!(
"Expected a quoted string or a valid marker name, found `{key}`"
)),
start,
len,
input: cursor.to_string(),
})
.inspect(|value| match value {
MarkerValue::MarkerEnvString(MarkerValueString::OsNameDeprecated) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"os.name is deprecated in favor of os_name".to_string(),
);
}
MarkerValue::MarkerEnvString(MarkerValueString::PlatformMachineDeprecated) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.machine is deprecated in favor of platform_machine".to_string(),
);
}
MarkerValue::MarkerEnvString(
MarkerValueString::PlatformPythonImplementationDeprecated,
) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.python_implementation is deprecated in favor of platform_python_implementation".to_string(),
);
}
MarkerValue::MarkerEnvString(
MarkerValueString::PythonImplementationDeprecated,
) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"python_implementation is deprecated in favor of platform_python_implementation"
.to_string(),
);
}
MarkerValue::MarkerEnvString(MarkerValueString::PlatformVersionDeprecated) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.version is deprecated in favor of platform_version"
.to_string(),
);
}
MarkerValue::MarkerEnvString(MarkerValueString::SysPlatformDeprecated) => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"sys.platform is deprecated in favor of sys_platform".to_string(),
);
}
_ => {}
})
}
}
}
Expand All @@ -121,14 +168,14 @@ pub(crate) fn parse_marker_key_op_value<T: Pep508Url>(
reporter: &mut impl Reporter,
) -> Result<Option<MarkerExpression>, Pep508Error<T>> {
cursor.eat_whitespace();
let l_value = parse_marker_value(cursor)?;
let l_value = parse_marker_value(cursor, reporter)?;
cursor.eat_whitespace();
// "not in" and "in" must be preceded by whitespace. We must already have matched a whitespace
// when we're here because other `parse_marker_key` would have pulled the characters in and
// errored
let operator = parse_marker_operator(cursor)?;
cursor.eat_whitespace();
let r_value = parse_marker_value(cursor)?;
let r_value = parse_marker_value(cursor, reporter)?;

// Convert a `<marker_value> <marker_op> <marker_value>` expression into its
// typed equivalent.
Expand Down
141 changes: 18 additions & 123 deletions crates/uv-pep508/src/marker/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,6 @@ impl MarkerTree {

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

Expand All @@ -826,7 +825,6 @@ impl MarkerTree {
env: Option<&MarkerEnvironment>,
extras: &[ExtraName],
) -> bool {
self.report_deprecated_options(&mut TracingReporter);
match env {
None => self.evaluate_extras(extras),
Some(env) => self.evaluate_reporter_impl(env, extras, &mut TracingReporter),
Expand All @@ -841,7 +839,6 @@ impl MarkerTree {
extras: &[ExtraName],
reporter: &mut impl Reporter,
) -> bool {
self.report_deprecated_options(reporter);
self.evaluate_reporter_impl(env, extras, reporter)
}

Expand Down Expand Up @@ -993,102 +990,6 @@ impl MarkerTree {
}
}

/// Same as [`Self::evaluate`], but instead of using logging to warn, you get a Vec with all
/// warnings collected
pub fn evaluate_collect_warnings(
&self,
env: &MarkerEnvironment,
extras: &[ExtraName],
) -> (bool, Vec<(MarkerWarningKind, String)>) {
let mut warnings = Vec::new();
let mut reporter = |kind, warning| {
warnings.push((kind, warning));
};
self.report_deprecated_options(&mut reporter);
let result = self.evaluate_reporter_impl(env, extras, &mut reporter);
(result, warnings)
}

/// Report the deprecated marker from <https://peps.python.org/pep-0345/#environment-markers>
fn report_deprecated_options(&self, reporter: &mut impl Reporter) {
let string_marker = match self.kind() {
MarkerTreeKind::True | MarkerTreeKind::False => return,
MarkerTreeKind::String(marker) => marker,
MarkerTreeKind::Version(marker) => {
for (_, tree) in marker.edges() {
tree.report_deprecated_options(reporter);
}
return;
}
MarkerTreeKind::In(marker) => {
for (_, tree) in marker.children() {
tree.report_deprecated_options(reporter);
}
return;
}
MarkerTreeKind::Contains(marker) => {
for (_, tree) in marker.children() {
tree.report_deprecated_options(reporter);
}
return;
}
MarkerTreeKind::Extra(marker) => {
for (_, tree) in marker.children() {
tree.report_deprecated_options(reporter);
}
return;
}
};

match string_marker.key() {
MarkerValueString::OsNameDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"os.name is deprecated in favor of os_name".to_string(),
);
}
MarkerValueString::PlatformMachineDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.machine is deprecated in favor of platform_machine".to_string(),
);
}
MarkerValueString::PlatformPythonImplementationDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.python_implementation is deprecated in favor of
platform_python_implementation"
.to_string(),
);
}
MarkerValueString::PythonImplementationDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"python_implementation is deprecated in favor of
platform_python_implementation"
.to_string(),
);
}
MarkerValueString::PlatformVersionDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"platform.version is deprecated in favor of platform_version".to_string(),
);
}
MarkerValueString::SysPlatformDeprecated => {
reporter.report(
MarkerWarningKind::DeprecatedMarkerName,
"sys.platform is deprecated in favor of sys_platform".to_string(),
);
}
_ => {}
}

for (_, tree) in string_marker.children() {
tree.report_deprecated_options(reporter);
}
}

/// Find a top level `extra == "..."` expression.
///
/// ASSUMPTION: There is one `extra = "..."`, and it's either the only marker or part of the
Expand Down Expand Up @@ -2010,14 +1911,15 @@ mod test {
let expected = [
"WARN warnings4: uv_pep508: os.name is deprecated in favor of os_name",
"WARN warnings4: uv_pep508: platform.machine is deprecated in favor of platform_machine",
"WARN warnings4: uv_pep508: platform.python_implementation is deprecated in favor of",
"WARN warnings4: uv_pep508: sys.platform is deprecated in favor of sys_platform",
"WARN warnings4: uv_pep508: platform.python_implementation is deprecated in favor of platform_python_implementation",
"WARN warnings4: uv_pep508: platform.version is deprecated in favor of platform_version",
"WARN warnings4: uv_pep508: sys.platform is deprecated in favor of sys_platform",
"WARN warnings4: uv_pep508: Comparing linux and posix lexicographically"
];
if lines == expected {
Ok(())
} else {
Err(format!("{lines:?}"))
Err(format!("{lines:#?}"))
}
});
}
Expand All @@ -2030,59 +1932,52 @@ mod test {
#[test]
fn test_marker_version_inverted() {
let env37 = env37();
let (result, warnings) = MarkerTree::from_str("python_version > '3.6'")
let result = MarkerTree::from_str("python_version > '3.6'")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(result);

let (result, warnings) = MarkerTree::from_str("'3.6' > python_version")
let result = MarkerTree::from_str("'3.6' > python_version")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(!result);

// Meaningless expressions are ignored, so this is always true.
let (result, warnings) = MarkerTree::from_str("'3.*' == python_version")
let result = MarkerTree::from_str("'3.*' == python_version")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(result);
}

#[test]
fn test_marker_string_inverted() {
let env37 = env37();
let (result, warnings) = MarkerTree::from_str("'nux' in sys_platform")
let result = MarkerTree::from_str("'nux' in sys_platform")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(result);

let (result, warnings) = MarkerTree::from_str("sys_platform in 'nux'")
let result = MarkerTree::from_str("sys_platform in 'nux'")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(!result);
}

#[test]
fn test_marker_version_star() {
let env37 = env37();
let (result, warnings) = MarkerTree::from_str("python_version == '3.7.*'")
let result = MarkerTree::from_str("python_version == '3.7.*'")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(result);
}

#[test]
fn test_tilde_equal() {
let env37 = env37();
let (result, warnings) = MarkerTree::from_str("python_version ~= '3.7'")
let result = MarkerTree::from_str("python_version ~= '3.7'")
.unwrap()
.evaluate_collect_warnings(&env37, &[]);
assert_eq!(warnings, &[]);
.evaluate(&env37, &[]);
assert!(result);
}

Expand Down
8 changes: 3 additions & 5 deletions crates/uv-pep508/src/verbatim_url.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ use std::sync::LazyLock;
use thiserror::Error;
use url::{ParseError, Url};

use uv_fs::{normalize_absolute_path, normalize_url_path};

use crate::Pep508Url;

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

let path = normalize_absolute_path(&path)
let path = uv_fs::normalize_absolute_path(&path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;

// Extract the fragment, if it exists.
Expand Down Expand Up @@ -92,7 +90,7 @@ impl VerbatimUrl {
};

// Normalize the path.
let path = normalize_absolute_path(path)
let path = uv_fs::normalize_absolute_path(path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;

// Extract the fragment, if it exists.
Expand Down Expand Up @@ -229,7 +227,7 @@ impl Pep508Url for VerbatimUrl {
{
let path = strip_host(path);

let path = normalize_url_path(path);
let path = uv_fs::normalize_url_path(path);

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

0 comments on commit 5eca3e9

Please sign in to comment.