Skip to content

Commit

Permalink
refactor(linter): clean up jsx-a11y/anchor-is-valid (#4831)
Browse files Browse the repository at this point in the history
  • Loading branch information
@DonIsaac
DonIsaac committed Aug 12, 2024
1 parent 8827659 commit 096ac7b
Show file tree
Hide file tree
Showing 4 changed files with 160 additions and 115 deletions.
60 changes: 60 additions & 0 deletions crates/oxc_ast/src/ast_impl/jsx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(_))
Expand Down
155 changes: 86 additions & 69 deletions crates/oxc_linter/src/rules/jsx_a11y/anchor_is_valid.rs
View file
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -35,8 +38,17 @@ fn cant_be_anchor(span0: Span) -> OxcDiagnostic {
pub struct AnchorIsValid(Box<AnchorIsValidConfig>);

#[derive(Debug, Default, Clone)]
struct AnchorIsValidConfig {
valid_hrefs: Vec<String>,
pub struct AnchorIsValidConfig {
/// Unique and sorted list of valid hrefs
valid_hrefs: Vec<CompactStr>,
}

impl Deref for AnchorIsValid {
type Target = AnchorIsValidConfig;

fn deref(&self) -> &Self::Target {
&self.0
}
}

declare_oxc_lint!(
Expand Down Expand Up @@ -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::<Vec<String>>()
});
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>) {
Expand All @@ -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 '<a {...props} />' 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 '<a {...props} />' 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<CompactStr>) -> 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<T: IntoIterator<Item = &'v Value>>(iter: T) -> Self {
let hrefs = iter.into_iter().filter_map(Value::as_str).map(CompactStr::from).collect();
Self::new(hrefs)
}
}

Expand Down
58 changes: 13 additions & 45 deletions crates/oxc_linter/src/utils/react.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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> {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -246,9 +219,7 @@ pub fn get_element_type<'c, 'a>(
context: &'c LintContext<'a>,
element: &JSXOpeningElement<'a>,
) -> Option<Cow<'c, str>> {
let JSXElementName::Identifier(ident) = &element.name else {
return None;
};
let name = element.name.as_identifier()?;

let OxlintSettings { jsx_a11y, .. } = context.settings();

Expand All @@ -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<f64, ()> {
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_span/src/atom.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit 096ac7b

Please sign in to comment.