From 9b4c5fbc352af8034c17e61181f884c475daf32c Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 30 Sep 2024 13:08:58 +0100 Subject: [PATCH 1/3] fix(lint): options for `noLabelWithoutControl` are optional --- CHANGELOG.md | 3 + crates/biome_analyze/CONTRIBUTING.md | 12 +- .../src/lint/a11y/no_label_without_control.rs | 220 ++++++++++-------- .../@biomejs/backend-jsonrpc/src/workspace.ts | 6 +- .../@biomejs/biome/configuration_schema.json | 10 +- 5 files changed, 136 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f70bbb2d78ce..35b653d67e20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b ### Configuration +#### Bug fixes +- Fix [#4125](https://github.com/biomejs/biome/issues/4125), where `noLabelWithoutControl` options where incorrectly marked as mandatory. Contributed by @ematipico + ### Editors - Fix a case where CSS files weren't correctly linted using the default configuration. Contributed by @ematipico diff --git a/crates/biome_analyze/CONTRIBUTING.md b/crates/biome_analyze/CONTRIBUTING.md index 7caa1af4ac30..11c9308c0782 100644 --- a/crates/biome_analyze/CONTRIBUTING.md +++ b/crates/biome_analyze/CONTRIBUTING.md @@ -161,16 +161,16 @@ We would like to set the options in the `biome.json` configuration file: } ``` -The first step is to create the Rust data representation of the rule's options. +The first step is to create the Rust data representation of the rule's options. Each option must be wrapped in a `Option`, this is required so the configuration schema won't mark them as required. ```rust use biome_deserialize_macros::Deserializable; #[derive(Clone, Debug, Default, Deserializable)] pub struct MyRuleOptions { - behavior: Behavior, - threshold: u8, - behavior_exceptions: Vec + behavior: Option, + threshold: Option, + behavior_exceptions: Option> } #[derive(Clone, Debug, Default, Deserializable)] @@ -215,10 +215,10 @@ You can simply use a derive macros: #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct MyRuleOptions { #[serde(default, skip_serializing_if = "is_default")] - main_behavior: Behavior, + main_behavior: Option, #[serde(default, skip_serializing_if = "is_default")] - extra_behaviors: Vec, + extra_behaviors: Option>, } #[derive(Debug, Default, Clone)] diff --git a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs index 8ba4b85881e9..3298c8071185 100644 --- a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs +++ b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs @@ -102,24 +102,17 @@ impl Rule for NoLabelWithoutControl { fn run(ctx: &RuleContext) -> Self::Signals { let node = ctx.query(); let options = ctx.options(); - let label_attributes = &options.label_attributes; - let label_components = &options.label_components; - let input_components = &options.input_components; - let element_name = node.name()?.name_value_token()?; let element_name = element_name.text_trimmed(); - let is_allowed_element = label_components - .iter() - .any(|label_component_name| label_component_name == element_name) + let is_allowed_element = options.has_element_name(element_name) || DEFAULT_LABEL_COMPONENTS.contains(&element_name); if !is_allowed_element { return None; } - let has_text_content = has_accessible_label(node, label_attributes); - let has_control_association = - has_for_attribute(node) || has_nested_control(node, input_components); + let has_text_content = options.has_accessible_label(node); + let has_control_association = has_for_attribute(node) || options.has_nested_control(node); if has_text_content && has_control_association { return None; @@ -159,14 +152,125 @@ impl Rule for NoLabelWithoutControl { #[derive(Clone, Debug, Default, Deserialize, Deserializable, Eq, PartialEq, Serialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] -#[serde(rename_all = "camelCase", deny_unknown_fields)] +#[serde(rename_all = "camelCase", deny_unknown_fields, default)] pub struct NoLabelWithoutControlOptions { /// Array of component names that should be considered the same as an `input` element. - pub input_components: Vec, + pub input_components: Option>, /// Array of attributes that should be treated as the `label` accessible text content. - pub label_attributes: Vec, + pub label_attributes: Option>, /// Array of component names that should be considered the same as a `label` element. - pub label_components: Vec, + pub label_components: Option>, +} + +impl NoLabelWithoutControlOptions { + /// Returns `true` whether the passed `attribute` meets one of the following conditions: + /// - Has a label attribute that corresponds to the `label_attributes` parameter + /// - Has a label among `DEFAULT_LABEL_ATTRIBUTES` + fn has_label_attribute(&self, attribute: &JsxAttribute) -> bool { + let Ok(attribute_name) = attribute.name().and_then(|name| name.name_token()) else { + return false; + }; + let attribute_name = attribute_name.text_trimmed(); + if !DEFAULT_LABEL_ATTRIBUTES.contains(&attribute_name) + && !self + .label_attributes + .as_ref() + .map(|label_attributes| label_attributes.iter().any(|name| name == attribute_name)) + .unwrap_or_default() + { + return false; + } + attribute + .initializer() + .and_then(|init| init.value().ok()) + .is_some_and(|v| has_label_attribute_value(&v)) + } + + /// Returns `true` whether the passed `jsx_tag` meets one of the following conditions: + /// - Has a label attribute that corresponds to the `label_attributes` parameter + /// - Has a label among `DEFAULT_LABEL_ATTRIBUTES` + /// - Has a child that acts as a label + fn has_accessible_label(&self, jsx_tag: &AnyJsxTag) -> bool { + let mut child_iter = jsx_tag.syntax().preorder(); + while let Some(event) = child_iter.next() { + match event { + WalkEvent::Enter(child) => match child.kind() { + JsSyntaxKind::JSX_EXPRESSION_CHILD + | JsSyntaxKind::JSX_SPREAD_CHILD + | JsSyntaxKind::JSX_TEXT => { + return true; + } + JsSyntaxKind::JSX_ELEMENT + | JsSyntaxKind::JSX_OPENING_ELEMENT + | JsSyntaxKind::JSX_CHILD_LIST + | JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT + | JsSyntaxKind::JSX_ATTRIBUTE_LIST => {} + JsSyntaxKind::JSX_ATTRIBUTE => { + let attribute = JsxAttribute::unwrap_cast(child); + if self.has_label_attribute(&attribute) { + return true; + } + child_iter.skip_subtree(); + } + _ => { + child_iter.skip_subtree(); + } + }, + WalkEvent::Leave(_) => {} + } + } + false + } + + /// Returns whether the passed `AnyJsxTag` have a child that is considered an input component + /// according to the passed `input_components` parameter + fn has_nested_control(&self, jsx_tag: &AnyJsxTag) -> bool { + let mut child_iter = jsx_tag.syntax().preorder(); + while let Some(event) = child_iter.next() { + match event { + WalkEvent::Enter(child) => match child.kind() { + JsSyntaxKind::JSX_ELEMENT + | JsSyntaxKind::JSX_OPENING_ELEMENT + | JsSyntaxKind::JSX_CHILD_LIST + | JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT => {} + _ => { + let Some(element_name) = AnyJsxElementName::cast(child) else { + child_iter.skip_subtree(); + continue; + }; + let Some(element_name) = element_name.name_value_token() else { + continue; + }; + let element_name = element_name.text_trimmed(); + if DEFAULT_INPUT_COMPONENTS.contains(&element_name) + || self + .input_components + .as_ref() + .map(|input_components| { + input_components.iter().any(|name| name == element_name) + }) + .unwrap_or_default() + { + return true; + } + } + }, + WalkEvent::Leave(_) => {} + } + } + false + } + + fn has_element_name(&self, element_name: &str) -> bool { + self.label_components + .as_ref() + .map(|label_components| { + label_components + .iter() + .any(|label_component_name| label_component_name == element_name) + }) + .unwrap_or_default() + } } pub struct NoLabelWithoutControlState { @@ -201,94 +305,6 @@ fn has_for_attribute(jsx_tag: &AnyJsxTag) -> bool { }) } -/// Returns whether the passed `AnyJsxTag` have a child that is considered an input component -/// according to the passed `input_components` parameter -fn has_nested_control(jsx_tag: &AnyJsxTag, input_components: &[String]) -> bool { - let mut child_iter = jsx_tag.syntax().preorder(); - while let Some(event) = child_iter.next() { - match event { - WalkEvent::Enter(child) => match child.kind() { - JsSyntaxKind::JSX_ELEMENT - | JsSyntaxKind::JSX_OPENING_ELEMENT - | JsSyntaxKind::JSX_CHILD_LIST - | JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT => {} - _ => { - let Some(element_name) = AnyJsxElementName::cast(child) else { - child_iter.skip_subtree(); - continue; - }; - let Some(element_name) = element_name.name_value_token() else { - continue; - }; - let element_name = element_name.text_trimmed(); - if DEFAULT_INPUT_COMPONENTS.contains(&element_name) - || input_components.iter().any(|name| name == element_name) - { - return true; - } - } - }, - WalkEvent::Leave(_) => {} - } - } - false -} - -/// Returns `true` whether the passed `jsx_tag` meets one of the following conditions: -/// - Has a label attribute that corresponds to the `label_attributes` parameter -/// - Has a label among `DEFAULT_LABEL_ATTRIBUTES` -/// - Has a child that acts as a label -fn has_accessible_label(jsx_tag: &AnyJsxTag, label_attributes: &[String]) -> bool { - let mut child_iter = jsx_tag.syntax().preorder(); - while let Some(event) = child_iter.next() { - match event { - WalkEvent::Enter(child) => match child.kind() { - JsSyntaxKind::JSX_EXPRESSION_CHILD - | JsSyntaxKind::JSX_SPREAD_CHILD - | JsSyntaxKind::JSX_TEXT => { - return true; - } - JsSyntaxKind::JSX_ELEMENT - | JsSyntaxKind::JSX_OPENING_ELEMENT - | JsSyntaxKind::JSX_CHILD_LIST - | JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT - | JsSyntaxKind::JSX_ATTRIBUTE_LIST => {} - JsSyntaxKind::JSX_ATTRIBUTE => { - let attribute = JsxAttribute::unwrap_cast(child); - if has_label_attribute(&attribute, label_attributes) { - return true; - } - child_iter.skip_subtree(); - } - _ => { - child_iter.skip_subtree(); - } - }, - WalkEvent::Leave(_) => {} - } - } - false -} - -/// Returns `true` whether the passed `attribute` meets one of the following conditions: -/// - Has a label attribute that corresponds to the `label_attributes` parameter -/// - Has a label among `DEFAULT_LABEL_ATTRIBUTES` -fn has_label_attribute(attribute: &JsxAttribute, label_attributes: &[String]) -> bool { - let Ok(attribute_name) = attribute.name().and_then(|name| name.name_token()) else { - return false; - }; - let attribute_name = attribute_name.text_trimmed(); - if !DEFAULT_LABEL_ATTRIBUTES.contains(&attribute_name) - && !label_attributes.iter().any(|name| name == attribute_name) - { - return false; - } - attribute - .initializer() - .and_then(|init| init.value().ok()) - .is_some_and(|v| has_label_attribute_value(&v)) -} - /// Returns whether the passed `jsx_attribute_value` has a valid value inside it fn has_label_attribute_value(jsx_attribute_value: &AnyJsxAttributeValue) -> bool { match jsx_attribute_value { diff --git a/packages/@biomejs/backend-jsonrpc/src/workspace.ts b/packages/@biomejs/backend-jsonrpc/src/workspace.ts index 81fa5a018fc5..0785a7793adb 100644 --- a/packages/@biomejs/backend-jsonrpc/src/workspace.ts +++ b/packages/@biomejs/backend-jsonrpc/src/workspace.ts @@ -2291,15 +2291,15 @@ export interface NoLabelWithoutControlOptions { /** * Array of component names that should be considered the same as an `input` element. */ - inputComponents: string[]; + inputComponents?: string[]; /** * Array of attributes that should be treated as the `label` accessible text content. */ - labelAttributes: string[]; + labelAttributes?: string[]; /** * Array of component names that should be considered the same as a `label` element. */ - labelComponents: string[]; + labelComponents?: string[]; } export interface ValidAriaRoleOptions { allowInvalidRoles: string[]; diff --git a/packages/@biomejs/biome/configuration_schema.json b/packages/@biomejs/biome/configuration_schema.json index b45098e9db28..f486de733280 100644 --- a/packages/@biomejs/biome/configuration_schema.json +++ b/packages/@biomejs/biome/configuration_schema.json @@ -2029,21 +2029,23 @@ }, "NoLabelWithoutControlOptions": { "type": "object", - "required": ["inputComponents", "labelAttributes", "labelComponents"], "properties": { "inputComponents": { "description": "Array of component names that should be considered the same as an `input` element.", - "type": "array", + "default": null, + "type": ["array", "null"], "items": { "type": "string" } }, "labelAttributes": { "description": "Array of attributes that should be treated as the `label` accessible text content.", - "type": "array", + "default": null, + "type": ["array", "null"], "items": { "type": "string" } }, "labelComponents": { "description": "Array of component names that should be considered the same as a `label` element.", - "type": "array", + "default": null, + "type": ["array", "null"], "items": { "type": "string" } } }, From 640a72804a2fdc82769b820078341989f4b02c44 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 30 Sep 2024 13:13:15 +0100 Subject: [PATCH 2/3] clippy --- .../src/lint/a11y/no_label_without_control.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs index 3298c8071185..0a6c4bf9753d 100644 --- a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs +++ b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs @@ -175,8 +175,9 @@ impl NoLabelWithoutControlOptions { && !self .label_attributes .as_ref() - .map(|label_attributes| label_attributes.iter().any(|name| name == attribute_name)) - .unwrap_or_default() + .is_some_and(|label_attributes| { + label_attributes.iter().any(|name| name == attribute_name) + }) { return false; } @@ -246,10 +247,9 @@ impl NoLabelWithoutControlOptions { || self .input_components .as_ref() - .map(|input_components| { + .is_some_and(|input_components| { input_components.iter().any(|name| name == element_name) }) - .unwrap_or_default() { return true; } @@ -264,12 +264,11 @@ impl NoLabelWithoutControlOptions { fn has_element_name(&self, element_name: &str) -> bool { self.label_components .as_ref() - .map(|label_components| { + .is_some_and(|label_components| { label_components .iter() .any(|label_component_name| label_component_name == element_name) }) - .unwrap_or_default() } } From 01de10b25fba906dedbe20bc5e54daf45acc4cf3 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 1 Oct 2024 08:50:30 +0100 Subject: [PATCH 3/3] apply feedback --- crates/biome_analyze/CONTRIBUTING.md | 20 +++++++++----- .../src/lint/a11y/no_label_without_control.rs | 26 +++++++------------ .../@biomejs/biome/configuration_schema.json | 12 ++++----- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/crates/biome_analyze/CONTRIBUTING.md b/crates/biome_analyze/CONTRIBUTING.md index 11c9308c0782..4635735b98c2 100644 --- a/crates/biome_analyze/CONTRIBUTING.md +++ b/crates/biome_analyze/CONTRIBUTING.md @@ -161,16 +161,16 @@ We would like to set the options in the `biome.json` configuration file: } ``` -The first step is to create the Rust data representation of the rule's options. Each option must be wrapped in a `Option`, this is required so the configuration schema won't mark them as required. +The first step is to create the Rust data representation of the rule's options. ```rust use biome_deserialize_macros::Deserializable; #[derive(Clone, Debug, Default, Deserializable)] pub struct MyRuleOptions { - behavior: Option, - threshold: Option, - behavior_exceptions: Option> + behavior: Behavior, + threshold: u8, + behavior_exceptions: Vec } #[derive(Clone, Debug, Default, Deserializable)] @@ -207,18 +207,24 @@ let options = ctx.options(); The compiler should warn you that `MyRuleOptions` does not implement some required types. We currently require implementing _serde_'s traits `Deserialize`/`Serialize`. + +Also, we use other `serde` macros to adjust the JSON configuration: +- `rename_all = "camelCase"`: it renames all fields in camel-case, so they are in line with the naming style of the `biome.json`. +- `deny_unknown_fields`: it raises an error if the configuration contains extraneous fields. +- `default`: it uses the `Default` value when the field is missing from `biome.json`. This macro makes the field optional. + You can simply use a derive macros: ```rust #[derive(Debug, Default, Clone, Serialize, Deserialize)] #[cfg_attr(feature = "schemars", derive(JsonSchema))] -#[serde(rename_all = "camelCase", deny_unknown_fields)] +#[serde(rename_all = "camelCase", deny_unknown_fields, default)] pub struct MyRuleOptions { #[serde(default, skip_serializing_if = "is_default")] - main_behavior: Option, + main_behavior: Behavior, #[serde(default, skip_serializing_if = "is_default")] - extra_behaviors: Option>, + extra_behaviors: Vec, } #[derive(Debug, Default, Clone)] diff --git a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs index 0a6c4bf9753d..39fe4c431c8d 100644 --- a/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs +++ b/crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs @@ -155,11 +155,11 @@ impl Rule for NoLabelWithoutControl { #[serde(rename_all = "camelCase", deny_unknown_fields, default)] pub struct NoLabelWithoutControlOptions { /// Array of component names that should be considered the same as an `input` element. - pub input_components: Option>, + pub input_components: Vec, /// Array of attributes that should be treated as the `label` accessible text content. - pub label_attributes: Option>, + pub label_attributes: Vec, /// Array of component names that should be considered the same as a `label` element. - pub label_components: Option>, + pub label_components: Vec, } impl NoLabelWithoutControlOptions { @@ -174,10 +174,8 @@ impl NoLabelWithoutControlOptions { if !DEFAULT_LABEL_ATTRIBUTES.contains(&attribute_name) && !self .label_attributes - .as_ref() - .is_some_and(|label_attributes| { - label_attributes.iter().any(|name| name == attribute_name) - }) + .iter() + .any(|name| name == attribute_name) { return false; } @@ -246,10 +244,8 @@ impl NoLabelWithoutControlOptions { if DEFAULT_INPUT_COMPONENTS.contains(&element_name) || self .input_components - .as_ref() - .is_some_and(|input_components| { - input_components.iter().any(|name| name == element_name) - }) + .iter() + .any(|name| name == element_name) { return true; } @@ -263,12 +259,8 @@ impl NoLabelWithoutControlOptions { fn has_element_name(&self, element_name: &str) -> bool { self.label_components - .as_ref() - .is_some_and(|label_components| { - label_components - .iter() - .any(|label_component_name| label_component_name == element_name) - }) + .iter() + .any(|label_component_name| label_component_name == element_name) } } diff --git a/packages/@biomejs/biome/configuration_schema.json b/packages/@biomejs/biome/configuration_schema.json index f486de733280..ede54dedd1da 100644 --- a/packages/@biomejs/biome/configuration_schema.json +++ b/packages/@biomejs/biome/configuration_schema.json @@ -2032,20 +2032,20 @@ "properties": { "inputComponents": { "description": "Array of component names that should be considered the same as an `input` element.", - "default": null, - "type": ["array", "null"], + "default": [], + "type": "array", "items": { "type": "string" } }, "labelAttributes": { "description": "Array of attributes that should be treated as the `label` accessible text content.", - "default": null, - "type": ["array", "null"], + "default": [], + "type": "array", "items": { "type": "string" } }, "labelComponents": { "description": "Array of component names that should be considered the same as a `label` element.", - "default": null, - "type": ["array", "null"], + "default": [], + "type": "array", "items": { "type": "string" } } },