Skip to content

Commit

Permalink
Use RangedValue in Options
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Jan 23, 2025
1 parent fe6f470 commit 997bd03
Show file tree
Hide file tree
Showing 10 changed files with 123 additions and 53 deletions.
6 changes: 4 additions & 2 deletions crates/red_knot/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use colored::Colorize;
use crossbeam::channel as crossbeam_channel;
use python_version::PythonVersion;
use red_knot_project::metadata::options::{EnvironmentOptions, Options};
use red_knot_project::metadata::value::RelativePathBuf;
use red_knot_project::metadata::value::{RangedValue, RelativePathBuf};
use red_knot_project::watch;
use red_knot_project::watch::ProjectWatcher;
use red_knot_project::{ProjectDatabase, ProjectMetadata};
Expand Down Expand Up @@ -73,7 +73,9 @@ impl Args {
fn to_options(&self) -> Options {
Options {
environment: Some(EnvironmentOptions {
python_version: self.python_version.map(Into::into),
python_version: self
.python_version
.map(|version| RangedValue::cli(version.into())),
venv_path: self.venv_path.as_ref().map(RelativePathBuf::cli),
typeshed: self.typeshed.as_ref().map(RelativePathBuf::cli),
extra_paths: self.extra_search_path.as_ref().map(|extra_search_paths| {
Expand Down
2 changes: 1 addition & 1 deletion crates/red_knot/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ division-by-zer = "warn" # incorrect rule name
success: false
exit_code: 1
----- stdout -----
warning[unknown-rule] Unknown lint rule `division-by-zer`
warning[unknown-rule] <temp_dir>/pyproject.toml:3:1 Unknown lint rule `division-by-zer`
----- stderr -----
");
Expand Down
16 changes: 10 additions & 6 deletions crates/red_knot/tests/file_watching.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::time::{Duration, Instant};
use anyhow::{anyhow, Context};
use red_knot_project::metadata::options::{EnvironmentOptions, Options};
use red_knot_project::metadata::pyproject::{PyProject, Tool};
use red_knot_project::metadata::value::RelativePathBuf;
use red_knot_project::metadata::value::{RangedValue, RelativePathBuf};
use red_knot_project::watch::{directory_watcher, ChangeEvent, ProjectWatcher};
use red_knot_project::{Db, ProjectDatabase, ProjectMetadata};
use red_knot_python_semantic::{resolve_module, ModuleName, PythonPlatform, PythonVersion};
Expand Down Expand Up @@ -897,8 +897,10 @@ print(sys.last_exc, os.getegid())
|_root_path, _project_path| {
Some(Options {
environment: Some(EnvironmentOptions {
python_version: Some(PythonVersion::PY311),
python_platform: Some(PythonPlatform::Identifier("win32".to_string())),
python_version: Some(RangedValue::cli(PythonVersion::PY311)),
python_platform: Some(RangedValue::cli(PythonPlatform::Identifier(
"win32".to_string(),
))),
..EnvironmentOptions::default()
}),
..Options::default()
Expand All @@ -921,8 +923,10 @@ print(sys.last_exc, os.getegid())
// Change the python version
case.update_options(Options {
environment: Some(EnvironmentOptions {
python_version: Some(PythonVersion::PY312),
python_platform: Some(PythonPlatform::Identifier("linux".to_string())),
python_version: Some(RangedValue::cli(PythonVersion::PY312)),
python_platform: Some(RangedValue::cli(PythonPlatform::Identifier(
"linux".to_string(),
))),
..EnvironmentOptions::default()
}),
..Options::default()
Expand Down Expand Up @@ -1382,7 +1386,7 @@ mod unix {
extra_paths: Some(vec![RelativePathBuf::cli(
".venv/lib/python3.12/site-packages",
)]),
python_version: Some(PythonVersion::PY312),
python_version: Some(RangedValue::cli(PythonVersion::PY312)),
..EnvironmentOptions::default()
}),
..Options::default()
Expand Down
2 changes: 1 addition & 1 deletion crates/red_knot_project/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl ProjectMetadata {
) -> Self {
let name = project
.and_then(|project| project.name.as_ref())
.map(|name| Name::new(&**name))
.map(|name| Name::new(&***name))
.unwrap_or_else(|| Name::new(root.file_name().unwrap_or("root")));

// TODO(https://github.com/astral-sh/ruff/issues/15491): Respect requires-python
Expand Down
84 changes: 60 additions & 24 deletions crates/red_knot_project/src/metadata/options.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use crate::metadata::value::{RelativePathBuf, ValueSource, ValueSourceGuard};
use crate::metadata::value::{RangedValue, RelativePathBuf, ValueSource, ValueSourceGuard};
use crate::Db;
use red_knot_python_semantic::lint::{GetLintError, Level, RuleSelection};
use red_knot_python_semantic::lint::{GetLintError, Level, LintSource, RuleSelection};
use red_knot_python_semantic::{
ProgramSettings, PythonPlatform, PythonVersion, SearchPathSettings, SitePackages,
};
use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity};
use ruff_db::files::File;
use ruff_db::files::{system_path_to_file, File};
use ruff_db::system::{System, SystemPath};
use ruff_macros::Combine;
use ruff_text_size::TextRange;
Expand Down Expand Up @@ -44,7 +44,12 @@ impl Options {
let (python_version, python_platform) = self
.environment
.as_ref()
.map(|env| (env.python_version, env.python_platform.as_ref()))
.map(|env| {
(
env.python_version.as_deref().copied(),
env.python_platform.as_deref(),
)
})
.unwrap_or_default();

ProgramSettings {
Expand Down Expand Up @@ -116,27 +121,42 @@ impl Options {
.flat_map(|rules| rules.inner.iter());

for (rule_name, level) in rules {
let source = rule_name.source();
match registry.get(rule_name) {
Ok(lint) => {
if let Ok(severity) = Severity::try_from(*level) {
selection.enable(lint, severity);
let lint_source = match source {
ValueSource::File(_) => LintSource::File,
ValueSource::Cli => LintSource::Cli,
};
if let Ok(severity) = Severity::try_from(**level) {
selection.enable(lint, severity, lint_source);
} else {
selection.disable(lint);
}
}
Err(GetLintError::Unknown(_)) => {
diagnostics.push(OptionDiagnostic::new(
DiagnosticId::UnknownRule,
format!("Unknown lint rule `{rule_name}`"),
Severity::Warning,
));
}
Err(GetLintError::Removed(_)) => {
diagnostics.push(OptionDiagnostic::new(
DiagnosticId::UnknownRule,
format!("The lint rule `{rule_name}` has been removed and is no longer supported"),
Severity::Warning,
));
Err(error) => {
// `system_path_to_file` can return `Err` if the file was deleted since the configuration
// was read. This should be rare and it should be okay to default to not showing a configuration
// file in that case.
let file = source
.file()
.and_then(|path| system_path_to_file(db.upcast(), path).ok());

// TODO: Add a note if the value was configured on the CLI
let diagnostic = match error {
GetLintError::Unknown(_) => OptionDiagnostic::new(
DiagnosticId::UnknownRule,
format!("Unknown lint rule `{rule_name}`"),
Severity::Warning,
),
GetLintError::Removed(_) => OptionDiagnostic::new(
DiagnosticId::UnknownRule,
format!("Unknown lint rule `{rule_name}`"),
Severity::Warning,
),
};

diagnostics.push(diagnostic.with_file(file).with_range(rule_name.range()));
}
}
}
Expand All @@ -149,10 +169,10 @@ impl Options {
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
pub struct EnvironmentOptions {
#[serde(skip_serializing_if = "Option::is_none")]
pub python_version: Option<PythonVersion>,
pub python_version: Option<RangedValue<PythonVersion>>,

#[serde(skip_serializing_if = "Option::is_none")]
pub python_platform: Option<PythonPlatform>,
pub python_platform: Option<RangedValue<PythonPlatform>>,

/// List of user-provided paths that should take first priority in the module resolution.
/// Examples in other type checkers are mypy's MYPYPATH environment variable,
Expand Down Expand Up @@ -183,7 +203,7 @@ pub struct SrcOptions {
#[derive(Debug, Default, Clone, Eq, PartialEq, Combine, Serialize, Deserialize)]
#[serde(rename_all = "kebab-case", transparent)]
pub struct Rules {
inner: FxHashMap<String, Level>,
inner: FxHashMap<RangedValue<String>, RangedValue<Level>>,
}

#[derive(Error, Debug)]
Expand All @@ -197,6 +217,8 @@ pub struct OptionDiagnostic {
id: DiagnosticId,
message: String,
severity: Severity,
file: Option<File>,
range: Option<TextRange>,
}

impl OptionDiagnostic {
Expand All @@ -205,8 +227,22 @@ impl OptionDiagnostic {
id,
message,
severity,
file: None,
range: None,
}
}

#[must_use]
fn with_file(mut self, file: Option<File>) -> Self {
self.file = file;
self
}

#[must_use]
fn with_range(mut self, range: Option<TextRange>) -> Self {
self.range = range;
self
}
}

impl Diagnostic for OptionDiagnostic {
Expand All @@ -219,11 +255,11 @@ impl Diagnostic for OptionDiagnostic {
}

fn file(&self) -> Option<File> {
None
self.file
}

fn range(&self) -> Option<TextRange> {
None
self.range
}

fn severity(&self) -> Severity {
Expand Down
8 changes: 4 additions & 4 deletions crates/red_knot_project/src/metadata/pyproject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::ops::Deref;
use thiserror::Error;

use crate::metadata::options::Options;
use crate::metadata::value::{ValueSource, ValueSourceGuard};
use crate::metadata::value::{RangedValue, ValueSource, ValueSourceGuard};

/// A `pyproject.toml` as specified in PEP 517.
#[derive(Deserialize, Serialize, Debug, Default, Clone)]
Expand Down Expand Up @@ -48,11 +48,11 @@ pub struct Project {
///
/// Note: Intentionally option to be more permissive during deserialization.
/// `PackageMetadata::from_pyproject` reports missing names.
pub name: Option<PackageName>,
pub name: Option<RangedValue<PackageName>>,
/// The version of the project
pub version: Option<Version>,
pub version: Option<RangedValue<Version>>,
/// The Python versions this project is compatible with.
pub requires_python: Option<VersionSpecifiers>,
pub requires_python: Option<RangedValue<VersionSpecifiers>>,
}

#[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)]
Expand Down
20 changes: 19 additions & 1 deletion crates/red_knot_project/src/metadata/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ pub enum ValueSource {
Cli,
}

impl ValueSource {
pub fn file(&self) -> Option<&SystemPath> {
match self {
ValueSource::File(path) => Some(&**path),
ValueSource::Cli => None,
}
}
}

thread_local! {
/// Serde doesn't provide any easy means to pass a value to a [`Deserialize`] implementation,
/// but we want to associate each deserialized [`RelativePath`] with the source from
Expand Down Expand Up @@ -53,7 +62,7 @@ impl Drop for ValueSourceGuard {
}
}

/// A configuration value that tracks where it originates from and the range in the source document.
/// A value that "remembers" where it comes from (source) and its range in source.
///
/// ## Equality, Hash, and Ordering
/// The equality, hash, and ordering are solely based on the value. They disregard the value's range
Expand All @@ -65,6 +74,11 @@ impl Drop for ValueSourceGuard {
pub struct RangedValue<T> {
value: T,
source: ValueSource,

/// The byte range of `value` in `source`.
///
/// Can be `None` because not all sources support a range.
/// For example, arguments provided on the CLI won't have a range attached.
range: Option<TextRange>,
}

Expand All @@ -89,6 +103,10 @@ impl<T> RangedValue<T> {
self.range
}

pub fn source(&self) -> &ValueSource {
&self.source
}

#[must_use]
pub fn with_source(mut self, source: ValueSource) -> Self {
self.source = source;
Expand Down
32 changes: 20 additions & 12 deletions crates/red_knot_python_semantic/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ pub struct RuleSelection {
/// Map with the severity for each enabled lint rule.
///
/// If a rule isn't present in this map, then it should be considered disabled.
lints: FxHashMap<LintId, Severity>,
lints: FxHashMap<LintId, (Severity, LintSource)>,
}

impl RuleSelection {
Expand All @@ -427,7 +427,7 @@ impl RuleSelection {
.filter_map(|lint| {
Severity::try_from(lint.default_level())
.ok()
.map(|severity| (*lint, severity))
.map(|severity| (*lint, (severity, LintSource::Default)))
})
.collect();

Expand All @@ -441,12 +441,14 @@ impl RuleSelection {

/// Returns an iterator over all enabled lints and their severity.
pub fn iter(&self) -> impl ExactSizeIterator<Item = (LintId, Severity)> + '_ {
self.lints.iter().map(|(&lint, &severity)| (lint, severity))
self.lints
.iter()
.map(|(&lint, &(severity, _))| (lint, severity))
}

/// Returns the configured severity for the lint with the given id or `None` if the lint is disabled.
pub fn severity(&self, lint: LintId) -> Option<Severity> {
self.lints.get(&lint).copied()
self.lints.get(&lint).map(|(severity, _)| *severity)
}

/// Returns `true` if the `lint` is enabled.
Expand All @@ -457,19 +459,25 @@ impl RuleSelection {
/// Enables `lint` and configures with the given `severity`.
///
/// Overrides any previous configuration for the lint.
pub fn enable(&mut self, lint: LintId, severity: Severity) {
self.lints.insert(lint, severity);
pub fn enable(&mut self, lint: LintId, severity: Severity, source: LintSource) {
self.lints.insert(lint, (severity, source));
}

/// Disables `lint` if it was previously enabled.
pub fn disable(&mut self, lint: LintId) {
self.lints.remove(&lint);
}
}

/// Merges the enabled lints from `other` into this selection.
///
/// Lints from `other` will override any existing configuration.
pub fn merge(&mut self, other: &RuleSelection) {
self.lints.extend(other.iter());
}
#[derive(Default, Copy, Clone, Debug, PartialEq, Eq)]
pub enum LintSource {
/// The user didn't enable the rule explicitly, instead it's enabled by default.
#[default]
Default,

/// The rule was enabled by using a CLI argument
Cli,

/// The rule was enabled in a configuration file.
File,
}
3 changes: 2 additions & 1 deletion crates/red_knot_wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use js_sys::Error;
use wasm_bindgen::prelude::*;

use red_knot_project::metadata::options::{EnvironmentOptions, Options};
use red_knot_project::metadata::value::RangedValue;
use red_knot_project::ProjectMetadata;
use red_knot_project::{Db, ProjectDatabase};
use ruff_db::diagnostic::Diagnostic;
Expand Down Expand Up @@ -48,7 +49,7 @@ impl Workspace {

workspace.apply_cli_options(Options {
environment: Some(EnvironmentOptions {
python_version: Some(settings.python_version.into()),
python_version: Some(RangedValue::cli(settings.python_version.into())),
..EnvironmentOptions::default()
}),
..Options::default()
Expand Down
Loading

0 comments on commit 997bd03

Please sign in to comment.