From d7561abaa37a7ed3e298f06cdadb2316869502b2 Mon Sep 17 00:00:00 2001 From: notmd Date: Mon, 28 Nov 2022 23:32:15 +0700 Subject: [PATCH 1/5] feat(rome_js_analyze): implement `noNonNullAssertion` rule --- .../src/categories.rs | 6 +- .../rome_js_analyze/src/analyzers/nursery.rs | 3 +- .../nursery/no_non_null_assertion.rs | 178 +++++++ crates/rome_js_analyze/src/lib.rs | 9 +- .../nursery/noNonNullAssertion/invalid.ts | 18 + .../noNonNullAssertion/invalid.ts.snap | 449 ++++++++++++++++++ .../specs/nursery/noNonNullAssertion/valid.ts | 6 + .../nursery/noNonNullAssertion/valid.ts.snap | 16 + .../src/configuration/linter/rules.rs | 5 +- npm/backend-jsonrpc/src/workspace.ts | 9 +- website/src/pages/lint/rules/index.mdx | 6 + .../pages/lint/rules/noNonNullAssertion.md | 60 +++ 12 files changed, 751 insertions(+), 14 deletions(-) create mode 100644 crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs create mode 100644 crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts create mode 100644 crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap create mode 100644 crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts create mode 100644 crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts.snap create mode 100644 website/src/pages/lint/rules/noNonNullAssertion.md diff --git a/crates/rome_diagnostics_categories/src/categories.rs b/crates/rome_diagnostics_categories/src/categories.rs index f3d4d37cb53..808d93edb26 100644 --- a/crates/rome_diagnostics_categories/src/categories.rs +++ b/crates/rome_diagnostics_categories/src/categories.rs @@ -73,22 +73,22 @@ define_dategories! { "lint/security/noDangerouslySetInnerHtml": "https://docs.rome.tools/lint/rules/noDangerouslySetInnerHtml", "lint/security/noDangerouslySetInnerHtmlWithChildren": "https://docs.rome.tools/lint/rules/noDangerouslySetInnerHtmlWithChildren", - // nursery "lint/nursery/noAccessKey": "https://docs.rome.tools/lint/rules/noAccessKey", "lint/nursery/noBannedTypes":"https://docs.rome.tools/lint/rules/noBannedTypes", "lint/nursery/noConditionalAssignment": "https://docs.rome.tools/lint/rules/noConditionalAssignment", "lint/nursery/noConstAssign": "https://docs.rome.tools/lint/rules/noConstAssign", - "lint/nursery/noDistractingElements": "https://docs.rome.tools/lint/rules/noDistractingElements", "lint/nursery/noConstructorReturn": "https://docs.rome.tools/lint/rules/noConstructorReturn", - "lint/nursery/noSetterReturn": "https://docs.rome.tools/lint/rules/noSetterReturn", + "lint/nursery/noDistractingElements": "https://docs.rome.tools/lint/rules/noDistractingElements", "lint/nursery/noDupeKeys":"https://docs.rome.tools/lint/rules/noDupeKeys", "lint/nursery/noEmptyInterface": "https://docs.rome.tools/lint/rules/noEmptyInterface", "lint/nursery/noExplicitAny": "https://docs.rome.tools/lint/rules/noExplicitAny", "lint/nursery/noExtraNonNullAssertion":"https://docs.rome.tools/lint/rules/noExtraNonNullAssertion", "lint/nursery/noHeaderScope": "https://docs.rome.tools/lint/rules/noHeaderScope", "lint/nursery/noInvalidConstructorSuper": "https://docs.rome.tools/lint/rules/noInvalidConstructorSuper", + "lint/nursery/noNonNullAssertion": "https://docs.rome.tools/lint/rules/noNonNullAssertion", "lint/nursery/noPrecisionLoss": "https://docs.rome.tools/lint/rules/noPrecisionLoss", + "lint/nursery/noSetterReturn": "https://docs.rome.tools/lint/rules/noSetterReturn", "lint/nursery/noStringCaseMismatch": "https://docs.rome.tools/lint/rules/noStringCaseMismatch", "lint/nursery/noUnsafeFinally": "https://docs.rome.tools/lint/rules/noUnsafeFinally", "lint/nursery/noVar": "https://docs.rome.tools/lint/rules/noVar", diff --git a/crates/rome_js_analyze/src/analyzers/nursery.rs b/crates/rome_js_analyze/src/analyzers/nursery.rs index 41c42ba8cb5..433153ce86e 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery.rs @@ -12,6 +12,7 @@ mod no_explicit_any; mod no_extra_non_null_assertion; mod no_header_scope; mod no_invalid_constructor_super; +mod no_non_null_assertion; mod no_precision_loss; mod no_setter_return; mod no_string_case_mismatch; @@ -21,4 +22,4 @@ mod use_default_switch_clause_last; mod use_flat_map; mod use_numeric_literals; mod use_valid_for_direction; -declare_group! { pub (crate) Nursery { name : "nursery" , rules : [self :: no_access_key :: NoAccessKey , self :: no_banned_types :: NoBannedTypes , self :: no_conditional_assignment :: NoConditionalAssignment , self :: no_constructor_return :: NoConstructorReturn , self :: no_distracting_elements :: NoDistractingElements , self :: no_dupe_keys :: NoDupeKeys , self :: no_empty_interface :: NoEmptyInterface , self :: no_explicit_any :: NoExplicitAny , self :: no_extra_non_null_assertion :: NoExtraNonNullAssertion , self :: no_header_scope :: NoHeaderScope , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: no_precision_loss :: NoPrecisionLoss , self :: no_setter_return :: NoSetterReturn , self :: no_string_case_mismatch :: NoStringCaseMismatch , self :: no_unsafe_finally :: NoUnsafeFinally , self :: no_void_type_return :: NoVoidTypeReturn , self :: use_default_switch_clause_last :: UseDefaultSwitchClauseLast , self :: use_flat_map :: UseFlatMap , self :: use_numeric_literals :: UseNumericLiterals , self :: use_valid_for_direction :: UseValidForDirection ,] } } +declare_group! { pub (crate) Nursery { name : "nursery" , rules : [self :: no_access_key :: NoAccessKey , self :: no_banned_types :: NoBannedTypes , self :: no_conditional_assignment :: NoConditionalAssignment , self :: no_constructor_return :: NoConstructorReturn , self :: no_distracting_elements :: NoDistractingElements , self :: no_dupe_keys :: NoDupeKeys , self :: no_empty_interface :: NoEmptyInterface , self :: no_explicit_any :: NoExplicitAny , self :: no_extra_non_null_assertion :: NoExtraNonNullAssertion , self :: no_header_scope :: NoHeaderScope , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: no_non_null_assertion :: NoNonNullAssertion , self :: no_precision_loss :: NoPrecisionLoss , self :: no_setter_return :: NoSetterReturn , self :: no_string_case_mismatch :: NoStringCaseMismatch , self :: no_unsafe_finally :: NoUnsafeFinally , self :: no_void_type_return :: NoVoidTypeReturn , self :: use_default_switch_clause_last :: UseDefaultSwitchClauseLast , self :: use_flat_map :: UseFlatMap , self :: use_numeric_literals :: UseNumericLiterals , self :: use_valid_for_direction :: UseValidForDirection ,] } } diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs new file mode 100644 index 00000000000..04ca5ad267c --- /dev/null +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs @@ -0,0 +1,178 @@ +use crate::JsRuleAction; +use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic}; +use rome_console::markup; +use rome_diagnostics::Applicability; +use rome_js_factory::make; +use rome_js_syntax::{JsAnyExpression, JsSyntaxKind, TsNonNullAssertionExpression}; +use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; + +declare_rule! { + /// Disallow non-null assertions using the `!` postfix operator. + /// + /// TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as + /// in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that + /// code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands + /// when values may be nullable. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```ts,expect_diagnostic + /// interface Example { + /// property?: string; + /// } + /// declare const example: Example; + /// const includesBaz = foo.property!.includes('baz'); + /// ``` + /// + /// ### Valid + /// + /// ```ts + /// interface Example { + /// property?: string; + /// } + /// + /// declare const example: Example; + /// const includesBaz = foo.property?.includes('baz') ?? false; + /// ``` + /// + pub(crate) NoNonNullAssertion { + version: "11.0.0", + name: "noNonNullAssertion", + recommended: false, + } +} + +fn can_replace_with_optional_chain(node: &TsNonNullAssertionExpression) -> bool { + match node.parent::() { + Some(parent) => { + matches!( + parent.syntax().kind(), + JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION + | JsSyntaxKind::JS_COMPUTED_MEMBER_EXPRESSION + | JsSyntaxKind::JS_CALL_EXPRESSION + ) + } + None => false, + } +} + +impl Rule for NoNonNullAssertion { + type Query = Ast; + type State = (TsNonNullAssertionExpression, bool); + type Signals = Option; + type Options = (); + + fn run(ctx: &RuleContext) -> Self::Signals { + let node = ctx.query(); + if let Some(child) = node.syntax().first_child() { + let is_child_non_null_assertion = + matches!(child.kind(), JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION); + if is_child_non_null_assertion { + return None; + } + + let most_outer_ts_non_null_assertion_node = node + .syntax() + .ancestors() + .take_while(|n| -> bool { + n.kind() == JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION + }) + .last()? + .cast::()?; + + return Some(( + most_outer_ts_non_null_assertion_node.clone(), + can_replace_with_optional_chain(&most_outer_ts_non_null_assertion_node), + )); + } else { + return Some((node.clone(), can_replace_with_optional_chain(node))); + } + } + + fn diagnostic(_ctx: &RuleContext, state: &Self::State) -> Option { + let (node, _) = state; + Some( + RuleDiagnostic::new( + rule_category!(), + node.syntax().text_trimmed_range(), + markup! { + "Forbidden non-null assertion." + }, + ).note( + markup! { + "Consider using the optional chain operator ""?."" instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + " + }, + ) + ) + } + + fn action(ctx: &RuleContext, state: &Self::State) -> Option { + let (node, can_fix) = state; + if !can_fix { + return None; + } + + let mut mutation = ctx.root().begin(); + + node.syntax() + .descendants() + .map_while(|x| { + let child = x.first_child()?; + if child.kind() == JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION { + Some(child.cast::()?) + } else { + None + } + }) + .for_each(|x| { + if let Some(excl_token) = x.excl_token().ok() { + mutation.remove_token(excl_token); + } + }); + + match node.parent::()? { + JsAnyExpression::JsComputedMemberExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTIONDOT), + ); + } + } + JsAnyExpression::JsCallExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTIONDOT), + ); + } + } + JsAnyExpression::JsStaticMemberExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTION), + ); + } + } + _ => {} + }; + + Some(JsRuleAction { + category: ActionCategory::QuickFix, + applicability: Applicability::MaybeIncorrect, + message: markup! { "Replace with optional chain operator ""?""." } + .to_owned(), + mutation, + }) + } +} diff --git a/crates/rome_js_analyze/src/lib.rs b/crates/rome_js_analyze/src/lib.rs index 0936deb7155..57f88b1fb84 100644 --- a/crates/rome_js_analyze/src/lib.rs +++ b/crates/rome_js_analyze/src/lib.rs @@ -149,13 +149,8 @@ mod tests { String::from_utf8(buffer).unwrap() } - const SOURCE: &str = r#"function f() { - return ( -
- ) - }; - f(); + const SOURCE: &str = r#"const x = {}; + x!!.y; "#; let parsed = parse(SOURCE, FileId::zero(), SourceType::jsx()); diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts new file mode 100644 index 00000000000..2b380574297 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts @@ -0,0 +1,18 @@ +x!; +x!.y; +x.y!; +!x!.y; +x!.y?.z; +x![y]; +x![y]?.z; +x.y.z!(); +x.y?.z!(); +x!!!; +x!!.y; +x.y!!; +x.y.z!!(); +x!?.[y].z; +x!?.y.z; +x!!!?.y.z; +x.y.z!?.(); +x.y.z!!!?.(); diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap new file mode 100644 index 00000000000..538aa8208e5 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap @@ -0,0 +1,449 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: invalid.ts +--- +# Input +```js +x!; +x!.y; +x.y!; +!x!.y; +x!.y?.z; +x![y]; +x![y]?.z; +x.y.z!(); +x.y?.z!(); +x!!!; +x!!.y; +x.y!!; +x.y.z!!(); +x!?.[y].z; +x!?.y.z; +x!!!?.y.z; +x.y.z!?.(); +x.y.z!!!?.(); + +``` + +# Diagnostics +``` +invalid.ts:1:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + > 1 │ x!; + │ ^^ + 2 │ x!.y; + 3 │ x.y!; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + +``` + +``` +invalid.ts:2:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 1 │ x!; + > 2 │ x!.y; + │ ^^ + 3 │ x.y!; + 4 │ !x!.y; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 1 1 │ x!; + 2 │ - x!.y; + 2 │ + x?.y; + 3 3 │ x.y!; + 4 4 │ !x!.y; + + +``` + +``` +invalid.ts:3:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 1 │ x!; + 2 │ x!.y; + > 3 │ x.y!; + │ ^^^^ + 4 │ !x!.y; + 5 │ x!.y?.z; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + +``` + +``` +invalid.ts:4:2 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 2 │ x!.y; + 3 │ x.y!; + > 4 │ !x!.y; + │ ^^ + 5 │ x!.y?.z; + 6 │ x![y]; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 2 2 │ x!.y; + 3 3 │ x.y!; + 4 │ - !x!.y; + 4 │ + !x?.y; + 5 5 │ x!.y?.z; + 6 6 │ x![y]; + + +``` + +``` +invalid.ts:5:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 3 │ x.y!; + 4 │ !x!.y; + > 5 │ x!.y?.z; + │ ^^ + 6 │ x![y]; + 7 │ x![y]?.z; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 3 3 │ x.y!; + 4 4 │ !x!.y; + 5 │ - x!.y?.z; + 5 │ + x?.y?.z; + 6 6 │ x![y]; + 7 7 │ x![y]?.z; + + +``` + +``` +invalid.ts:6:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 4 │ !x!.y; + 5 │ x!.y?.z; + > 6 │ x![y]; + │ ^^ + 7 │ x![y]?.z; + 8 │ x.y.z!(); + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 4 4 │ !x!.y; + 5 5 │ x!.y?.z; + 6 │ - x![y]; + 6 │ + x?.[y]; + 7 7 │ x![y]?.z; + 8 8 │ x.y.z!(); + + +``` + +``` +invalid.ts:7:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 5 │ x!.y?.z; + 6 │ x![y]; + > 7 │ x![y]?.z; + │ ^^ + 8 │ x.y.z!(); + 9 │ x.y?.z!(); + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 5 5 │ x!.y?.z; + 6 6 │ x![y]; + 7 │ - x![y]?.z; + 7 │ + x?.[y]?.z; + 8 8 │ x.y.z!(); + 9 9 │ x.y?.z!(); + + +``` + +``` +invalid.ts:8:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 6 │ x![y]; + 7 │ x![y]?.z; + > 8 │ x.y.z!(); + │ ^^^^^^ + 9 │ x.y?.z!(); + 10 │ x!!!; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 6 6 │ x![y]; + 7 7 │ x![y]?.z; + 8 │ - x.y.z!(); + 8 │ + x.y.z?.(); + 9 9 │ x.y?.z!(); + 10 10 │ x!!!; + + +``` + +``` +invalid.ts:9:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 7 │ x![y]?.z; + 8 │ x.y.z!(); + > 9 │ x.y?.z!(); + │ ^^^^^^^ + 10 │ x!!!; + 11 │ x!!.y; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 7 7 │ x![y]?.z; + 8 8 │ x.y.z!(); + 9 │ - x.y?.z!(); + 9 │ + x.y?.z?.(); + 10 10 │ x!!!; + 11 11 │ x!!.y; + + +``` + +``` +invalid.ts:10:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 8 │ x.y.z!(); + 9 │ x.y?.z!(); + > 10 │ x!!!; + │ ^^^^ + 11 │ x!!.y; + 12 │ x.y!!; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + +``` + +``` +invalid.ts:11:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 9 │ x.y?.z!(); + 10 │ x!!!; + > 11 │ x!!.y; + │ ^^^ + 12 │ x.y!!; + 13 │ x.y.z!!(); + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 9 9 │ x.y?.z!(); + 10 10 │ x!!!; + 11 │ - x!!.y; + 11 │ + x?.y; + 12 12 │ x.y!!; + 13 13 │ x.y.z!!(); + + +``` + +``` +invalid.ts:12:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 10 │ x!!!; + 11 │ x!!.y; + > 12 │ x.y!!; + │ ^^^^^ + 13 │ x.y.z!!(); + 14 │ x!?.[y].z; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + +``` + +``` +invalid.ts:13:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 11 │ x!!.y; + 12 │ x.y!!; + > 13 │ x.y.z!!(); + │ ^^^^^^^ + 14 │ x!?.[y].z; + 15 │ x!?.y.z; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 11 11 │ x!!.y; + 12 12 │ x.y!!; + 13 │ - x.y.z!!(); + 13 │ + x.y.z?.(); + 14 14 │ x!?.[y].z; + 15 15 │ x!?.y.z; + + +``` + +``` +invalid.ts:14:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 12 │ x.y!!; + 13 │ x.y.z!!(); + > 14 │ x!?.[y].z; + │ ^^ + 15 │ x!?.y.z; + 16 │ x!!!?.y.z; + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 14 │ x!?.[y].z; + │ - + +``` + +``` +invalid.ts:15:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 13 │ x.y.z!!(); + 14 │ x!?.[y].z; + > 15 │ x!?.y.z; + │ ^^ + 16 │ x!!!?.y.z; + 17 │ x.y.z!?.(); + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 15 │ x!?.y.z; + │ - + +``` + +``` +invalid.ts:16:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 14 │ x!?.[y].z; + 15 │ x!?.y.z; + > 16 │ x!!!?.y.z; + │ ^^^^ + 17 │ x.y.z!?.(); + 18 │ x.y.z!!!?.(); + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 16 │ x!!!?.y.z; + │ --- + +``` + +``` +invalid.ts:17:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 15 │ x!?.y.z; + 16 │ x!!!?.y.z; + > 17 │ x.y.z!?.(); + │ ^^^^^^ + 18 │ x.y.z!!!?.(); + 19 │ + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 17 │ x.y.z!?.(); + │ - + +``` + +``` +invalid.ts:18:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 16 │ x!!!?.y.z; + 17 │ x.y.z!?.(); + > 18 │ x.y.z!!!?.(); + │ ^^^^^^^^ + 19 │ + + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + + + i Suggested fix: Replace with optional chain operator ?. + + 18 │ x.y.z!!!?.(); + │ --- + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts new file mode 100644 index 00000000000..83d1de9445e --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts @@ -0,0 +1,6 @@ +x; +x.y; +x.y.z; +x?.y.z; +x?.y?.z; +!x; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts.snap b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts.snap new file mode 100644 index 00000000000..18797edb6ec --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/valid.ts.snap @@ -0,0 +1,16 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: valid.ts +--- +# Input +```js +x; +x.y; +x.y.z; +x?.y.z; +x?.y?.z; +!x; + +``` + + diff --git a/crates/rome_service/src/configuration/linter/rules.rs b/crates/rome_service/src/configuration/linter/rules.rs index e7972f3bfd9..0a4b99e9dd7 100644 --- a/crates/rome_service/src/configuration/linter/rules.rs +++ b/crates/rome_service/src/configuration/linter/rules.rs @@ -753,6 +753,8 @@ struct NurserySchema { no_header_scope: Option, #[doc = "Prevents the incorrect use of super() inside classes. It also checks whether a call super() is missing from classes that extends other constructors."] no_invalid_constructor_super: Option, + #[doc = "Disallow non-null assertions using the ! postfix operator."] + no_non_null_assertion: Option, #[doc = "Disallow literal numbers that lose precision"] no_precision_loss: Option, #[doc = "Disallow returning a value from a setter"] @@ -782,7 +784,7 @@ struct NurserySchema { } impl Nursery { const CATEGORY_NAME: &'static str = "nursery"; - pub(crate) const CATEGORY_RULES: [&'static str; 25] = [ + pub(crate) const CATEGORY_RULES: [&'static str; 26] = [ "noAccessKey", "noBannedTypes", "noConditionalAssignment", @@ -795,6 +797,7 @@ impl Nursery { "noExtraNonNullAssertion", "noHeaderScope", "noInvalidConstructorSuper", + "noNonNullAssertion", "noPrecisionLoss", "noSetterReturn", "noStringCaseMismatch", diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index 33e692e4756..2776fb27b45 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -395,6 +395,10 @@ export interface Nursery { * Prevents the incorrect use of super() inside classes. It also checks whether a call super() is missing from classes that extends other constructors. */ noInvalidConstructorSuper?: RuleConfiguration; + /** + * Disallow non-null assertions using the ! postfix operator. + */ + noNonNullAssertion?: RuleConfiguration; /** * Disallow literal numbers that lose precision */ @@ -659,16 +663,17 @@ export type Category = | "lint/nursery/noBannedTypes" | "lint/nursery/noConditionalAssignment" | "lint/nursery/noConstAssign" - | "lint/nursery/noDistractingElements" | "lint/nursery/noConstructorReturn" - | "lint/nursery/noSetterReturn" + | "lint/nursery/noDistractingElements" | "lint/nursery/noDupeKeys" | "lint/nursery/noEmptyInterface" | "lint/nursery/noExplicitAny" | "lint/nursery/noExtraNonNullAssertion" | "lint/nursery/noHeaderScope" | "lint/nursery/noInvalidConstructorSuper" + | "lint/nursery/noNonNullAssertion" | "lint/nursery/noPrecisionLoss" + | "lint/nursery/noSetterReturn" | "lint/nursery/noStringCaseMismatch" | "lint/nursery/noUnsafeFinally" | "lint/nursery/noVar" diff --git a/website/src/pages/lint/rules/index.mdx b/website/src/pages/lint/rules/index.mdx index f7a2a9d666b..4c777100cf8 100644 --- a/website/src/pages/lint/rules/index.mdx +++ b/website/src/pages/lint/rules/index.mdx @@ -517,6 +517,12 @@ Prevents the incorrect use of super() inside classes. It also checks whether a call super() is missing from classes that extends other constructors.
+

