feat(rome_js_analyze): noVoidTypeReturn
#3806
Conversation
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
I see how this rule can be useful in catching bugs, but it may be controversial because using return iReturnVoid() can be useful.
But I think we can add it as a nursery rule and see if users find it useful.
I recommend adding support for the void operator that explicitly returns void.
function f(): void {
return void 0;
}
crates/rome_js_analyze/src/analyzers/nursery/no_void_type_return.rs
Outdated
Show resolved
Hide resolved
| declare_node_union! { | ||
| pub(crate) JsFunctionMethod = JsArrowFunctionExpression | JsFunctionDeclaration | JsFunctionExportDefaultDeclaration | JsFunctionExpression | JsMethodClassMember | JsMethodObjectMember | ||
| } |
There was a problem hiding this comment.
You also want to handle getter and setter methods.
There was a problem hiding this comment.
setters cannot be handled because they cannot be annotated with a return type. I added getters. I noted that getters have a different interface for the return type. Is this intentional?
There was a problem hiding this comment.
That's fair, but doesn't that mean that a return with a non-void argument in a setter is always an error?
I noted that getters have a different interface for the return type. Is this intentional?
The reason is that getters are more limited regarding the return type annotation.
There was a problem hiding this comment.
That's fair, but doesn't that mean that a return with a non-void argument in a setter is always an error?
I suppose. This is why we have noSetterReturn. Do you suggest adding the check of noSetterReturn, and certainly noCOntrsutorReturn, in this rule? I first hesitated. However, I think it is better to keep distinct rules: this one relies on type annotation, while the others rely on constructor identifier/set keyword.
The reason is that getters are more limited regarding the return type annotation.
This makes sense. However, it is often simpler to handle types with uniform interfaces. I could open a discussion about that?
crates/rome_js_analyze/src/analyzers/nursery/no_void_type_return.rs
Outdated
Show resolved
Hide resolved
Conaclos
requested review from
ematipico
and removed request for
xunilrj and
leops
I always find it a bit confusing in unfamiliar codebases if I don't know the function already. I'll check the tooltip of the function to see if it returns something despite having a name that sounds a lot like it returns void. From the existing rules in Rome core and even in the recommended set I'd say there are more opinionated rules than that already. Plus style will at least be consistent across a large codebase and not sometimes So all in all would argue that's a good thing to forbid by default. |
* 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
This is an exclusive rule for Rome!
This disallows returning a value when the return-type of a function is
void.Test Plan
Unit tests and doc-tests included