feat(rome_js_analyze): add noExtraNonNullAssertion rule #3797
Conversation
kaioduarte
requested review from
leops,
ematipico and
xunilrj
as code owners
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
| } | ||
|
|
||
| impl Rule for NoExtraNonNullAssertion { | ||
| type Query = Ast<TsNonNullAssertionExpression>; |
There was a problem hiding this comment.
Rome also has a TsNonNullAssertionAssignment for null assertions in assignment positions:
a!!.b = null
There was a problem hiding this comment.
I added a test to cover this case and it's working with the current implementation.
There was a problem hiding this comment.
Right, because it is in the object path. This example should work:
a!!! = null
There was a problem hiding this comment.
Thanks for the example! I handled that on f434c37.
crates/rome_js_analyze/src/analyzers/nursery/no_extra_non_null_assertion.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/no_extra_non_null_assertion.rs
Outdated
Show resolved
Hide resolved
| fn has_extra_non_null_assertion(expression: JsAnyExpression) -> bool { | ||
| match expression { | ||
| JsAnyExpression::TsNonNullAssertionExpression(_) => return true, | ||
| JsAnyExpression::JsStaticMemberExpression(static_member_exp) => { |
There was a problem hiding this comment.
This function should have a similar branch for JsComputedMemberExpression nodes with an optional_chain_token
There was a problem hiding this comment.
Actually, the optional chain check doesn't work well in this case because it checks the entire expression, not only what's inside the brackets, invalidating this case 😕
obj?.[key!]There was a problem hiding this comment.
Ah right, in order to correctly detect the case where the expressions are nested as obj!?.[key] we could check that computed_member_expr.object() == query to ignore the case where the non-null assertion is nested in the member position
There was a problem hiding this comment.
Checking the object also can create a false positive, like:
a![b]Also, the ESLint rule doesn't emit a diagnostic for the above example. Maybe it's okay to ignore that branch?
kaioduarte
force-pushed
the
feat/no-extra-non-null-assertion
branch
4 times, most recently
from
5186531
to
f434c37
Compare
kaioduarte
force-pushed
the
feat/no-extra-non-null-assertion
branch
from
f8d9b07
to
5d00415
Compare
* upstream/main: (73 commits) fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789) feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791) chore: run rustfmt and typo fix (rome#3840) feat(rome_js_analyze): use exhaustive deps support properties (rome#3581) website(docs): Fix text formatting (rome#3828) feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806) feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812) fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834) feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797) fix(rome_lsp): lsp friendly catch unwind (rome#3740) feat(rome_js_semantic): model improvements (rome#3825) feat(rome_json_parser): JSON Lexer (rome#3809) feat(rome_js_analyze): implement `noDistractingElements` (rome#3820) fix(rome_js_formatter): shothanded named import line break with default import (rome#3826) feat(rome_js_analyze): `noConstructorReturn` (rome#3805) feat(website): change enabledNurseryRules to All/Recommended select (rome#3810) feat(rome_js_analyze): noSetterReturn feat(rome_js_analyze): noConstructorReturn feat(rome_analyze): suppress rule via code actions (rome#3572) feat(rome_js_analyze): `noVar` (rome#3765) ...
Summary
Implement no-extra-non-null-assertion.
Test Plan
Unit tests