This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_analyze): lint/correctness/noDupeKeys #3562
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
a933e70
WIP feat(rome_js_analyze): lint/correctness/noDupeKeys
jeysal 577aed0
multi failure test
jeysal d4f1dbd
ci fixes
jeysal 9faaee0
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal b1bad71
fix diagnostic copy
jeysal 278e0af
fix obj member removal code action and thus fix snap test
jeysal cfb93ce
update rule release version
jeysal 880e50e
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal 5b08f04
consistent diagnostics: always only highlight the last overwriting pr…
jeysal 4d6b1cc
codegen
jeysal 83702b5
thanks clippy!
jeysal 9fffd25
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal 9df697f
Update crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs
jeysal 2393996
Micha suggestion
jeysal 4e95ccf
Another Micha suggestion
jeysal 034e0fc
Merge remote-tracking branch 'origin/no-dupe-keys' into no-dupe-keys
jeysal 1fc7282
codegen
jeysal 62c5af4
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal fed78da
micha feedback refactors
jeysal 725d684
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal 09adebb
codegen
jeysal 1b0f6fa
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
jeysal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
311 changes: 311 additions & 0 deletions
311
crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,311 @@ | ||
use crate::utils::batch::JsBatchMutation; | ||
use rome_analyze::context::RuleContext; | ||
use rome_analyze::{declare_rule, Ast, Rule, RuleDiagnostic}; | ||
use rome_console::markup; | ||
use rome_js_syntax::{ | ||
JsAnyObjectMember, JsGetterObjectMember, JsObjectExpression, JsSetterObjectMember, | ||
}; | ||
use rome_js_syntax::{ | ||
JsMethodObjectMember, JsPropertyObjectMember, JsShorthandPropertyObjectMember, TextRange, | ||
}; | ||
use rome_rowan::{AstNode, BatchMutationExt}; | ||
use std::cmp::Ordering; | ||
use std::collections::HashMap; | ||
use std::fmt::Display; | ||
|
||
use crate::JsRuleAction; | ||
|
||
declare_rule! { | ||
/// Prevents object literals having more than one property declaration for the same name. | ||
/// If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored, which is likely a mistake. | ||
/// | ||
/// ## Examples | ||
/// | ||
/// ### Invalid | ||
/// | ||
/// ```js,expect_diagnostic | ||
/// const obj = { | ||
/// a: 1, | ||
/// a: 2, | ||
/// } | ||
/// ``` | ||
/// | ||
/// ```js,expect_diagnostic | ||
/// const obj = { | ||
/// set a(v) {}, | ||
/// a: 2, | ||
/// } | ||
/// ``` | ||
/// | ||
/// ### Valid | ||
/// | ||
/// ```js | ||
/// const obj = { | ||
/// a: 1, | ||
/// b: 2, | ||
/// } | ||
/// ``` | ||
/// | ||
/// ```js | ||
/// const obj = { | ||
/// get a() { return 1; }, | ||
/// set a(v) {}, | ||
/// } | ||
/// ``` | ||
/// | ||
pub(crate) NoDupeKeys { | ||
version: "11.0.0", | ||
name: "noDupeKeys", | ||
recommended: false, // should be once out of nursery | ||
} | ||
} | ||
|
||
/// An object member defining a single object property. | ||
enum MemberDefinition { | ||
Getter(JsGetterObjectMember), | ||
Setter(JsSetterObjectMember), | ||
Method(JsMethodObjectMember), | ||
Property(JsPropertyObjectMember), | ||
ShorthandProperty(JsShorthandPropertyObjectMember), | ||
} | ||
impl MemberDefinition { | ||
fn name(&self) -> Option<String> { | ||
match self { | ||
MemberDefinition::Getter(getter) => { | ||
getter.name().ok()?.as_js_literal_member_name()?.name().ok() | ||
} | ||
MemberDefinition::Setter(setter) => { | ||
setter.name().ok()?.as_js_literal_member_name()?.name().ok() | ||
} | ||
MemberDefinition::Method(method) => { | ||
method.name().ok()?.as_js_literal_member_name()?.name().ok() | ||
} | ||
MemberDefinition::Property(property) => property | ||
.name() | ||
.ok()? | ||
.as_js_literal_member_name()? | ||
.name() | ||
.ok(), | ||
MemberDefinition::ShorthandProperty(shorthand_property) => { | ||
Some(shorthand_property.name().ok()?.text()) | ||
} | ||
} | ||
} | ||
|
||
fn range(&self) -> TextRange { | ||
match self { | ||
MemberDefinition::Getter(getter) => getter.range(), | ||
MemberDefinition::Setter(setter) => setter.range(), | ||
MemberDefinition::Method(method) => method.range(), | ||
MemberDefinition::Property(property) => property.range(), | ||
MemberDefinition::ShorthandProperty(shorthand_property) => shorthand_property.range(), | ||
} | ||
} | ||
|
||
fn node(&self) -> JsAnyObjectMember { | ||
match self { | ||
MemberDefinition::Getter(getter) => JsAnyObjectMember::from(getter.clone()), | ||
MemberDefinition::Setter(setter) => JsAnyObjectMember::from(setter.clone()), | ||
MemberDefinition::Method(method) => JsAnyObjectMember::from(method.clone()), | ||
MemberDefinition::Property(property) => JsAnyObjectMember::from(property.clone()), | ||
MemberDefinition::ShorthandProperty(shorthand_property) => { | ||
JsAnyObjectMember::from(shorthand_property.clone()) | ||
} | ||
} | ||
} | ||
} | ||
impl Display for MemberDefinition { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.write_str(match self { | ||
Self::Getter(_) => "getter", | ||
Self::Setter(_) => "setter", | ||
Self::Method(_) => "method", | ||
Self::Property(_) => "property value", | ||
Self::ShorthandProperty(_) => "shorthand property", | ||
})?; | ||
if let Some(name) = self.name() { | ||
f.write_str(" named ")?; | ||
f.write_str(&name)?; | ||
} | ||
Ok(()) | ||
} | ||
} | ||
enum MemberDefinitionError { | ||
NotASinglePropertyMember, | ||
UnknownMemberType, | ||
} | ||
impl TryFrom<JsAnyObjectMember> for MemberDefinition { | ||
type Error = MemberDefinitionError; | ||
|
||
fn try_from(member: JsAnyObjectMember) -> Result<Self, Self::Error> { | ||
match member { | ||
JsAnyObjectMember::JsGetterObjectMember(member) => Ok(MemberDefinition::Getter(member)), | ||
JsAnyObjectMember::JsSetterObjectMember(member) => Ok(MemberDefinition::Setter(member)), | ||
JsAnyObjectMember::JsMethodObjectMember(member) => Ok(MemberDefinition::Method(member)), | ||
JsAnyObjectMember::JsPropertyObjectMember(member) => { | ||
Ok(MemberDefinition::Property(member)) | ||
} | ||
JsAnyObjectMember::JsShorthandPropertyObjectMember(member) => { | ||
Ok(MemberDefinition::ShorthandProperty(member)) | ||
} | ||
JsAnyObjectMember::JsSpread(_) => Err(MemberDefinitionError::NotASinglePropertyMember), | ||
JsAnyObjectMember::JsUnknownMember(_) => Err(MemberDefinitionError::UnknownMemberType), | ||
} | ||
} | ||
} | ||
|
||
/// A descriptor for a property that is, as far as we can tell from statically analyzing the object expression, | ||
/// not overwritten by another object member and will make it into the object. | ||
#[derive(Clone)] | ||
enum DefinedProperty { | ||
Get(TextRange), | ||
Set(TextRange), | ||
GetSet(TextRange, TextRange), | ||
Value(TextRange), | ||
} | ||
impl From<MemberDefinition> for DefinedProperty { | ||
fn from(definition: MemberDefinition) -> Self { | ||
match definition { | ||
MemberDefinition::Getter(getter) => DefinedProperty::Get(getter.range()), | ||
MemberDefinition::Setter(setter) => DefinedProperty::Set(setter.range()), | ||
MemberDefinition::Method(method) => DefinedProperty::Value(method.range()), | ||
MemberDefinition::Property(property) => DefinedProperty::Value(property.range()), | ||
MemberDefinition::ShorthandProperty(shorthand_property) => { | ||
DefinedProperty::Value(shorthand_property.range()) | ||
} | ||
} | ||
} | ||
} | ||
|
||
pub(crate) struct PropertyConflict(DefinedProperty, MemberDefinition); | ||
impl DefinedProperty { | ||
fn extend_with( | ||
&self, | ||
member_definition: MemberDefinition, | ||
) -> Result<DefinedProperty, PropertyConflict> { | ||
match (self, member_definition) { | ||
// Add missing get/set counterpart | ||
(DefinedProperty::Set(set_range), MemberDefinition::Getter(getter)) => { | ||
Ok(DefinedProperty::GetSet(getter.range(), *set_range)) | ||
} | ||
|
||
(DefinedProperty::Get(get_range), MemberDefinition::Setter(setter)) => { | ||
Ok(DefinedProperty::GetSet(*get_range, setter.range())) | ||
} | ||
// Else conflict | ||
(defined_property, member_definition) => Err(PropertyConflict( | ||
defined_property.clone(), | ||
member_definition, | ||
)), | ||
} | ||
} | ||
} | ||
|
||
impl Rule for NoDupeKeys { | ||
type Query = Ast<JsObjectExpression>; | ||
type State = PropertyConflict; | ||
type Signals = Vec<Self::State>; | ||
type Options = (); | ||
|
||
fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||
let node = ctx.query(); | ||
|
||
let mut defined_properties: HashMap<String, DefinedProperty> = HashMap::new(); | ||
let mut signals: Self::Signals = Vec::new(); | ||
|
||
for member_definition in node | ||
.members() | ||
.into_iter() | ||
.flatten() | ||
.filter_map(|member| MemberDefinition::try_from(member).ok()) | ||
// Note that we iterate from last to first property, so that we highlight properties being overwritten as problems and not those that take effect. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be a bit of a hot take, and ESLint does it the other way. |
||
.rev() | ||
{ | ||
if let Some(member_name) = member_definition.name() { | ||
match defined_properties.remove(&member_name) { | ||
None => { | ||
defined_properties | ||
.insert(member_name, DefinedProperty::from(member_definition)); | ||
} | ||
Some(defined_property) => { | ||
match defined_property.extend_with(member_definition) { | ||
Ok(new_defined_property) => { | ||
defined_properties.insert(member_name, new_defined_property); | ||
} | ||
Err(conflict) => { | ||
signals.push(conflict); | ||
defined_properties.insert(member_name, defined_property); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
signals | ||
} | ||
|
||
fn diagnostic( | ||
_ctx: &RuleContext<Self>, | ||
PropertyConflict(defined_property, member_definition): &Self::State, | ||
) -> Option<RuleDiagnostic> { | ||
let mut diagnostic = RuleDiagnostic::new( | ||
rule_category!(), | ||
member_definition.range(), | ||
format!( | ||
"This {} is later overwritten by an object member with the same name.", | ||
member_definition | ||
), | ||
); | ||
diagnostic = match defined_property { | ||
DefinedProperty::Get(range) => { | ||
diagnostic.detail(range, "Overwritten with this getter.") | ||
} | ||
DefinedProperty::Set(range) => { | ||
diagnostic.detail(range, "Overwritten with this setter.") | ||
} | ||
DefinedProperty::Value(range) => { | ||
diagnostic.detail(range, "Overwritten with this value.") | ||
} | ||
DefinedProperty::GetSet(get_range, set_range) => match member_definition { | ||
MemberDefinition::Getter(_) => { | ||
diagnostic.detail(get_range, "Overwritten with this getter.") | ||
} | ||
MemberDefinition::Setter(_) => { | ||
diagnostic.detail(set_range, "Overwritten with this setter.") | ||
} | ||
MemberDefinition::Method(_) | ||
| MemberDefinition::Property(_) | ||
| MemberDefinition::ShorthandProperty(_) => match get_range.ordering(*set_range) { | ||
Ordering::Less => diagnostic.detail(set_range, "Overwritten with this setter."), | ||
Ordering::Greater => { | ||
diagnostic.detail(get_range, "Overwritten with this getter.") | ||
} | ||
Ordering::Equal => { | ||
panic!( | ||
"The ranges of the property getter and property setter cannot overlap." | ||
) | ||
} | ||
}, | ||
}, | ||
}; | ||
diagnostic = diagnostic.note("If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored."); | ||
|
||
Some(diagnostic) | ||
} | ||
|
||
fn action( | ||
ctx: &RuleContext<Self>, | ||
PropertyConflict(_, member_definition): &Self::State, | ||
) -> Option<JsRuleAction> { | ||
let mut batch = ctx.root().begin(); | ||
batch.remove_js_object_member(&member_definition.node()); | ||
Some(JsRuleAction { | ||
category: rome_analyze::ActionCategory::QuickFix, | ||
// The property initialization could contain side effects | ||
applicability: rome_diagnostics::Applicability::MaybeIncorrect, | ||
message: markup!("Remove this " {member_definition.to_string()}).to_owned(), | ||
mutation: batch, | ||
}) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went ahead and pressed sort on these as there was no apparent order to them