-
-
Notifications
You must be signed in to change notification settings - Fork 504
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.
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
feat(biome_js_analyzer): implement noImplicitAnyLet #395
Conversation
Maybe |
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.
We should also add some tests in a JavaScript file to ensure that the rule does not affect JavaScript files.
/// Typescript variable declaration without any `type` or `initialization` can cause issue later in the code. | ||
/// | ||
/// |
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.
I could add more details:
/// Typescript variable declaration without any `type` or `initialization` can cause issue later in the code. | |
/// | |
/// | |
/// TypeScript variable declaration without any type annotation and initialization have the `any` type. | |
/// The any type in TypeScript is a dangerous “escape hatch” from the type system. | |
/// Using any disables many type checking rules and is generally best used only as a last resort or when prototyping code. | |
/// TypeScript’s `--noImplicitAny` compiler option doesn't report this case. |
use biome_js_syntax::{JsFileSource, JsVariableDeclaration, JsVariableDeclarator}; | ||
|
||
declare_rule! { | ||
/// Restrict use of implicit any type in Typescript. |
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.
/// Restrict use of implicit any type in Typescript. | |
/// Disallow use of implicit `any` type on variable declarations. |
rule_category!(), | ||
variable.text_range(), | ||
markup! { | ||
"Variable " <Emphasis>{variable.text()}</Emphasis> " has implicitly " <Emphasis>"any"</Emphasis> " type" |
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.
I think we can remove the variable name.
"Variable " <Emphasis>{variable.text()}</Emphasis> " has implicitly " <Emphasis>"any"</Emphasis> " type" | |
"This variable has implicitly the " <Emphasis>"any"</Emphasis> " type." |
}, | ||
) | ||
.note(markup! { | ||
"Declare type or initialize the variable with some value" |
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.
We should also explain why the variable has implicitly the any
type before suggesting a way to fix it.
"Declare type or initialize the variable with some value" | |
"Variable declarations without type annotation and initialization have implicitly the "<Emphasis>"any"</Emphasis>" type. Declare type or initialize the variable with some value." |
var someVar1; | ||
someVar1 = '123'; | ||
someVar1 = 123; | ||
|
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.
Could you add some example with for loops, and multiple declarators?
e.g.
let x = 0, y, z = 0;
var x = 0, y, z = 0;
for(let a = 0, b; a < 5; a++) {}
@@ -0,0 +1,6 @@ | |||
/* should not generate diagnostics */ | |||
|
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.
We should add const
, using
and for-of
examples to ensure that they are ignored.
e.g.
const x = 0;
for(let y of xs) {}
using z = f();
We can keep the rule as recommended. Thus, we have to fix the website. If it is too much work to do, then set |
/// ```ts,expect_diagnostic | ||
/// var a; | ||
/// a = 2; | ||
/// let b; | ||
/// b = 1 |
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.
Each snippet in the invalid section must raise only one diagnostic, this means that you'd need to break down this snippet in four code blocks with expect_diagnostic
directive.
The valid section doesn't have this restriction.
@b4s36t4 Are you still interested in completing this contribution? |
@Conaclos Yes, been sick past few days. Will complete this by End of this week. |
Friendly ping @b4s36t4, are you still interested? |
@Conaclos @ematipico Can I take over this pull request? cc: @Conaclos |
Sure, but please keep the commits of @b4s36t4 You can see |
@ematipico Yes. I will also take over commits with "git cherry-pick". |
Implements #389
Summary
Creates a new rule to restrict the usage of variables without any type or initialisation in TS
More here -> https://www.typescriptlang.org/tsconfig#noImplicitAny
Test Plan
Wrote test cases for valid and invalid situation, inspired form here #381