From 096ac7bee51c7be55457d56fc3d7b6b379186a3f Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Mon, 12 Aug 2024 01:11:54 +0000 Subject: [PATCH] refactor(linter): clean up jsx-a11y/anchor-is-valid (#4831) --- crates/oxc_ast/src/ast_impl/jsx.rs | 60 +++++++ .../src/rules/jsx_a11y/anchor_is_valid.rs | 155 ++++++++++-------- crates/oxc_linter/src/utils/react.rs | 58 ++----- crates/oxc_span/src/atom.rs | 2 +- 4 files changed, 160 insertions(+), 115 deletions(-) diff --git a/crates/oxc_ast/src/ast_impl/jsx.rs b/crates/oxc_ast/src/ast_impl/jsx.rs index aed7c74ef2a1d..3ced6b276f7b2 100644 --- a/crates/oxc_ast/src/ast_impl/jsx.rs +++ b/crates/oxc_ast/src/ast_impl/jsx.rs @@ -24,6 +24,15 @@ impl<'a> fmt::Display for JSXNamespacedName<'a> { } } +impl<'a> JSXElementName<'a> { + pub fn as_identifier(&self) -> Option<&JSXIdentifier<'a>> { + match self { + Self::Identifier(id) => Some(id.as_ref()), + _ => None, + } + } +} + impl<'a> JSXMemberExpression<'a> { pub fn get_object_identifier(&self) -> &JSXIdentifier { let mut member_expr = self; @@ -91,11 +100,62 @@ impl<'a> JSXAttribute<'a> { matches!(&self.name, JSXAttributeName::Identifier(ident) if ident.name == name) } + pub fn is_identifier_ignore_case(&self, name: &str) -> bool { + matches!(&self.name, JSXAttributeName::Identifier(ident) if ident.name.eq_ignore_ascii_case(name)) + } + pub fn is_key(&self) -> bool { self.is_identifier("key") } } +impl<'a> JSXAttributeName<'a> { + pub fn as_identifier(&self) -> Option<&JSXIdentifier<'a>> { + match self { + Self::Identifier(ident) => Some(ident.as_ref()), + Self::NamespacedName(_) => None, + } + } + pub fn get_identifier(&self) -> &JSXIdentifier<'a> { + match self { + Self::Identifier(ident) => ident.as_ref(), + Self::NamespacedName(namespaced) => &namespaced.property, + } + } +} +impl<'a> JSXAttributeValue<'a> { + pub fn as_string_literal(&self) -> Option<&StringLiteral<'a>> { + match self { + Self::StringLiteral(lit) => Some(lit.as_ref()), + _ => None, + } + } +} + +impl<'a> JSXAttributeItem<'a> { + /// Get the contained [`JSXAttribute`] if it is an attribute item, otherwise + /// returns [`None`]. + /// + /// This is the inverse of [`JSXAttributeItem::as_spread`]. + pub fn as_attribute(&self) -> Option<&JSXAttribute<'a>> { + match self { + Self::Attribute(attr) => Some(attr), + Self::SpreadAttribute(_) => None, + } + } + + /// Get the contained [`JSXSpreadAttribute`] if it is a spread attribute item, + /// otherwise returns [`None`]. + /// + /// This is the inverse of [`JSXAttributeItem::as_attribute`]. + pub fn as_spread(&self) -> Option<&JSXSpreadAttribute<'a>> { + match self { + Self::Attribute(_) => None, + Self::SpreadAttribute(spread) => Some(spread), + } + } +} + impl<'a> JSXChild<'a> { pub const fn is_expression_container(&self) -> bool { matches!(self, Self::ExpressionContainer(_)) diff --git a/crates/oxc_linter/src/rules/jsx_a11y/anchor_is_valid.rs b/crates/oxc_linter/src/rules/jsx_a11y/anchor_is_valid.rs index bad859069fb99..a5ff81774b385 100644 --- a/crates/oxc_linter/src/rules/jsx_a11y/anchor_is_valid.rs +++ b/crates/oxc_linter/src/rules/jsx_a11y/anchor_is_valid.rs @@ -1,10 +1,13 @@ +use std::ops::Deref; + use oxc_ast::{ ast::{JSXAttributeItem, JSXAttributeValue, JSXElementName, JSXExpression}, AstKind, }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{CompactStr, Span}; +use serde_json::Value; use crate::{ context::LintContext, @@ -35,8 +38,17 @@ fn cant_be_anchor(span0: Span) -> OxcDiagnostic { pub struct AnchorIsValid(Box); #[derive(Debug, Default, Clone)] -struct AnchorIsValidConfig { - valid_hrefs: Vec, +pub struct AnchorIsValidConfig { + /// Unique and sorted list of valid hrefs + valid_hrefs: Vec, +} + +impl Deref for AnchorIsValid { + type Target = AnchorIsValidConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } } declare_oxc_lint!( @@ -109,11 +121,10 @@ declare_oxc_lint!( impl Rule for AnchorIsValid { fn from_configuration(value: serde_json::Value) -> Self { - let valid_hrefs = - value.get("validHrefs").and_then(|v| v.as_array()).map_or_else(Vec::new, |array| { - array.iter().filter_map(|v| v.as_str().map(String::from)).collect::>() - }); - Self(Box::new(AnchorIsValidConfig { valid_hrefs })) + let Some(valid_hrefs) = value.get("validHrefs").and_then(Value::as_array) else { + return Self::default(); + }; + Self(Box::new(valid_hrefs.iter().collect())) } fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { @@ -124,76 +135,82 @@ impl Rule for AnchorIsValid { let Some(name) = &get_element_type(ctx, &jsx_el.opening_element) else { return; }; - if name == "a" { - if let Option::Some(herf_attr) = - has_jsx_prop_ignore_case(&jsx_el.opening_element, "href") - { - // Check if the 'a' element has a correct href attribute - match herf_attr { - JSXAttributeItem::Attribute(attr) => match &attr.value { - Some(value) => { - let is_empty = check_value_is_empty(value, &self.0.valid_hrefs); - if is_empty { - if has_jsx_prop_ignore_case(&jsx_el.opening_element, "onclick") - .is_some() - { - ctx.diagnostic(cant_be_anchor(ident.span)); - return; - } - ctx.diagnostic(incorrect_href(ident.span)); - return; - } - } - None => { - ctx.diagnostic(incorrect_href(ident.span)); - return; - } - }, - JSXAttributeItem::SpreadAttribute(_) => { - // pass - return; - } - } + if name != "a" { + return; + }; + if let Option::Some(href_attr) = + has_jsx_prop_ignore_case(&jsx_el.opening_element, "href") + { + let JSXAttributeItem::Attribute(attr) = href_attr else { return; - } - // Exclude '' case - let has_spreed_attr = - jsx_el.opening_element.attributes.iter().any(|attr| match attr { - JSXAttributeItem::SpreadAttribute(_) => true, - JSXAttributeItem::Attribute(_) => false, - }); - if has_spreed_attr { + }; + + // Check if the 'a' element has a correct href attribute + let Some(value) = attr.value.as_ref() else { + ctx.diagnostic(incorrect_href(ident.span)); + return; + }; + + let is_empty = self.check_value_is_empty(value); + if is_empty { + if has_jsx_prop_ignore_case(&jsx_el.opening_element, "onclick").is_some() { + ctx.diagnostic(cant_be_anchor(ident.span)); + return; + } + ctx.diagnostic(incorrect_href(ident.span)); return; } - ctx.diagnostic(missing_href_attribute(ident.span)); + return; } + // Exclude '' case + let has_spread_attr = jsx_el.opening_element.attributes.iter().any(|attr| match attr { + JSXAttributeItem::SpreadAttribute(_) => true, + JSXAttributeItem::Attribute(_) => false, + }); + if has_spread_attr { + return; + } + ctx.diagnostic(missing_href_attribute(ident.span)); } } } -fn check_value_is_empty(value: &JSXAttributeValue, valid_hrefs: &[String]) -> bool { - match value { - JSXAttributeValue::Element(_) => false, - JSXAttributeValue::StringLiteral(str_lit) => { - let href_value = str_lit.value.to_string(); // Assuming Atom implements ToString - href_value.is_empty() - || href_value == "#" - || href_value == "javascript:void(0)" - || !valid_hrefs.contains(&href_value) +impl AnchorIsValid { + fn check_value_is_empty(&self, value: &JSXAttributeValue) -> bool { + match value { + JSXAttributeValue::Element(_) => false, + JSXAttributeValue::StringLiteral(str_lit) => self.is_invalid_href(&str_lit.value), + JSXAttributeValue::ExpressionContainer(exp) => match &exp.expression { + JSXExpression::Identifier(ident) => ident.name == "undefined", + JSXExpression::NullLiteral(_) => true, + JSXExpression::StringLiteral(str_lit) => self.is_invalid_href(&str_lit.value), + _ => false, + }, + JSXAttributeValue::Fragment(_) => true, } - JSXAttributeValue::ExpressionContainer(exp) => match &exp.expression { - JSXExpression::Identifier(ident) => ident.name == "undefined", - JSXExpression::NullLiteral(_) => true, - JSXExpression::StringLiteral(str_lit) => { - let href_value = str_lit.value.to_string(); - href_value.is_empty() - || href_value == "#" - || href_value == "javascript:void(0)" - || !valid_hrefs.contains(&href_value) - } - _ => false, - }, - JSXAttributeValue::Fragment(_) => true, + } +} + +impl AnchorIsValidConfig { + fn new(mut valid_hrefs: Vec) -> Self { + valid_hrefs.sort_unstable(); + valid_hrefs.dedup(); + Self { valid_hrefs } + } + + fn is_invalid_href(&self, href: &str) -> bool { + href.is_empty() || href == "#" || href == "javascript:void(0)" || !self.contains(href) + } + + fn contains(&self, href: &str) -> bool { + self.valid_hrefs.binary_search_by(|valid_href| valid_href.as_str().cmp(href)).is_ok() + } +} + +impl<'v> FromIterator<&'v Value> for AnchorIsValidConfig { + fn from_iter>(iter: T) -> Self { + let hrefs = iter.into_iter().filter_map(Value::as_str).map(CompactStr::from).collect(); + Self::new(hrefs) } } diff --git a/crates/oxc_linter/src/utils/react.rs b/crates/oxc_linter/src/utils/react.rs index 734dad13988ee..5b0ccf990e372 100644 --- a/crates/oxc_linter/src/utils/react.rs +++ b/crates/oxc_linter/src/utils/react.rs @@ -3,7 +3,7 @@ use std::borrow::Cow; use oxc_ast::{ ast::{ CallExpression, Expression, JSXAttributeItem, JSXAttributeName, JSXAttributeValue, - JSXChild, JSXElement, JSXElementName, JSXExpression, JSXOpeningElement, MemberExpression, + JSXChild, JSXElement, JSXExpression, JSXOpeningElement, MemberExpression, }, match_member_expression, AstKind, }; @@ -28,40 +28,22 @@ pub fn has_jsx_prop<'a, 'b>( node: &'b JSXOpeningElement<'a>, target_prop: &'b str, ) -> Option<&'b JSXAttributeItem<'a>> { - node.attributes.iter().find(|attr| match attr { - JSXAttributeItem::SpreadAttribute(_) => false, - JSXAttributeItem::Attribute(attr) => { - let JSXAttributeName::Identifier(name) = &attr.name else { - return false; - }; - - name.name == target_prop - } - }) + node.attributes + .iter() + .find(|attr| attr.as_attribute().is_some_and(|attr| attr.is_identifier(target_prop))) } pub fn has_jsx_prop_ignore_case<'a, 'b>( node: &'b JSXOpeningElement<'a>, target_prop: &'b str, ) -> Option<&'b JSXAttributeItem<'a>> { - node.attributes.iter().find(|attr| match attr { - JSXAttributeItem::SpreadAttribute(_) => false, - JSXAttributeItem::Attribute(attr) => { - let JSXAttributeName::Identifier(name) = &attr.name else { - return false; - }; - - name.name.eq_ignore_ascii_case(target_prop) - } + node.attributes.iter().find(|attr| { + attr.as_attribute().is_some_and(|attr| attr.is_identifier_ignore_case(target_prop)) }) } pub fn get_prop_value<'a, 'b>(item: &'b JSXAttributeItem<'a>) -> Option<&'b JSXAttributeValue<'a>> { - if let JSXAttributeItem::Attribute(attr) = item { - attr.value.as_ref() - } else { - None - } + item.as_attribute().and_then(|item| item.value.as_ref()) } pub fn get_jsx_attribute_name<'a>(attr: &JSXAttributeName<'a>) -> Cow<'a, str> { @@ -74,13 +56,7 @@ pub fn get_jsx_attribute_name<'a>(attr: &JSXAttributeName<'a>) -> Cow<'a, str> { } pub fn get_string_literal_prop_value<'a>(item: &'a JSXAttributeItem<'_>) -> Option<&'a str> { - get_prop_value(item).and_then(|v| { - if let JSXAttributeValue::StringLiteral(s) = v { - Some(s.value.as_str()) - } else { - None - } - }) + get_prop_value(item).and_then(JSXAttributeValue::as_string_literal).map(|s| s.value.as_str()) } // ref: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/src/util/isHiddenFromScreenReader.js @@ -132,11 +108,8 @@ pub fn is_presentation_role(jsx_opening_el: &JSXOpeningElement) -> bool { let Some(role) = has_jsx_prop(jsx_opening_el, "role") else { return false; }; - let Some("presentation" | "none") = get_string_literal_prop_value(role) else { - return false; - }; - true + matches!(get_string_literal_prop_value(role), Some("presentation" | "none")) } // TODO: Should re-implement @@ -246,9 +219,7 @@ pub fn get_element_type<'c, 'a>( context: &'c LintContext<'a>, element: &JSXOpeningElement<'a>, ) -> Option> { - let JSXElementName::Identifier(ident) = &element.name else { - return None; - }; + let name = element.name.as_identifier()?; let OxlintSettings { jsx_a11y, .. } = context.settings(); @@ -259,17 +230,14 @@ pub fn get_element_type<'c, 'a>( has_jsx_prop_ignore_case(element, polymorphic_prop_name_value) }) .and_then(get_prop_value) - .and_then(|prop_value| match prop_value { - JSXAttributeValue::StringLiteral(str) => Some(str.value.as_str()), - _ => None, - }); + .and_then(JSXAttributeValue::as_string_literal) + .map(|s| s.value.as_str()); - let raw_type = polymorphic_prop.unwrap_or_else(|| ident.name.as_str()); + let raw_type = polymorphic_prop.unwrap_or_else(|| name.name.as_str()); match jsx_a11y.components.get(raw_type) { Some(component) => Some(Cow::Borrowed(component)), None => Some(Cow::Borrowed(raw_type)), } - // Some(String::from(jsx_a11y.components.get(raw_type).map_or(raw_type, |c| c))) } pub fn parse_jsx_value(value: &JSXAttributeValue) -> Result { diff --git a/crates/oxc_span/src/atom.rs b/crates/oxc_span/src/atom.rs index 0d3ff890b4c75..c1bcee7036169 100644 --- a/crates/oxc_span/src/atom.rs +++ b/crates/oxc_span/src/atom.rs @@ -198,7 +198,7 @@ impl<'a> fmt::Display for Atom<'a> { /// /// Currently implemented as just a wrapper around [`compact_str::CompactString`], /// but will be reduced in size with a custom implementation later. -#[derive(Clone, Eq)] +#[derive(Clone, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serialize", derive(serde::Deserialize))] pub struct CompactStr(CompactString);