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.

Sign up for GitHub

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

Remove E999 as a rule, disallow any disablement methods for syntax error #11901

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Remove E999 as a rule, disallow any disablement methods for syntax error #11901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ff212c
Do not consider `E999` (syntax error) as a rule
dhruvmanila Jun 26, 2024
4907e45
Deprecate `E999`
dhruvmanila Jun 26, 2024
02b5c55
Code review changes
dhruvmanila Jun 26, 2024
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
30 changes: 15 additions & 15 deletions crates/ruff/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use tempfile::NamedTempFile;

use ruff_cache::{CacheKey, CacheKeyHasher};
use ruff_diagnostics::{DiagnosticKind, Fix};
use ruff_linter::message::Message;
use ruff_linter::message::{DiagnosticMessage, Message};
use ruff_linter::{warn_user, VERSION};
use ruff_macros::CacheKey;
use ruff_notebook::NotebookIndex;
Expand Down Expand Up @@ -333,12 +333,14 @@ impl FileCache {
let file = SourceFileBuilder::new(path.to_string_lossy(), &*lint.source).finish();
lint.messages
.iter()
.map(|msg| Message {
kind: msg.kind.clone(),
range: msg.range,
fix: msg.fix.clone(),
file: file.clone(),
noqa_offset: msg.noqa_offset,
.map(|msg| {
Message::Diagnostic(DiagnosticMessage {
kind: msg.kind.clone(),
range: msg.range,
fix: msg.fix.clone(),
file: file.clone(),
noqa_offset: msg.noqa_offset,
})
})
.collect()
};
Expand Down Expand Up @@ -412,18 +414,19 @@ impl LintCacheData {
notebook_index: Option<NotebookIndex>,
) -> Self {
let source = if let Some(msg) = messages.first() {
msg.file.source_text().to_owned()
msg.source_file().source_text().to_owned()
} else {
String::new() // No messages, no need to keep the source!
};

let messages = messages
.iter()
.filter_map(|message| message.as_diagnostic_message())
.map(|msg| {
// Make sure that all message use the same source file.
assert_eq!(
msg.file,
messages.first().unwrap().file,
&msg.file,
messages.first().unwrap().source_file(),
"message uses a different source file"
);
CacheMessage {
Expand Down Expand Up @@ -571,6 +574,7 @@ mod tests {
use test_case::test_case;

use ruff_cache::CACHE_DIR_NAME;
use ruff_linter::message::Message;
use ruff_linter::settings::flags;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_python_ast::PySourceType;
Expand Down Expand Up @@ -633,11 +637,7 @@ mod tests {
UnsafeFixes::Enabled,
)
.unwrap();
if diagnostics
.messages
.iter()
.any(|m| m.kind.name == "SyntaxError")
{
if diagnostics.messages.iter().any(Message::is_syntax_error) {
parse_errors.push(path.clone());
}
paths.push(path);
Expand Down
92 changes: 48 additions & 44 deletions crates/ruff/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@ use std::path::Path;
use anyhow::{Context, Result};
use colored::Colorize;
use log::{debug, error, warn};
use ruff_linter::codes::Rule;
use rustc_hash::FxHashMap;

use ruff_diagnostics::Diagnostic;
use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource};
use ruff_linter::logging::DisplayParseError;
use ruff_linter::message::Message;
use ruff_linter::message::{Message, SyntaxErrorMessage};
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::registry::AsRule;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::source_kind::{SourceError, SourceKind};
use ruff_linter::{fs, IOError, SyntaxError};
use ruff_linter::{fs, IOError};
use ruff_notebook::{Notebook, NotebookError, NotebookIndex};
use ruff_python_ast::{PySourceType, SourceType, TomlSourceType};
use ruff_source_file::SourceFileBuilder;
Expand Down Expand Up @@ -55,57 +55,61 @@ impl Diagnostics {
path: Option<&Path>,
settings: &LinterSettings,
) -> Self {
let diagnostic = match err {
match err {
// IO errors.
SourceError::Io(_)
| SourceError::Notebook(NotebookError::Io(_) | NotebookError::Json(_)) => {
Diagnostic::new(
IOError {
message: err.to_string(),
},
TextRange::default(),
)
if settings.rules.enabled(Rule::IOError) {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let source_file = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::from_diagnostic(
Diagnostic::new(
IOError {
message: err.to_string(),
},
TextRange::default(),
),
source_file,
TextSize::default(),
)],
FxHashMap::default(),
)
} else {
match path {
Some(path) => {
warn!(
"{}{}{} {err}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
}
None => {
warn!("{}{} {err}", "Failed to lint".bold(), ":".bold());
}
}

Self::default()
}
}
// Syntax errors.
SourceError::Notebook(
NotebookError::InvalidJson(_)
| NotebookError::InvalidSchema(_)
| NotebookError::InvalidFormat(_),
) => Diagnostic::new(
SyntaxError {
message: err.to_string(),
},
TextRange::default(),
),
};

if settings.rules.enabled(diagnostic.kind.rule()) {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::from_diagnostic(
diagnostic,
dummy,
TextSize::default(),
)],
FxHashMap::default(),
)
} else {
match path {
Some(path) => {
warn!(
"{}{}{} {err}",
"Failed to lint ".bold(),
fs::relativize_path(path).bold(),
":".bold()
);
}
None => {
warn!("{}{} {err}", "Failed to lint".bold(), ":".bold());
}
) => {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::SyntaxError(SyntaxErrorMessage {
message: err.to_string(),
range: TextRange::default(),
file: dummy,
})],
FxHashMap::default(),
)
}

Self::default()
}
}
}
Expand Down
63 changes: 36 additions & 27 deletions crates/ruff/src/printer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ use ruff_linter::fs::relativize_path;
use ruff_linter::logging::LogLevel;
use ruff_linter::message::{
AzureEmitter, Emitter, EmitterContext, GithubEmitter, GitlabEmitter, GroupedEmitter,
JsonEmitter, JsonLinesEmitter, JunitEmitter, PylintEmitter, RdjsonEmitter, SarifEmitter,
TextEmitter,
JsonEmitter, JsonLinesEmitter, JunitEmitter, Message, MessageKind, PylintEmitter,
RdjsonEmitter, SarifEmitter, TextEmitter,
};
use ruff_linter::notify_user;
use ruff_linter::registry::{AsRule, Rule};
use ruff_linter::registry::Rule;
use ruff_linter::settings::flags::{self};
use ruff_linter::settings::types::{OutputFormat, UnsafeFixes};

Expand All @@ -37,12 +37,13 @@ bitflags! {

#[derive(Serialize)]
struct ExpandedStatistics {
code: SerializeRuleAsCode,
name: SerializeRuleAsTitle,
code: Option<SerializeRuleAsCode>,
name: SerializeMessageKindAsTitle,
count: usize,
fixable: bool,
}

#[derive(Copy, Clone)]
struct SerializeRuleAsCode(Rule);

impl Serialize for SerializeRuleAsCode {
Expand All @@ -66,26 +67,26 @@ impl From<Rule> for SerializeRuleAsCode {
}
}

struct SerializeRuleAsTitle(Rule);
struct SerializeMessageKindAsTitle(MessageKind);

impl Serialize for SerializeRuleAsTitle {
impl Serialize for SerializeMessageKindAsTitle {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.serialize_str(self.0.as_ref())
serializer.serialize_str(self.0.as_str())
}
}

impl Display for SerializeRuleAsTitle {
impl Display for SerializeMessageKindAsTitle {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0.as_ref())
write!(f, "{}", self.0.as_str())
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
}
}

impl From<Rule> for SerializeRuleAsTitle {
fn from(rule: Rule) -> Self {
Self(rule)
impl From<MessageKind> for SerializeMessageKindAsTitle {
fn from(kind: MessageKind) -> Self {
Self(kind)
}
}

Expand Down Expand Up @@ -341,24 +342,23 @@ impl Printer {
let statistics: Vec<ExpandedStatistics> = diagnostics
.messages
.iter()
.map(|message| (message.kind.rule(), message.fix.is_some()))
.sorted()
.fold(vec![], |mut acc, (rule, fixable)| {
if let Some((prev_rule, _, count)) = acc.last_mut() {
if *prev_rule == rule {
.sorted_by_key(|message| (message.rule(), message.fixable()))
.fold(vec![], |mut acc: Vec<(&Message, usize)>, message| {
if let Some((prev_message, count)) = acc.last_mut() {
if prev_message.rule() == message.rule() {
*count += 1;
return acc;
}
}
acc.push((rule, fixable, 1));
acc.push((message, 1));
acc
})
.iter()
.map(|(rule, fixable, count)| ExpandedStatistics {
code: (*rule).into(),
name: (*rule).into(),
count: *count,
fixable: *fixable,
.map(|&(message, count)| ExpandedStatistics {
code: message.rule().map(std::convert::Into::into),
name: message.kind().into(),
count,
fixable: message.fixable(),
})
.sorted_by_key(|statistic| Reverse(statistic.count))
.collect();
Expand All @@ -381,7 +381,12 @@ impl Printer {
);
let code_width = statistics
.iter()
.map(|statistic| statistic.code.to_string().len())
.map(|statistic| {
statistic
.code
.map_or_else(String::new, |rule| rule.to_string())
.len()
})
.max()
.unwrap();
let any_fixable = statistics.iter().any(|statistic| statistic.fixable);
Expand All @@ -395,7 +400,11 @@ impl Printer {
writer,
"{:>count_width$}\t{:<code_width$}\t{}{}",
statistic.count.to_string().bold(),
statistic.code.to_string().red().bold(),
statistic
.code
.map_or_else(String::new, |rule| rule.to_string())
.red()
.bold(),
if any_fixable {
if statistic.fixable {
&fixable
Expand Down Expand Up @@ -545,7 +554,7 @@ impl FixableStatistics {
let mut unapplicable_unsafe = 0;

for message in &diagnostics.messages {
if let Some(fix) = &message.fix {
if let Some(fix) = message.fix() {
if fix.applies(unsafe_fixes.required_applicability()) {
applicable += 1;
} else {
Expand Down
Loading
Loading