diff --git a/apps/oxlint/fixtures/invalid_config_tuple_rules/.oxlintrc.json b/apps/oxlint/fixtures/invalid_config_tuple_rules/.oxlintrc.json new file mode 100644 index 0000000000000..59790b7f76f0a --- /dev/null +++ b/apps/oxlint/fixtures/invalid_config_tuple_rules/.oxlintrc.json @@ -0,0 +1,13 @@ +{ + "categories": { + "correctness": "off" + }, + "rules": { + // Invalid config, "asc" and "desc" are the only valid values for the first config option. + "eslint/sort-keys": ["error", "foo", { "allowLineSeparatedGroups": true }], + // Invalid config, exceptRange should be a boolean. + "eslint/yoda": ["error", "never", { "exceptRange": 123 }], + // Invalid config, second option should be an object. + "eslint/eqeqeq": ["error", "always", "foo"] + } +} diff --git a/apps/oxlint/fixtures/valid_complex_config/.oxlintrc.json b/apps/oxlint/fixtures/valid_complex_config/.oxlintrc.json index fbec6466f7bb0..0e5c7c2090ec6 100644 --- a/apps/oxlint/fixtures/valid_complex_config/.oxlintrc.json +++ b/apps/oxlint/fixtures/valid_complex_config/.oxlintrc.json @@ -43,6 +43,11 @@ "vue/define-emits-declaration": ["error", "type-based"], "vue/define-props-declaration": ["error", "runtime"], - "jsdoc/require-returns": ["error", { "exemptedBy": ["test"] }] + "jsdoc/require-returns": ["error", { "exemptedBy": ["test"] }], + + // tuple config options + "eslint/sort-keys": ["error", "asc", { "allowLineSeparatedGroups": true }], + "eslint/yoda": ["error", "never", { "exceptRange": true }], + "eslint/eqeqeq": ["error", "always", { "null": "ignore" }] } } diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 68a2b61b518fb..2444bc9a433c5 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -1524,4 +1524,9 @@ export { redundant }; .with_cwd("fixtures/invalid_config_complex_enum".into()) .test_and_snapshot(&[]); } + + #[test] + fn test_invalid_config_invalid_config_tuple_rules() { + Tester::new().with_cwd("fixtures/invalid_config_tuple_rules".into()).test_and_snapshot(&[]); + } } diff --git a/apps/oxlint/src/snapshots/fixtures__invalid_config_tuple_rules_@oxlint.snap b/apps/oxlint/src/snapshots/fixtures__invalid_config_tuple_rules_@oxlint.snap new file mode 100644 index 0000000000000..0484218fcbcf5 --- /dev/null +++ b/apps/oxlint/src/snapshots/fixtures__invalid_config_tuple_rules_@oxlint.snap @@ -0,0 +1,16 @@ +--- +source: apps/oxlint/src/tester.rs +--- +########## +arguments: +working directory: fixtures/invalid_config_tuple_rules +---------- +Failed to parse oxlint configuration file. + + x Invalid configuration for rule `eqeqeq`: invalid type: string "foo", expected struct EqeqeqOptions, received `[ "always", "foo" ]` + | Invalid configuration for rule `sort-keys`: unknown variant `foo`, expected `desc` or `asc`, received `[ "foo", { "allowLineSeparatedGroups": true } ]` + | Invalid configuration for rule `yoda`: invalid type: integer `123`, expected a boolean, received `[ "never", { "exceptRange": 123 } ]` + +---------- +CLI result: InvalidOptionConfig +---------- diff --git a/apps/oxlint/src/snapshots/fixtures__valid_complex_config_@oxlint.snap b/apps/oxlint/src/snapshots/fixtures__valid_complex_config_@oxlint.snap index df55583749a75..28cd24f17bf29 100644 --- a/apps/oxlint/src/snapshots/fixtures__valid_complex_config_@oxlint.snap +++ b/apps/oxlint/src/snapshots/fixtures__valid_complex_config_@oxlint.snap @@ -6,7 +6,7 @@ arguments: working directory: fixtures/valid_complex_config ---------- Found 0 warnings and 0 errors. -Finished in ms on 0 files with 22 rules using 1 threads. +Finished in ms on 0 files with 25 rules using 1 threads. ---------- CLI result: LintSucceeded ---------- diff --git a/crates/oxc_linter/src/rule.rs b/crates/oxc_linter/src/rule.rs index 438d079f82f35..1c8bfb1b39562 100644 --- a/crates/oxc_linter/src/rule.rs +++ b/crates/oxc_linter/src/rule.rs @@ -91,6 +91,9 @@ pub trait Rule: Sized + Default + fmt::Debug { /// } /// } /// ``` +/// +/// For rules that take a tuple configuration object, e.g. `["foobar", { "param": true, "other_param": false }]`, +/// use [`TupleRuleConfig`] instead. #[derive(Debug, Clone)] pub struct DefaultRuleConfig(T); @@ -144,6 +147,85 @@ where } } +/// A wrapper type for deserializing ESLint-style tuple rule configurations. +/// +/// Some ESLint rules take configurations in tuple form, e.g. `["foo", { "bar": "baz" }]` +/// where different array elements correspond to different config fields. This type +/// deserializes the entire array as `T`, which should be a tuple struct with +/// `#[serde(default)]` to handle partial configurations. +/// +/// If the configuration is invalid, it will return a deserialization error that is +/// handled by the linter and reported to the user. +/// +/// # Examples +/// +/// ```ignore +/// #[derive(Debug, Default, Clone, Deserialize)] +/// #[serde(default)] +/// pub struct MyTupleRule(EnumOption, ObjectConfig); +/// +/// impl Rule for MyTupleRule { +/// fn from_configuration(value: serde_json::Value) -> Result { +/// serde_json::from_value::>(value).map(TupleRuleConfig::into_inner) +/// } +/// } +/// ``` +#[derive(Debug, Clone)] +pub struct TupleRuleConfig(T); + +impl TupleRuleConfig { + /// Unwraps the inner configuration value. + pub fn into_inner(self) -> T { + self.0 + } +} + +impl Default for TupleRuleConfig { + fn default() -> Self { + Self(T::default()) + } +} + +impl<'de, T> serde::Deserialize<'de> for TupleRuleConfig +where + T: serde::de::DeserializeOwned + Default, +{ + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error; + + let value = serde_json::Value::deserialize(deserializer)?; + + if let serde_json::Value::Array(arr) = value { + let config = if arr.is_empty() { + T::default() + } else { + // Parse the entire array as the tuple configuration. + let arr_value = serde_json::Value::Array(arr); + T::deserialize(&arr_value).map_err(|e| { + // Try to include the config array in the error message if we can. + // Collapse any whitespace so we emit a single-line message. + if let Ok(value_str) = serde_json::to_string_pretty(&arr_value) { + let compact = value_str.split_whitespace().collect::>().join(" "); + D::Error::custom(format!("{e}, received `{compact}`")) + } else { + D::Error::custom(e) + } + })? + }; + + Ok(TupleRuleConfig(config)) + } else if value == serde_json::Value::Null { + // Missing configuration (null) is treated as default (no rule options provided) + Ok(TupleRuleConfig(T::default())) + } else { + Err(D::Error::custom("Expected array for rule configuration")) + } + } +} + pub trait RuleRunner: Rule { /// `AstType`s that this rule acts on, or `None` if the codegen /// can't figure it out and the linter should call `run` on every node. @@ -416,9 +498,13 @@ impl From for FixKind { #[cfg(test)] mod test { - use crate::{RuleMeta, RuleRunner}; + use crate::{ + RuleMeta, RuleRunner, + rule::{DefaultRuleConfig, TupleRuleConfig}, + }; use super::RuleCategory; + use rustc_hash::FxHashMap; #[test] #[cfg(feature = "ruledocs")] @@ -515,6 +601,177 @@ mod test { ); } + #[test] + fn test_deserialize_default_rule_config_single() { + // single element present + assert_default_rule_config("[123]", &123u32); + assert_default_rule_config("[true]", &true); + assert_default_rule_config("[false]", &false); + + // empty array should use defaults + assert_default_rule_config("[]", &String::default()); + } + + #[derive(serde::Deserialize, Debug, PartialEq, Eq)] + #[serde(default)] + struct Obj { + foo: String, + } + + impl Default for Obj { + fn default() -> Self { + Self { foo: "defaultval".to_string() } + } + } + + #[derive(serde::Deserialize, Debug, PartialEq, Eq)] + #[serde(default)] + struct Pair(u32, Obj); + + impl Default for Pair { + fn default() -> Self { + Self(123u32, Obj::default()) + } + } + + #[test] + fn test_deserialize_tuple_rule_config() { + // both elements present + assert_tuple_rule_config( + r#"[42, { "foo": "abc" }]"#, + &Pair(42u32, Obj { foo: "abc".to_string() }), + ); + + // only first element present -> Since Pair has #[serde(default)], + // serde will use the default value for the missing second field. + assert_tuple_rule_config("[10]", &Pair(10u32, Obj { foo: "defaultval".to_string() })); + + // Should also be able to handle both elements if they are both passed + assert_tuple_rule_config( + r#"[10, { "foo": "bar" }]"#, + &Pair(10u32, Obj { foo: "bar".to_string() }), + ); + + // empty array -> both default + assert_tuple_rule_config("[]", &Pair(123u32, Obj { foo: "defaultval".to_string() })); + } + + #[test] + fn test_deserialize_default_rule_config_object_in_array() { + // Single-element array containing an object should parse into the object + // configuration (fallback behavior, not the "entire-array as T" path). + assert_default_rule_config(r#"[{ "foo": "xyz" }]"#, &Obj { foo: "xyz".to_string() }); + + // Empty array -> default + assert_default_rule_config("[]", &Obj { foo: "defaultval".to_string() }); + } + + #[derive(serde::Deserialize, Debug, PartialEq, Eq, Default)] + #[serde(default)] + struct ComplexConfig { + foo: FxHashMap, + } + + #[test] + fn test_deserialize_default_rule_config_with_complex_shape() { + // A complex object shape for the rule config, like + // `[ { "foo": { "obj": "value" } } ]`. + assert_default_rule_config( + r#"[ { "foo": { "obj": "value" } } ]"#, + &ComplexConfig { + foo: std::iter::once(("obj".to_string(), "value".to_string())).collect(), + }, + ); + } + + #[derive(serde::Deserialize, Debug, PartialEq, Eq, Default)] + #[serde(rename_all = "camelCase")] + enum EnumOptions { + #[default] + OptionA, + OptionB, + } + + #[test] + fn test_deserialize_default_rule_config_with_enum_config() { + // A basic enum config option. + assert_default_rule_config(r#"["optionA"]"#, &EnumOptions::OptionA); + + // Works with non-default value as well. + assert_default_rule_config(r#"["optionB"]"#, &EnumOptions::OptionB); + + // Works with an empty array. + assert_default_rule_config(r"[]", &EnumOptions::OptionA); + } + + #[derive(serde::Deserialize, Default, Debug, PartialEq, Eq)] + #[serde(default)] + struct TupleWithEnumAndObjectConfig(EnumOptions, Obj); + + #[test] + fn test_deserialize_tuple_rule_config_with_enum_and_object() { + // A basic enum config option with an object. + assert_tuple_rule_config( + r#"["optionA", { "foo": "bar" }]"#, + &TupleWithEnumAndObjectConfig(EnumOptions::OptionA, Obj { foo: "bar".to_string() }), + ); + + // Ensure that we can pass just one value and it'll provide the default for the second. + assert_tuple_rule_config( + r#"["optionB"]"#, + &TupleWithEnumAndObjectConfig( + EnumOptions::OptionB, + Obj { foo: "defaultval".to_string() }, + ), + ); + } + + #[derive(serde::Deserialize, Debug, PartialEq, Eq)] + #[serde(default)] + struct ExampleObjConfig { + baz: String, + qux: bool, + } + + impl Default for ExampleObjConfig { + fn default() -> Self { + Self { baz: "defaultbaz".to_string(), qux: false } + } + } + + #[test] + fn test_deserialize_default_rule_with_object_with_multiple_fields() { + // Test a rule config that is a simple object with multiple fields. + assert_default_rule_config( + r#"[{ "baz": "fooval", "qux": true }]"#, + &ExampleObjConfig { baz: "fooval".to_string(), qux: true }, + ); + + // Ensure that missing fields get their default values. + assert_default_rule_config( + r#"[{ "qux": true }]"#, + &ExampleObjConfig { baz: "defaultbaz".to_string(), qux: true }, + ); + } + + // Ensure that the provided JSON deserializes into the expected value with DefaultRuleConfig. + fn assert_default_rule_config(json: &str, expected: &T) + where + T: serde::de::DeserializeOwned + Default + PartialEq + std::fmt::Debug, + { + let de: DefaultRuleConfig = serde_json::from_str(json).unwrap(); + assert_eq!(de.into_inner(), *expected); + } + + // Ensure that the provided JSON deserializes into the expected value with TupleRuleConfig. + fn assert_tuple_rule_config(json: &str, expected: &T) + where + T: serde::de::DeserializeOwned + Default + PartialEq + std::fmt::Debug, + { + let de: TupleRuleConfig = serde_json::from_str(json).unwrap(); + assert_eq!(de.into_inner(), *expected); + } + fn assert_rule_runs_on_node_types( rule: &R, node_types: &[oxc_ast::AstType], diff --git a/crates/oxc_linter/src/rules/eslint/eqeqeq.rs b/crates/oxc_linter/src/rules/eslint/eqeqeq.rs index 95a33925f0299..5bfbb51e34685 100644 --- a/crates/oxc_linter/src/rules/eslint/eqeqeq.rs +++ b/crates/oxc_linter/src/rules/eslint/eqeqeq.rs @@ -7,9 +7,14 @@ use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; use oxc_syntax::operator::{BinaryOperator, UnaryOperator}; use schemars::JsonSchema; +use serde::Deserialize; -use crate::fixer::{RuleFix, RuleFixer}; -use crate::{AstNode, context::LintContext, rule::Rule}; +use crate::{ + AstNode, + context::LintContext, + fixer::{RuleFix, RuleFixer}, + rule::{Rule, TupleRuleConfig}, +}; fn eqeqeq_diagnostic(actual: &str, expected: &str, span: Span) -> OxcDiagnostic { OxcDiagnostic::warn(format!("Expected {expected} and instead saw {actual}")) @@ -17,11 +22,40 @@ fn eqeqeq_diagnostic(actual: &str, expected: &str, span: Span) -> OxcDiagnostic .with_label(span) } -#[derive(Debug, Default, Clone, JsonSchema)] +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] #[serde(rename_all = "camelCase", default)] -pub struct Eqeqeq { - compare_type: CompareType, - null_type: NullType, +pub struct Eqeqeq(CompareType, EqeqeqOptions); + +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "camelCase", default, deny_unknown_fields)] +pub struct EqeqeqOptions { + /// Configuration for whether to allow/disallow comparisons against `null`, + /// e.g. `foo == null` or `foo != null` + null: NullType, +} + +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "kebab-case")] +enum CompareType { + /// Always require triple-equal comparisons, `===`/`!==`. + /// This is the default. + #[default] + Always, + /// Allow certain safe comparisons to use `==`/`!=` (`typeof`, literals, nullish). + Smart, +} + +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "kebab-case")] +enum NullType { + /// Always require triple-equals when comparing with null, `=== null`/`!== null`. + /// This is the default. + #[default] + Always, + /// Never require triple-equals when comparing with null, always use `== null`/`!= null`. + Never, + /// Ignore null comparisons, allow either `== null`/`!= null` or `=== null`/`!== null`. + Ignore, } declare_oxc_lint!( @@ -33,25 +67,7 @@ declare_oxc_lint!( /// /// Using non-strict equality operators leads to unexpected behavior due to type coercion, which can cause hard-to-find bugs. /// - /// ### Options - /// - /// First option: - /// - Type: `string` - /// - Default: `"always"` - /// - /// Possible values: - /// * `"always"` - always require `===`/`!==` - /// * `"smart"` - allow safe comparisons (`typeof`, literals, nullish) - /// - /// Second option (only used with `"always"`): - /// - Type: `object` - /// - Properties: - /// - `null`: `string` (default: `"always"`) - `"ignore"` allows `== null` and `!= null`. - /// - /// Possible values for `null`: - /// * `"always"` - always require `=== null`/`!== null` - /// * `"never"` - always require `== null`/`!= null` - /// * `"ignore"` - allow both `== null`/`!= null` and `=== null`/`!== null` + /// ### Examples /// /// Example JSON configuration: /// ```json @@ -60,8 +76,6 @@ declare_oxc_lint!( /// } /// ``` /// - /// ### Examples - /// /// #### `"always"` (default) /// /// Examples of **incorrect** code for this rule: @@ -156,7 +170,6 @@ declare_oxc_lint!( /// if (foo == null) {} /// if (foo != null) {} /// ``` - /// Eqeqeq, eslint, pedantic, @@ -164,45 +177,9 @@ declare_oxc_lint!( config = Eqeqeq, ); -#[derive(Debug, Default, Clone, JsonSchema)] -#[serde(rename_all = "lowercase")] -enum CompareType { - #[default] - Always, - Smart, -} - -impl CompareType { - pub fn from(raw: &str) -> Self { - match raw { - "smart" => Self::Smart, - _ => Self::Always, - } - } -} - -#[derive(Debug, Default, Clone, JsonSchema)] -#[serde(rename_all = "lowercase")] -enum NullType { - #[default] - Always, - Never, - Ignore, -} - -impl NullType { - pub fn from(raw: &str) -> Self { - match raw { - "always" => Self::Always, - "never" => Self::Never, - _ => Self::Ignore, - } - } -} - impl Eqeqeq { fn report_inverse_null_comparison(&self, binary_expr: &BinaryExpression, ctx: &LintContext) { - if !matches!(self.null_type, NullType::Never) { + if !matches!(self.1.null, NullType::Never) { return; } let operator = binary_expr.operator.as_str(); @@ -216,26 +193,17 @@ impl Eqeqeq { impl Rule for Eqeqeq { fn from_configuration(value: serde_json::Value) -> Result { - let first_arg = value.get(0).and_then(serde_json::Value::as_str); - - let null_type = value - .get(usize::from(first_arg.is_some())) - .and_then(|v| v.get("null")) - .and_then(serde_json::Value::as_str) - .map(NullType::from) - .unwrap_or_default(); - - let compare_type = first_arg.map(CompareType::from).unwrap_or_default(); - - Ok(Self { compare_type, null_type }) + serde_json::from_value::>(value).map(TupleRuleConfig::into_inner) } fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { let AstKind::BinaryExpression(binary_expr) = node.kind() else { return; }; + let Eqeqeq(compare_type, options) = self; + let is_null_comparison = is_null_check(binary_expr); - let must_enforce_null = matches!(self.null_type, NullType::Always); + let must_enforce_null = matches!(options.null, NullType::Always); if !matches!(binary_expr.operator, BinaryOperator::Equality | BinaryOperator::Inequality) { if is_null_comparison { @@ -251,7 +219,7 @@ impl Rule for Eqeqeq { // - Comparing two literal values // - Evaluating the value of typeof // - Comparing against null - if matches!(self.compare_type, CompareType::Smart) + if matches!(compare_type, CompareType::Smart) && (is_typeof_check || is_same_type_literals || is_null_comparison) { return; @@ -361,13 +329,11 @@ fn test() { ("foo == null", Some(json!(["smart"]))), ("foo === null", None), // Always use === or !== with `null` - ("null === null", Some(json!(["always", {"null": "always"}]))), + ("null === null", Some(json!(["always", { "null": "always" }]))), // Never use === or !== with `null` - ("null == null", Some(json!(["always", {"null": "never"}]))), + ("null == null", Some(json!(["always", { "null": "never" }]))), // Do not apply this rule to `null`. - ("null == null", Some(json!(["smart", {"null": "ignore"}]))), - // Issue: - ("href != null", Some(json!([{"null": "ignore"}]))), + ("null == null", Some(json!(["smart", { "null": "ignore" }]))), ("a === b", Some(serde_json::json!(["always"]))), ("typeof a == 'number'", Some(serde_json::json!(["smart"]))), ("'string' != typeof a", Some(serde_json::json!(["smart"]))), @@ -376,6 +342,7 @@ fn test() { ("true == true", Some(serde_json::json!(["smart"]))), ("null == a", Some(serde_json::json!(["smart"]))), ("a == null", Some(serde_json::json!(["smart"]))), + // We intentionally do not support this option because it is deprecated in ESLint. // ("null == a", Some(serde_json::json!(["allow-null"]))), // ("a == null", Some(serde_json::json!(["allow-null"]))), ("a == null", Some(serde_json::json!(["always", { "null": "ignore" }]))), @@ -389,11 +356,15 @@ fn test() { ("null != null", Some(serde_json::json!(["always", { "null": "never" }]))), ("foo === /abc/u", Some(serde_json::json!(["always", { "null": "never" }]))), // { "ecmaVersion": 2015 }, ("foo === 1n", Some(serde_json::json!(["always", { "null": "never" }]))), // { "ecmaVersion": 2020 } + // Originally for issue: + // We previously allowed exclusion of the first value, but that + // causes difficulties in validation, so let's not. + ("href != null", Some(json!(["always", { "null": "ignore" }]))), ]; let fail = vec![ // ESLint will perform like below case - ("null >= 1", Some(json!(["always", {"null": "never"}]))), + ("null >= 1", Some(json!(["always", { "null": "never" }]))), ("typeof foo == 'undefined'", None), ("'hello' != 'world'", None), ("0 == 0", None), @@ -403,7 +374,7 @@ fn test() { ("foo == true", None), ("bananas != 1", None), ("value == undefined", None), - ("null == null", Some(json!(["always", {"null": "always"}]))), + ("null == null", Some(json!(["always", { "null": "always" }]))), ]; let fix = vec![ diff --git a/crates/oxc_linter/src/rules/eslint/sort_keys.rs b/crates/oxc_linter/src/rules/eslint/sort_keys.rs index ea5344a4e8ef0..ea67fb3f0bf9c 100644 --- a/crates/oxc_linter/src/rules/eslint/sort_keys.rs +++ b/crates/oxc_linter/src/rules/eslint/sort_keys.rs @@ -8,14 +8,18 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; -use crate::{AstNode, context::LintContext, rule::Rule}; +use crate::{ + AstNode, + context::LintContext, + rule::{Rule, TupleRuleConfig}, +}; -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default, Clone, Deserialize)] pub struct SortKeys(Box); -#[derive(Debug, Default, Clone, Eq, PartialEq, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Default, Clone, Eq, PartialEq, Deserialize, JsonSchema)] #[serde(rename_all = "lowercase")] /// Sorting order for keys. Accepts "asc" for ascending or "desc" for descending. pub enum SortOrder { @@ -24,8 +28,8 @@ pub enum SortOrder { Asc, } -#[derive(Debug, Clone, JsonSchema)] -#[serde(rename_all = "camelCase", default)] +#[derive(Debug, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "camelCase", default, deny_unknown_fields)] pub struct SortKeysOptions { /// Whether the sort comparison is case-sensitive (A < a when true). case_sensitive: bool, @@ -49,19 +53,10 @@ impl Default for SortKeysOptions { } } -#[derive(Debug, Default, Clone, JsonSchema)] +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] #[serde(default)] pub struct SortKeysConfig(SortOrder, SortKeysOptions); -impl SortKeys { - fn sort_order(&self) -> &SortOrder { - &(*self.0).0 - } - fn options(&self) -> &SortKeysOptions { - &(*self.0).1 - } -} - fn sort_properties_diagnostic(span: Span) -> OxcDiagnostic { OxcDiagnostic::warn("Object keys should be sorted").with_label(span) } @@ -102,50 +97,16 @@ declare_oxc_lint!( impl Rule for SortKeys { fn from_configuration(value: serde_json::Value) -> Result { - let Some(config_array) = value.as_array() else { - return Ok(Self::default()); - }; - - let sort_order = if config_array.is_empty() { - SortOrder::Asc - } else { - config_array[0].as_str().map_or(SortOrder::Asc, |s| match s { - "desc" => SortOrder::Desc, - _ => SortOrder::Asc, - }) - }; - - let config = if config_array.len() > 1 { - config_array[1].as_object().unwrap() - } else { - &serde_json::Map::new() - }; - - let case_sensitive = - config.get("caseSensitive").and_then(serde_json::Value::as_bool).unwrap_or(true); - let natural = config.get("natural").and_then(serde_json::Value::as_bool).unwrap_or(false); - let min_keys = config - .get("minKeys") - .and_then(serde_json::Value::as_u64) - .map_or(2, |n| n.try_into().unwrap_or(2)); - let allow_line_separated_groups = config - .get("allowLineSeparatedGroups") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); - - Ok(Self(Box::new(SortKeysConfig( - sort_order, - SortKeysOptions { case_sensitive, natural, min_keys, allow_line_separated_groups }, - )))) + serde_json::from_value::>(value).map(TupleRuleConfig::into_inner) } fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { if let AstKind::ObjectExpression(dec) = node.kind() { - let options = self.options(); + let SortKeysConfig(sort_order, options) = &*self.0; + if dec.properties.len() < options.min_keys { return; } - let sort_order = self.sort_order().clone(); let mut property_groups: Vec> = vec![vec![]]; @@ -194,7 +155,7 @@ impl Rule for SortKeys { alphanumeric_sort(group); } - if sort_order == SortOrder::Desc { + if sort_order == &SortOrder::Desc { group.reverse(); } } @@ -264,7 +225,7 @@ impl Rule for SortKeys { } else { alphanumeric_sort(&mut sorted_keys); } - if sort_order == SortOrder::Desc { + if sort_order == &SortOrder::Desc { sorted_keys.reverse(); } diff --git a/crates/oxc_linter/src/rules/eslint/yoda.rs b/crates/oxc_linter/src/rules/eslint/yoda.rs index e544b19aa80c7..58d962e41e9b1 100644 --- a/crates/oxc_linter/src/rules/eslint/yoda.rs +++ b/crates/oxc_linter/src/rules/eslint/yoda.rs @@ -9,8 +9,15 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_ecmascript::{ToBigInt, WithoutGlobalReferenceInformation}; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; - -use crate::{AstNode, context::LintContext, rule::Rule, utils::is_same_expression}; +use schemars::JsonSchema; +use serde::Deserialize; + +use crate::{ + AstNode, + context::LintContext, + rule::{Rule, TupleRuleConfig}, + utils::is_same_expression, +}; fn yoda_diagnostic(span: Span, never: bool, operator: &str) -> OxcDiagnostic { let expected_side = if never { "right" } else { "left" }; @@ -19,16 +26,39 @@ fn yoda_diagnostic(span: Span, never: bool, operator: &str) -> OxcDiagnostic { .with_label(span) } -#[derive(Debug, Clone)] -pub struct Yoda { - never: bool, +#[derive(Debug, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "camelCase", default)] +pub struct Yoda(AllowYoda, YodaOptions); + +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "camelCase", default, deny_unknown_fields)] +pub struct YodaOptions { + /// If the `"exceptRange"` property is `true`, the rule *allows* yoda conditions + /// in range comparisons which are wrapped directly in parentheses, including the + /// parentheses of an `if` or `while` condition. + /// A *range* comparison tests whether a variable is inside or outside the range + /// between two literal values. except_range: bool, + /// If the `"onlyEquality"` property is `true`, the rule reports yoda + /// conditions *only* for the equality operators `==` and `===`. The `onlyEquality` + /// option allows a superset of the exceptions which `exceptRange` allows, thus + /// both options are not useful together. only_equality: bool, } +#[derive(Debug, Default, Clone, JsonSchema, Deserialize, PartialEq)] +#[serde(rename_all = "kebab-case")] +enum AllowYoda { + /// The default `"never"` option can have exception options in an object literal, via `exceptRange` and `onlyEquality`. + #[default] + Never, + /// The `"always"` option requires that literal values must always come first in comparisons. + Always, +} + impl Default for Yoda { fn default() -> Self { - Self { never: true, except_range: false, only_equality: false } + Self(AllowYoda::Never, YodaOptions { except_range: false, only_equality: false }) } } @@ -46,25 +76,20 @@ declare_oxc_lint!( // // ... /// } /// ``` + /// /// This is called a Yoda condition because it reads as, "if red equals the color", similar to the way the Star Wars character Yoda speaks. Compare to the other way of arranging the operands: + /// /// ```js /// if (color === "red") { /// // ... /// } /// ``` + /// /// This typically reads, "if the color equals red", which is arguably a more natural way to describe the comparison. /// Proponents of Yoda conditions highlight that it is impossible to mistakenly use `=` instead of `==` because you cannot assign to a literal value. Doing so will cause a syntax error and you will be informed of the mistake early on. This practice was therefore very common in early programming where tools were not yet available. /// Opponents of Yoda conditions point out that tooling has made us better programmers because tools will catch the mistaken use of `=` instead of `==` (ESLint will catch this for you). Therefore, they argue, the utility of the pattern doesn't outweigh the readability hit the code takes while using Yoda conditions. /// - /// ### Options - /// - /// This rule can take a string option: - /// * If it is the default `"never"`, then comparisons must never be Yoda conditions. - /// * If it is `"always"`, then the literal value must always come first. - /// The default `"never"` option can have exception options in an object literal: - /// * If the `"exceptRange"` property is `true`, the rule *allows* yoda conditions in range comparisons which are wrapped directly in parentheses, including the parentheses of an `if` or `while` condition. The default value is `false`. A *range* comparison tests whether a variable is inside or outside the range between two literal values. - /// * If the `"onlyEquality"` property is `true`, the rule reports yoda conditions *only* for the equality operators `==` and `===`. The default value is `false`. - /// The `onlyEquality` option allows a superset of the exceptions which `exceptRange` allows, thus both options are not useful together. + /// ### Examples /// /// #### never /// @@ -119,6 +144,7 @@ declare_oxc_lint!( /// #### exceptRange /// /// Examples of **correct** code for the `"never", { "exceptRange": true }` options: + /// /// ```js /// function isReddish(color) { /// return (color.hue < 60 || 300 < color.hue); @@ -189,34 +215,13 @@ declare_oxc_lint!( Yoda, eslint, style, - fix + fix, + config = Yoda, ); impl Rule for Yoda { fn from_configuration(value: serde_json::Value) -> Result { - let mut config = Self::default(); - - let Some(arr) = value.as_array() else { - return Ok(config); - }; - - let option1 = arr.first().and_then(serde_json::Value::as_str); - let option2 = arr.get(1).and_then(serde_json::Value::as_object); - - if option1 == Some("always") { - config.never = false; - } - - if let Some(option2) = option2 { - if option2.get("exceptRange").and_then(serde_json::Value::as_bool) == Some(true) { - config.except_range = true; - } - if option2.get("onlyEquality").and_then(serde_json::Value::as_bool) == Some(true) { - config.only_equality = true; - } - } - - Ok(config) + serde_json::from_value::>(value).map(TupleRuleConfig::into_inner) } fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { @@ -224,11 +229,13 @@ impl Rule for Yoda { return; }; + let Yoda(allow_yoda, options) = self; + let parent_node = ctx.nodes().parent_node(node.id()); if let AstKind::LogicalExpression(logical_expr) = parent_node.kind() { let parent_logical_expr = ctx.nodes().parent_node(parent_node.id()); - if self.except_range + if options.except_range && is_parenthesized(parent_logical_expr) && is_range(logical_expr, ctx) { @@ -240,18 +247,18 @@ impl Rule for Yoda { return; } - if self.only_equality && !is_equality(expr) { + if options.only_equality && !is_equality(expr) { return; } // never - if self.never && is_yoda(expr) { - do_diagnostic_with_fix(expr, ctx, self.never); + if allow_yoda == &AllowYoda::Never && is_yoda(expr) { + do_diagnostic_with_fix(expr, ctx, true); } // always - if !self.never && is_not_yoda(expr) { - do_diagnostic_with_fix(expr, ctx, self.never); + if allow_yoda == &AllowYoda::Always && is_not_yoda(expr) { + do_diagnostic_with_fix(expr, ctx, false); } } } diff --git a/crates/oxc_linter/tests/rule_configuration_documentation_test.rs b/crates/oxc_linter/tests/rule_configuration_documentation_test.rs index 4a3d507d23388..25ebb9f75a693 100644 --- a/crates/oxc_linter/tests/rule_configuration_documentation_test.rs +++ b/crates/oxc_linter/tests/rule_configuration_documentation_test.rs @@ -35,7 +35,6 @@ fn test_rules_with_custom_configuration_have_schema() { "eslint/no-empty-function", "eslint/no-restricted-imports", "eslint/no-warning-comments", - "eslint/yoda", // jest "jest/valid-title", // react