+ noNonNullAssertion +

+Disallow non-null assertions using the ! postfix operator. +
+

noPrecisionLoss

diff --git a/website/src/pages/lint/rules/noNonNullAssertion.md b/website/src/pages/lint/rules/noNonNullAssertion.md new file mode 100644 index 00000000000..7d355fbe6ce --- /dev/null +++ b/website/src/pages/lint/rules/noNonNullAssertion.md @@ -0,0 +1,60 @@ +--- +title: Lint Rule noNonNullAssertion +parent: lint/rules/index +--- + +# noNonNullAssertion (since v11.0.0) + +Disallow non-null assertions using the `!` postfix operator. + +TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as +in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that +code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands +when values may be nullable. + +## Examples + +### Invalid + +```ts +interface Example { + property?: string; +} +declare const example: Example; +const includesBaz = foo.property!.includes('baz'); +``` + +
nursery/noNonNullAssertion.js:5:21 lint/nursery/noNonNullAssertion  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ Forbidden non-null assertion.
+  
+    3 │ }
+    4 │ declare const example: Example;
+  > 5 │ const includesBaz = foo.property!.includes('baz');
+      │                     ^^^^^^^^^^^^^
+    6 │ 
+  
+  ℹ Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.',
+                        
+  
+  ℹ Suggested fix: Replace with optional chain operator ?.
+  
+    3 3 │   }
+    4 4 │   declare const example: Example;
+    5   │ - const·includesBaz·=·foo.property!.includes('baz');
+      5 │ + const·includesBaz·=·foo.property?.includes('baz');
+    6 6 │   
+  
+
+ +### Valid + +```ts +interface Example { + property?: string; +} + +declare const example: Example; +const includesBaz = foo.property?.includes('baz') ?? false; +``` + From 1361251da13b51432a86a59e4132f7a367601471 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 29 Nov 2022 01:09:20 +0700 Subject: [PATCH 2/5] chore: update diagnotic message --- .../nursery/no_non_null_assertion.rs | 2 +- .../noNonNullAssertion/invalid.ts.snap | 36 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs index 04ca5ad267c..9d02a0f837f 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs @@ -102,7 +102,7 @@ impl Rule for NoNonNullAssertion { }, ).note( markup! { - "Consider using the optional chain operator ""?."" instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + "Consider using the optional chain operator ""?."" instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. " }, ) diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap index 538aa8208e5..6d45d1e14aa 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap @@ -36,7 +36,7 @@ invalid.ts:1:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━ 2 │ x!.y; 3 │ x.y!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. @@ -53,7 +53,7 @@ invalid.ts:2:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 3 │ x.y!; 4 │ !x!.y; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -79,7 +79,7 @@ invalid.ts:3:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━ 4 │ !x!.y; 5 │ x!.y?.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. @@ -97,7 +97,7 @@ invalid.ts:4:2 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 5 │ x!.y?.z; 6 │ x![y]; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -124,7 +124,7 @@ invalid.ts:5:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 6 │ x![y]; 7 │ x![y]?.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -151,7 +151,7 @@ invalid.ts:6:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 7 │ x![y]?.z; 8 │ x.y.z!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -178,7 +178,7 @@ invalid.ts:7:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 8 │ x.y.z!(); 9 │ x.y?.z!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -205,7 +205,7 @@ invalid.ts:8:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 9 │ x.y?.z!(); 10 │ x!!!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -232,7 +232,7 @@ invalid.ts:9:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 10 │ x!!!; 11 │ x!!.y; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -259,7 +259,7 @@ invalid.ts:10:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━ 11 │ x!!.y; 12 │ x.y!!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. @@ -277,7 +277,7 @@ invalid.ts:11:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 12 │ x.y!!; 13 │ x.y.z!!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -304,7 +304,7 @@ invalid.ts:12:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━ 13 │ x.y.z!!(); 14 │ x!?.[y].z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. @@ -322,7 +322,7 @@ invalid.ts:13:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 14 │ x!?.[y].z; 15 │ x!?.y.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -349,7 +349,7 @@ invalid.ts:14:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 15 │ x!?.y.z; 16 │ x!!!?.y.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -371,7 +371,7 @@ invalid.ts:15:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 16 │ x!!!?.y.z; 17 │ x.y.z!?.(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -393,7 +393,7 @@ invalid.ts:16:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 17 │ x.y.z!?.(); 18 │ x.y.z!!!?.(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -415,7 +415,7 @@ invalid.ts:17:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 18 │ x.y.z!!!?.(); 19 │ - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. @@ -436,7 +436,7 @@ invalid.ts:18:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ │ ^^^^^^^^ 19 │ - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. i Suggested fix: Replace with optional chain operator ?. From 74b6f25ad57d6f33bdd0d3eb14a3a837c27469d9 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 29 Nov 2022 01:31:08 +0700 Subject: [PATCH 3/5] chore: codegen --- crates/rome_service/src/configuration/linter/rules.rs | 2 +- editors/vscode/configuration_schema.json | 11 +++++++++++ npm/backend-jsonrpc/src/workspace.ts | 4 ++-- npm/rome/configuration_schema.json | 11 +++++++++++ website/src/pages/lint/rules/noNonNullAssertion.md | 2 +- 5 files changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/rome_service/src/configuration/linter/rules.rs b/crates/rome_service/src/configuration/linter/rules.rs index db79891cd99..faed9a24bb1 100644 --- a/crates/rome_service/src/configuration/linter/rules.rs +++ b/crates/rome_service/src/configuration/linter/rules.rs @@ -790,7 +790,7 @@ struct NurserySchema { } impl Nursery { const CATEGORY_NAME: &'static str = "nursery"; - pub(crate) const CATEGORY_RULES: [&'static str; 28] = [ + pub(crate) const CATEGORY_RULES: [&'static str; 29] = [ "noAccessKey", "noBannedTypes", "noConditionalAssignment", diff --git a/editors/vscode/configuration_schema.json b/editors/vscode/configuration_schema.json index 6b6d01ab955..00761d15c69 100644 --- a/editors/vscode/configuration_schema.json +++ b/editors/vscode/configuration_schema.json @@ -896,6 +896,17 @@ } ] }, + "noNonNullAssertion": { + "description": "Disallow non-null assertions using the ! postfix operator.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "noPrecisionLoss": { "description": "Disallow literal numbers that lose precision", "anyOf": [ diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index 70df33fc1ac..f707a7b5f09 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -961,7 +961,7 @@ export interface RenameResult { } export interface Workspace { supportsFeature( - params: SupportsFeatureParams + params: SupportsFeatureParams, ): Promise; updateSettings(params: UpdateSettingsParams): Promise; openFile(params: OpenFileParams): Promise; @@ -971,7 +971,7 @@ export interface Workspace { getControlFlowGraph(params: GetControlFlowGraphParams): Promise; getFormatterIr(params: GetFormatterIRParams): Promise; pullDiagnostics( - params: PullDiagnosticsParams + params: PullDiagnosticsParams, ): Promise; pullActions(params: PullActionsParams): Promise; formatFile(params: FormatFileParams): Promise; diff --git a/npm/rome/configuration_schema.json b/npm/rome/configuration_schema.json index 6b6d01ab955..00761d15c69 100644 --- a/npm/rome/configuration_schema.json +++ b/npm/rome/configuration_schema.json @@ -896,6 +896,17 @@ } ] }, + "noNonNullAssertion": { + "description": "Disallow non-null assertions using the ! postfix operator.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "noPrecisionLoss": { "description": "Disallow literal numbers that lose precision", "anyOf": [ diff --git a/website/src/pages/lint/rules/noNonNullAssertion.md b/website/src/pages/lint/rules/noNonNullAssertion.md index 7d355fbe6ce..465a57a7065 100644 --- a/website/src/pages/lint/rules/noNonNullAssertion.md +++ b/website/src/pages/lint/rules/noNonNullAssertion.md @@ -34,7 +34,7 @@ const includesBaz = foo.property!.includes('baz'); │ ^^^^^^^^^^^^^ 6 │ - ℹ Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator.', + ℹ Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. ℹ Suggested fix: Replace with optional chain operator ?. From ced3942a40cb2236e71d6132e678a6a02693b853 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 29 Nov 2022 22:37:52 +0700 Subject: [PATCH 4/5] fix: code review --- .../nursery/no_non_null_assertion.rs | 227 +++++++++--------- .../nursery/noNonNullAssertion/invalid.ts | 2 + .../noNonNullAssertion/invalid.ts.snap | 118 ++++----- .../pages/lint/rules/noNonNullAssertion.md | 25 +- 4 files changed, 175 insertions(+), 197 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs index 178132f2fc9..d1508eaf0cb 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs @@ -3,15 +3,17 @@ use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_factory::make; -use rome_js_syntax::{AnyJsExpression, JsSyntaxKind, TsNonNullAssertionExpression}; -use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; +use rome_js_syntax::{ + AnyJsExpression, JsSyntaxKind, TsNonNullAssertionAssignment, TsNonNullAssertionExpression, +}; +use rome_rowan::{declare_node_union, AstNode, BatchMutationExt}; declare_rule! { /// Disallow non-null assertions using the `!` postfix operator. /// - /// TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as - /// in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that - /// code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands + /// TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as + /// in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that + /// code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands /// when values may be nullable. /// /// ## Examples @@ -25,14 +27,17 @@ declare_rule! { /// declare const example: Example; /// const includesBaz = foo.property!.includes('baz'); /// ``` - /// + /// ```ts,expect_diagnostic + /// (b!! as number) = "test"; + /// ``` + /// /// ### Valid - /// + /// /// ```ts /// interface Example { /// property?: string; /// } - /// + /// /// declare const example: Example; /// const includesBaz = foo.property?.includes('baz') ?? false; /// ``` @@ -44,135 +49,117 @@ declare_rule! { } } -fn can_replace_with_optional_chain(node: &TsNonNullAssertionExpression) -> bool { - match node.parent::() { - Some(parent) => { - matches!( - parent.syntax().kind(), - JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION - | JsSyntaxKind::JS_COMPUTED_MEMBER_EXPRESSION - | JsSyntaxKind::JS_CALL_EXPRESSION - ) - } - None => false, - } +declare_node_union! { + pub(crate) AnyTsNonNullAssertion = TsNonNullAssertionExpression | TsNonNullAssertionAssignment } impl Rule for NoNonNullAssertion { - type Query = Ast; - type State = (TsNonNullAssertionExpression, bool); + type Query = Ast; + type State = (); type Signals = Option; type Options = (); fn run(ctx: &RuleContext) -> Self::Signals { - let node = ctx.query(); - if let Some(child) = node.syntax().first_child() { - let is_child_non_null_assertion = - matches!(child.kind(), JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION); - if is_child_non_null_assertion { - return None; - } - - let most_outer_ts_non_null_assertion_node = node - .syntax() - .ancestors() - .take_while(|n| -> bool { - n.kind() == JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION - }) - .last()? - .cast::()?; - - return Some(( - most_outer_ts_non_null_assertion_node.clone(), - can_replace_with_optional_chain(&most_outer_ts_non_null_assertion_node), - )); - } else { - return Some((node.clone(), can_replace_with_optional_chain(node))); + match ctx.query() { + AnyTsNonNullAssertion::TsNonNullAssertionExpression(node) => node + .parent::() + .map_or(Some(()), |_| None), + AnyTsNonNullAssertion::TsNonNullAssertionAssignment(node) => node + .parent::() + .map_or(Some(()), |_| None), } } - fn diagnostic(_ctx: &RuleContext, state: &Self::State) -> Option { - let (node, _) = state; - Some( - RuleDiagnostic::new( - rule_category!(), - node.syntax().text_trimmed_range(), - markup! { - "Forbidden non-null assertion." - }, - ).note( - markup! { - "Consider using the optional chain operator ""?."" instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - " - }, - ) - ) + fn diagnostic(ctx: &RuleContext, _state: &Self::State) -> Option { + Some(RuleDiagnostic::new( + rule_category!(), + ctx.query().range(), + markup! { + "Forbidden non-null assertion." + }, + )) } - fn action(ctx: &RuleContext, state: &Self::State) -> Option { - let (node, can_fix) = state; - if !can_fix { - return None; - } + fn action(ctx: &RuleContext, _state: &Self::State) -> Option { + let node = ctx.query(); + match node { + AnyTsNonNullAssertion::TsNonNullAssertionAssignment(_) => None, + AnyTsNonNullAssertion::TsNonNullAssertionExpression(node) => { + if !can_replace_with_optional_chain(node) { + return None; + } + let mut mutation = ctx.root().begin(); - let mut mutation = ctx.root().begin(); + let assertions = + std::iter::successors(node.expression().ok(), |expression| match expression { + AnyJsExpression::TsNonNullAssertionExpression(assertion) => { + assertion.expression().ok() + } + _ => None, + }); - node.syntax() - .descendants() - .map_while(|x| { - let child = x.first_child()?; - if child.kind() == JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION { - Some(child.cast::()?) - } else { - None - } - }) - .for_each(|x| { - if let Some(excl_token) = x.excl_token().ok() { - mutation.remove_token(excl_token); + for assertion in assertions { + if let Some(non_null_expr) = assertion.as_ts_non_null_assertion_expression() { + mutation.remove_token(non_null_expr.excl_token().ok()?); + } } - }); - match node.parent::()? { - AnyJsExpression::JsComputedMemberExpression(parent) => { - if parent.is_optional() { - mutation.remove_token(node.excl_token().ok()?); - } else { - mutation.replace_token( - node.excl_token().ok()?, - make::token(JsSyntaxKind::QUESTIONDOT), - ); - } - } - AnyJsExpression::JsCallExpression(parent) => { - if parent.is_optional() { - mutation.remove_token(node.excl_token().ok()?); - } else { - mutation.replace_token( - node.excl_token().ok()?, - make::token(JsSyntaxKind::QUESTIONDOT), - ); - } - } - AnyJsExpression::JsStaticMemberExpression(parent) => { - if parent.is_optional() { - mutation.remove_token(node.excl_token().ok()?); - } else { - mutation.replace_token( - node.excl_token().ok()?, - make::token(JsSyntaxKind::QUESTION), - ); - } + match node.parent::()? { + AnyJsExpression::JsComputedMemberExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTIONDOT), + ); + } + } + AnyJsExpression::JsCallExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTIONDOT), + ); + } + } + AnyJsExpression::JsStaticMemberExpression(parent) => { + if parent.is_optional() { + mutation.remove_token(node.excl_token().ok()?); + } else { + mutation.replace_token( + node.excl_token().ok()?, + make::token(JsSyntaxKind::QUESTION), + ); + } + } + _ => {} + }; + + Some(JsRuleAction { + category: ActionCategory::QuickFix, + applicability: Applicability::MaybeIncorrect, + message: markup! { "Replace with optional chain operator ""?."" This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator" } + .to_owned(), + mutation, + }) } - _ => {} - }; + } + } +} - Some(JsRuleAction { - category: ActionCategory::QuickFix, - applicability: Applicability::MaybeIncorrect, - message: markup! { "Replace with optional chain operator ""?""." } - .to_owned(), - mutation, - }) +fn can_replace_with_optional_chain(node: &TsNonNullAssertionExpression) -> bool { + match node.parent::() { + Some(parent) => { + matches!( + parent.syntax().kind(), + JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION + | JsSyntaxKind::JS_COMPUTED_MEMBER_EXPRESSION + | JsSyntaxKind::JS_CALL_EXPRESSION + ) + } + None => false, } } diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts index 2b380574297..15c75315302 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts @@ -16,3 +16,5 @@ x!?.y.z; x!!!?.y.z; x.y.z!?.(); x.y.z!!!?.(); +(b! as number) = "test"; +(b!! as number) = "test"; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap index 6d45d1e14aa..8271b09f6e3 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noNonNullAssertion/invalid.ts.snap @@ -22,6 +22,8 @@ x!?.y.z; x!!!?.y.z; x.y.z!?.(); x.y.z!!!?.(); +(b! as number) = "test"; +(b!! as number) = "test"; ``` @@ -36,9 +38,6 @@ invalid.ts:1:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━ 2 │ x!.y; 3 │ x.y!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - ``` @@ -53,10 +52,7 @@ invalid.ts:2:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 3 │ x.y!; 4 │ !x!.y; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 1 1 │ x!; 2 │ - x!.y; @@ -79,9 +75,6 @@ invalid.ts:3:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━━ 4 │ !x!.y; 5 │ x!.y?.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - ``` @@ -97,10 +90,7 @@ invalid.ts:4:2 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 5 │ x!.y?.z; 6 │ x![y]; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 2 2 │ x!.y; 3 3 │ x.y!; @@ -124,10 +114,7 @@ invalid.ts:5:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 6 │ x![y]; 7 │ x![y]?.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 3 3 │ x.y!; 4 4 │ !x!.y; @@ -151,10 +138,7 @@ invalid.ts:6:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 7 │ x![y]?.z; 8 │ x.y.z!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 4 4 │ !x!.y; 5 5 │ x!.y?.z; @@ -178,10 +162,7 @@ invalid.ts:7:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 8 │ x.y.z!(); 9 │ x.y?.z!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 5 5 │ x!.y?.z; 6 6 │ x![y]; @@ -205,10 +186,7 @@ invalid.ts:8:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 9 │ x.y?.z!(); 10 │ x!!!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 6 6 │ x![y]; 7 7 │ x![y]?.z; @@ -232,10 +210,7 @@ invalid.ts:9:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 10 │ x!!!; 11 │ x!!.y; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 7 7 │ x![y]?.z; 8 8 │ x.y.z!(); @@ -259,9 +234,6 @@ invalid.ts:10:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━ 11 │ x!!.y; 12 │ x.y!!; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - ``` @@ -277,10 +249,7 @@ invalid.ts:11:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 12 │ x.y!!; 13 │ x.y.z!!(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 9 9 │ x.y?.z!(); 10 10 │ x!!!; @@ -304,9 +273,6 @@ invalid.ts:12:1 lint/nursery/noNonNullAssertion ━━━━━━━━━━ 13 │ x.y.z!!(); 14 │ x!?.[y].z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - ``` @@ -322,10 +288,7 @@ invalid.ts:13:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 14 │ x!?.[y].z; 15 │ x!?.y.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 11 11 │ x!!.y; 12 12 │ x.y!!; @@ -349,10 +312,7 @@ invalid.ts:14:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 15 │ x!?.y.z; 16 │ x!!!?.y.z; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 14 │ x!?.[y].z; │ - @@ -371,10 +331,7 @@ invalid.ts:15:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 16 │ x!!!?.y.z; 17 │ x.y.z!?.(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 15 │ x!?.y.z; │ - @@ -393,10 +350,7 @@ invalid.ts:16:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 17 │ x.y.z!?.(); 18 │ x.y.z!!!?.(); - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 16 │ x!!!?.y.z; │ --- @@ -413,12 +367,9 @@ invalid.ts:17:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ > 17 │ x.y.z!?.(); │ ^^^^^^ 18 │ x.y.z!!!?.(); - 19 │ - - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - + 19 │ (b! as number) = "test"; - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 17 │ x.y.z!?.(); │ - @@ -434,16 +385,43 @@ invalid.ts:18:1 lint/nursery/noNonNullAssertion FIXABLE ━━━━━━━ 17 │ x.y.z!?.(); > 18 │ x.y.z!!!?.(); │ ^^^^^^^^ - 19 │ + 19 │ (b! as number) = "test"; + 20 │ (b!! as number) = "test"; - i Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - i Suggested fix: Replace with optional chain operator ?. + i Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 18 │ x.y.z!!!?.(); │ --- ``` +``` +invalid.ts:19:2 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 17 │ x.y.z!?.(); + 18 │ x.y.z!!!?.(); + > 19 │ (b! as number) = "test"; + │ ^^ + 20 │ (b!! as number) = "test"; + 21 │ + + +``` + +``` +invalid.ts:20:2 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Forbidden non-null assertion. + + 18 │ x.y.z!!!?.(); + 19 │ (b! as number) = "test"; + > 20 │ (b!! as number) = "test"; + │ ^^^ + 21 │ + + +``` + diff --git a/website/src/pages/lint/rules/noNonNullAssertion.md b/website/src/pages/lint/rules/noNonNullAssertion.md index 465a57a7065..2b9f32ba53d 100644 --- a/website/src/pages/lint/rules/noNonNullAssertion.md +++ b/website/src/pages/lint/rules/noNonNullAssertion.md @@ -7,9 +7,9 @@ parent: lint/rules/index Disallow non-null assertions using the `!` postfix operator. -TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as -in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that -code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands +TypeScript's `!` non-null assertion operator asserts to the type system that an expression is non-nullable, as +in not `null` or `undefined`. Using assertions to tell the type system new information is often a sign that +code is not fully type-safe. It's generally better to structure program logic so that TypeScript understands when values may be nullable. ## Examples @@ -34,10 +34,7 @@ const includesBaz = foo.property!.includes('baz'); │ ^^^^^^^^^^^^^ 6 │ - ℹ Consider using the optional chain operator ?. instead. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator. - - - ℹ Suggested fix: Replace with optional chain operator ?. + ℹ Suggested fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator 3 3 │ } 4 4 │ declare const example: Example; @@ -47,6 +44,20 @@ const includesBaz = foo.property!.includes('baz'); +```ts +(b!! as number) = "test"; +``` + +
nursery/noNonNullAssertion.js:1:2 lint/nursery/noNonNullAssertion ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ Forbidden non-null assertion.
+  
+  > 1 │ (b!! as number) = "test";
+      │  ^^^
+    2 │ 
+  
+
+ ### Valid ```ts From dbdddb4352c71681aa05796fefc009c73c92bf09 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 29 Nov 2022 22:54:24 +0700 Subject: [PATCH 5/5] refactor: match expression directly instead of use syntax kind --- .../analyzers/nursery/no_non_null_assertion.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs index d1508eaf0cb..ec3200b88de 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_non_null_assertion.rs @@ -151,15 +151,10 @@ impl Rule for NoNonNullAssertion { } fn can_replace_with_optional_chain(node: &TsNonNullAssertionExpression) -> bool { - match node.parent::() { - Some(parent) => { - matches!( - parent.syntax().kind(), - JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION - | JsSyntaxKind::JS_COMPUTED_MEMBER_EXPRESSION - | JsSyntaxKind::JS_CALL_EXPRESSION - ) - } - None => false, - } + use AnyJsExpression::*; + + matches!( + node.parent::(), + Some(JsStaticMemberExpression(_) | JsComputedMemberExpression(_) | JsCallExpression(_)) + ) }