Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

b4s36t4
Copy link
Contributor

@b4s36t4 b4s36t4 commented Sep 23, 2023

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

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Sep 23, 2023
@b4s36t4
Copy link
Contributor Author

b4s36t4 commented Sep 23, 2023

Maybe recommended should be false for the rule? as the website is raising the lint errors 🤓.

Copy link
Member

@Conaclos Conaclos left a 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.

Comment on lines +8 to +10
/// Typescript variable declaration without any `type` or `initialization` can cause issue later in the code.
///
///
Copy link
Member

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:

Suggested change
/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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"
Copy link
Member

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.

Suggested change
"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"
Copy link
Member

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.

Suggested change
"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;

Copy link
Member

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 */

Copy link
Member

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();

@Conaclos
Copy link
Member

Maybe recommended should be false for the rule? as the website is raising the lint errors 🤓.

We can keep the rule as recommended. Thus, we have to fix the website. If it is too much work to do, then set recommended to false :)

Comment on lines +18 to +22
/// ```ts,expect_diagnostic
/// var a;
/// a = 2;
/// let b;
/// b = 1
Copy link
Member

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.

@Conaclos
Copy link
Member

@b4s36t4 Are you still interested in completing this contribution?

@b4s36t4
Copy link
Contributor Author

b4s36t4 commented Oct 16, 2023

@Conaclos Yes, been sick past few days. Will complete this by End of this week.

@ematipico
Copy link
Member

Friendly ping @b4s36t4, are you still interested?

@TaKO8Ki
Copy link
Contributor

TaKO8Ki commented Nov 13, 2023

@Conaclos @ematipico Can I take over this pull request? cc: @Conaclos

@ematipico
Copy link
Member

@Conaclos @ematipico Can I take over this pull request? cc: @Conaclos

Sure, but please keep the commits of @b4s36t4

You can see git cherry-pick

@TaKO8Ki
Copy link
Contributor

TaKO8Ki commented Nov 13, 2023

@ematipico Yes. I will also take over commits with "git cherry-pick".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants