Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hidden --extension to override inference of source type from file extension #8373

Merged
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions crates/ruff_cli/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use ruff_linter::line_width::LineLength;
use ruff_linter::logging::LogLevel;
use ruff_linter::registry::Rule;
use ruff_linter::settings::types::{
FilePattern, PatternPrefixPair, PerFileIgnore, PreviewMode, PythonVersion, SerializationFormat,
UnsafeFixes,
ExtensionPair, FilePattern, PatternPrefixPair, PerFileIgnore, PreviewMode, PythonVersion,
SerializationFormat, UnsafeFixes,
};
use ruff_linter::{RuleParser, RuleSelector, RuleSelectorParser};
use ruff_workspace::configuration::{Configuration, RuleSelection};
Expand Down Expand Up @@ -351,6 +351,9 @@ pub struct CheckCommand {
conflicts_with = "watch",
)]
pub show_settings: bool,
/// List of mappings from file extension to language (one of ["python", "ipynb", "pyi"]).
#[arg(long, value_delimiter = ',', hide = true)]
pub extension_override: Option<Vec<ExtensionPair>>,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
/// Dev-only argument to show fixes
#[arg(long, hide = true)]
pub ecosystem_ci: bool,
Expand Down Expand Up @@ -500,6 +503,7 @@ impl CheckCommand {
statistics: self.statistics,
stdin_filename: self.stdin_filename,
watch: self.watch,
extension_override: self.extension_override,
},
CliOverrides {
dummy_variable_rgx: self.dummy_variable_rgx,
Expand Down Expand Up @@ -598,6 +602,7 @@ pub struct CheckArguments {
pub statistics: bool,
pub stdin_filename: Option<PathBuf>,
pub watch: bool,
pub extension_override: Option<Vec<ExtensionPair>>,
}

/// CLI settings that are distinct from configuration (commands, lists of files,
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_cli/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ mod tests {
use anyhow::Result;
use filetime::{set_file_mtime, FileTime};
use itertools::Itertools;
use rustc_hash::FxHashMap;
use test_case::test_case;

use ruff_cache::CACHE_DIR_NAME;
Expand Down Expand Up @@ -627,6 +628,7 @@ mod tests {
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
&FxHashMap::default(),
)
.unwrap();
if diagnostics
Expand Down Expand Up @@ -673,6 +675,7 @@ mod tests {
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
&FxHashMap::default(),
)
.unwrap();
}
Expand Down Expand Up @@ -1046,6 +1049,7 @@ mod tests {
flags::Noqa::Enabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
&FxHashMap::default(),
)
}

Expand Down
19 changes: 17 additions & 2 deletions crates/ruff_cli/src/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_hash::FxHashMap;
use ruff_diagnostics::Diagnostic;
use ruff_linter::message::Message;
use ruff_linter::registry::Rule;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::settings::types::{Language, UnsafeFixes};
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::{fs, warn_user_once, IOError};
use ruff_python_ast::imports::ImportMap;
Expand All @@ -30,6 +30,7 @@ use crate::diagnostics::Diagnostics;
use crate::panic::catch_unwind;

