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

fix(lint): options for noLabelWithoutControl are optional #4138

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 were 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
Expand Down
12 changes: 6 additions & 6 deletions crates/biome_analyze/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@arendjr arendjr Sep 30, 2024

Choose a reason for hiding this comment

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

And then I see this as motivation for the Option wrappers :) Could we make the configuration schema respect the default annotations in addition to only Option?


```rust
use biome_deserialize_macros::Deserializable;

#[derive(Clone, Debug, Default, Deserializable)]
pub struct MyRuleOptions {
behavior: Behavior,
threshold: u8,
behavior_exceptions: Vec<String>
behavior: Option<Behavior>,
threshold: Option<u8>,
behavior_exceptions: Option<Vec<String>>
}

#[derive(Clone, Debug, Default, Deserializable)]
Expand Down Expand Up @@ -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<Behavior>,

#[serde(default, skip_serializing_if = "is_default")]
extra_behaviors: Vec<Behavior>,
extra_behaviors: Option<Vec<Behavior>>,
}

#[derive(Debug, Default, Clone)]
Expand Down
219 changes: 117 additions & 102 deletions crates/biome_js_analyze/src/lint/a11y/no_label_without_control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,24 +102,17 @@ impl Rule for NoLabelWithoutControl {
fn run(ctx: &RuleContext<Self>) -> 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;
Expand Down Expand Up @@ -159,14 +152,124 @@ 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)]
Copy link
Member

@Conaclos Conaclos Sep 30, 2024

Choose a reason for hiding this comment

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

Using the serde(default) attribute should be enough to make optional the fields.
Is there an interest to have both Option<> and serde(default)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed it might make more sense to use plain Vecs only. It uses less memory, and makes consuming code easier. The only exception should be when an empty vector and None are semantically different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try the serde(default), but the issue is the schema, not the default values.

Copy link
Member

Choose a reason for hiding this comment

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

If i am not mistaken, schemars recognizes serde(default)

pub struct NoLabelWithoutControlOptions {
/// Array of component names that should be considered the same as an `input` element.
pub input_components: Vec<String>,
pub input_components: Option<Vec<String>>,
/// Array of attributes that should be treated as the `label` accessible text content.
pub label_attributes: Vec<String>,
pub label_attributes: Option<Vec<String>>,
/// Array of component names that should be considered the same as a `label` element.
pub label_components: Vec<String>,
pub label_components: Option<Vec<String>>,
}

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()
.is_some_and(|label_attributes| {
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 `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()
.is_some_and(|input_components| {
input_components.iter().any(|name| name == element_name)
})
{
return true;
}
}
},
WalkEvent::Leave(_) => {}
}
}
false
}

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)
})
}
}

pub struct NoLabelWithoutControlState {
Expand Down Expand Up @@ -201,94 +304,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 {
Expand Down
6 changes: 3 additions & 3 deletions packages/@biomejs/backend-jsonrpc/src/workspace.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions packages/@biomejs/biome/configuration_schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.