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.

Sign up for GitHub

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

refactor(linter): clean up jsx-a11y/anchor-is-valid #4831

Merged
merged 1 commit into from
Aug 12, 2024
Merged

refactor(linter): clean up jsx-a11y/anchor-is-valid #4831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 60 additions & 0 deletions crates/oxc_ast/src/ast_impl/jsx.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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