/// Run the linter over a collection of files.
#[allow(clippy::too_many_arguments)]
pub(crate) fn check(
files: &[PathBuf],
pyproject_config: &PyprojectConfig,
Expand All @@ -38,6 +39,7 @@ pub(crate) fn check(
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
extension_override: &FxHashMap<String, Language>,
) -> Result<Diagnostics> {
// Collect all the Python files to check.
let start = Instant::now();
Expand Down Expand Up @@ -103,6 +105,7 @@ pub(crate) fn check(
noqa,
fix_mode,
unsafe_fixes,
extension_override,
)
.map_err(|e| {
(Some(path.to_path_buf()), {
Expand Down Expand Up @@ -184,6 +187,7 @@ pub(crate) fn check(

/// Wraps [`lint_path`](crate::diagnostics::lint_path) in a [`catch_unwind`](std::panic::catch_unwind) and emits
/// a diagnostic if the linting the file panics.
#[allow(clippy::too_many_arguments)]
fn lint_path(
path: &Path,
package: Option<&Path>,
Expand All @@ -192,9 +196,19 @@ fn lint_path(
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
extension_override: &FxHashMap<String, Language>,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Diagnostics> {
let result = catch_unwind(|| {
crate::diagnostics::lint_path(path, package, settings, cache, noqa, fix_mode, unsafe_fixes)
crate::diagnostics::lint_path(
path,
package,
settings,
cache,
noqa,
fix_mode,
unsafe_fixes,
extension_override,
)
});

match result {
Expand Down Expand Up @@ -280,6 +294,7 @@ mod test {
flags::Noqa::Disabled,
flags::FixMode::Generate,
UnsafeFixes::Enabled,
&FxHashMap::default(),
)
.unwrap();
let mut output = Vec::new();
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_cli/src/commands/check_stdin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use anyhow::Result;

use ruff_linter::packaging;
use ruff_linter::settings::flags;
use ruff_linter::settings::types::Language;
use ruff_workspace::resolver::{match_exclusion, python_file_at_path, PyprojectConfig};
use rustc_hash::FxHashMap;

use crate::args::CliOverrides;
use crate::diagnostics::{lint_stdin, Diagnostics};
Expand All @@ -17,6 +19,7 @@ pub(crate) fn check_stdin(
overrides: &CliOverrides,
noqa: flags::Noqa,
fix_mode: flags::FixMode,
extension_override: &FxHashMap<String, Language>,
) -> Result<Diagnostics> {
if let Some(filename) = filename {
if !python_file_at_path(filename, pyproject_config, overrides)? {
Expand All @@ -42,6 +45,7 @@ pub(crate) fn check_stdin(
&pyproject_config.settings,
noqa,
fix_mode,
extension_override,
)?;
diagnostics.messages.sort_unstable();
Ok(diagnostics)
Expand Down
85 changes: 56 additions & 29 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ use ruff_linter::logging::DisplayParseError;
use ruff_linter::message::Message;
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::registry::AsRule;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::settings::types::{Language, UnsafeFixes};
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::source_kind::{SourceError, SourceKind};
use ruff_linter::{fs, IOError, SyntaxError};
use ruff_notebook::{Notebook, NotebookError, NotebookIndex};
use ruff_python_ast::imports::ImportMap;
use ruff_python_ast::{SourceType, TomlSourceType};
use ruff_python_ast::{PySourceType, SourceType, TomlSourceType};
use ruff_source_file::{LineIndex, SourceCode, SourceFileBuilder};
use ruff_text_size::{TextRange, TextSize};
use ruff_workspace::Settings;
Expand Down Expand Up @@ -177,7 +177,23 @@ impl AddAssign for FixMap {
}
}

fn get_override_source_type(
path: Option<&Path>,
extension: &FxHashMap<String, Language>,
) -> Option<PySourceType> {
let Some(ext) = path.and_then(|p| p.extension()).and_then(|p| p.to_str()) else {
return None;
};
match extension.get(ext) {
Some(Language::Python) => Some(PySourceType::Python),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not noticing this in my first review 😓

How will this work if the cell contained IPython escape commands (!pwd, %matplotlib inline)? The parser mode is tied up with PySourceType. This mode is then used to allow the escape commands.

impl AsMode for PySourceType {
fn as_mode(&self) -> Mode {
match self {
PySourceType::Python | PySourceType::Stub => Mode::Module,
PySourceType::Ipynb => Mode::Ipython,
}
}
}

Now, if the cell contained escape commands and the override flag tells that this is a Python language (--extension-override ipynb:python), then it'll throw a syntax error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jupyterlab-lsp can replace notebook magics with equivalent python code in most cases, so in this use case these syntax errors are avoided (I think the code is here).

Any python code affected by magic ends up in a string so doesn't get checked, for example with magics like %time and %%time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting us know about that.

Coming from a public API perspective, this might still be a problem as, for example, if someone's trying to use it in a similar manner with an integration that doesn't perform those transformations, we'd throw an error.

I would love any other opinion on this. I'm fine with including the flag but if we include it in the config, then it's part of the public API which is where I'm unsure of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW jupyter nbconvert --to script also seems to convert notebook magics to python in the same way. nbdev has a routine callled scrub_magics which removes magics altogether, though it seems to be optional.

The docs could make it clear that python means python without notebookisms. Maybe something like:

Users who use this option to lint code extracted from notebooks as python should ensure that the code should be free of IPython magics, as these will cause a syntax error.

Some(Language::Ipynb) => Some(PySourceType::Ipynb),
Some(Language::Pyi) => Some(PySourceType::Stub),
None => None,
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
}
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved

/// Lint the source code at the given `Path`.
#[allow(clippy::too_many_arguments)]
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) fn lint_path(
path: &Path,
package: Option<&Path>,
Expand All @@ -186,6 +202,7 @@ pub(crate) fn lint_path(
noqa: flags::Noqa,
fix_mode: flags::FixMode,
unsafe_fixes: UnsafeFixes,
extension_override: &FxHashMap<String, Language>,
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Diagnostics> {
// Check the cache.
let caching = match cache {
Expand Down Expand Up @@ -220,32 +237,35 @@ pub(crate) fn lint_path(
};

debug!("Checking: {}", path.display());

let source_type = match SourceType::from(path) {
SourceType::Toml(TomlSourceType::Pyproject) => {
let messages = if settings
.rules
.iter_enabled()
.any(|rule_code| rule_code.lint_source().is_pyproject_toml())
{
let contents = match std::fs::read_to_string(path).map_err(SourceError::from) {
Ok(contents) => contents,
Err(err) => {
return Ok(Diagnostics::from_source_error(&err, Some(path), settings));
}
let source_type = match get_override_source_type(Some(path), extension_override) {
Some(source_type) => source_type,
None => match SourceType::from(path) {
SourceType::Toml(TomlSourceType::Pyproject) => {
let messages = if settings
.rules
.iter_enabled()
.any(|rule_code| rule_code.lint_source().is_pyproject_toml())
{
let contents = match std::fs::read_to_string(path).map_err(SourceError::from) {
Ok(contents) => contents,
Err(err) => {
return Ok(Diagnostics::from_source_error(&err, Some(path), settings));
}
};
let source_file =
SourceFileBuilder::new(path.to_string_lossy(), contents).finish();
lint_pyproject_toml(source_file, settings)
} else {
vec![]
};
let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish();
lint_pyproject_toml(source_file, settings)
} else {
vec![]
};
return Ok(Diagnostics {
messages,
..Diagnostics::default()
});
}
SourceType::Toml(_) => return Ok(Diagnostics::default()),
SourceType::Python(source_type) => source_type,
return Ok(Diagnostics {
messages,
..Diagnostics::default()
});
}
SourceType::Toml(_) => return Ok(Diagnostics::default()),
SourceType::Python(source_type) => source_type,
},
};

// Extract the sources from the file.
Expand Down Expand Up @@ -368,10 +388,17 @@ pub(crate) fn lint_stdin(
settings: &Settings,
noqa: flags::Noqa,
fix_mode: flags::FixMode,
extension_override: &FxHashMap<String, Language>,
) -> Result<Diagnostics> {
// TODO(charlie): Support `pyproject.toml`.
let SourceType::Python(source_type) = path.map(SourceType::from).unwrap_or_default() else {
return Ok(Diagnostics::default());
let source_type = if let Some(source_type) = get_override_source_type(path, extension_override)
{
source_type
} else {
let SourceType::Python(source_type) = path.map(SourceType::from).unwrap_or_default() else {
return Ok(Diagnostics::default());
};
source_type
};

// Extract the sources from the file.
Expand Down
10 changes: 10 additions & 0 deletions crates/ruff_cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use ruff_linter::settings::flags::FixMode;
use ruff_linter::settings::types::SerializationFormat;
use ruff_linter::{fs, warn_user, warn_user_once};
use ruff_workspace::Settings;
use rustc_hash::FxHashMap;

use crate::args::{Args, CheckCommand, Command, FormatCommand, HelpFormat};
use crate::printer::{Flags as PrinterFlags, Printer};
Expand Down Expand Up @@ -314,6 +315,11 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
unsafe_fixes,
printer_flags,
);
let extension_override = cli
.extension_override
.map(|eo| eo.into_iter().map(|y| (y.extension, y.language)))
.map(FxHashMap::from_iter)
.unwrap_or_default();

if cli.watch {
if output_format != SerializationFormat::Text {
Expand Down Expand Up @@ -342,6 +348,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
noqa.into(),
fix_mode,
unsafe_fixes,
&extension_override,
)?;
printer.write_continuously(&mut writer, &messages)?;

Expand Down Expand Up @@ -375,6 +382,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
noqa.into(),
fix_mode,
unsafe_fixes,
&extension_override,
)?;
printer.write_continuously(&mut writer, &messages)?;
}
Expand All @@ -392,6 +400,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
&overrides,
noqa.into(),
fix_mode,
&extension_override,
)?
} else {
commands::check::check(
Expand All @@ -402,6 +411,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
noqa.into(),
fix_mode,
unsafe_fixes,
&extension_override,
)?
};

Expand Down
Loading
Loading