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

fix(analyzer): False positive in rule noUndeclaredVariables #243 #350

Merged
merged 3 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
### JavaScript APIs
### Linter

#### Bug fixes

- Fix [#243](https://github.com/biomejs/biome/issues/243) a false positive case where the incorrect scope was defined for the `infer` type. in rule [noUndeclaredVariables](https://biomejs.dev/linter/rules/no-undeclared-variables/). Contributed by @denbezrukov

#### New features

- Add [noMisleadingInstantiator](https://biomejs.dev/linter/rules/no-mileading-instantiator) rule. The rule reports the misleading use of the `new` and `constructor` methods. Contributed by @unvalley
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export type WithSelectors<S> = S extends { getState: () => infer T }
? S & { use: { [K in keyof T]: () => T[K] } }
: never;

type A = number extends infer T ? T : never;
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
source: crates/biome_js_analyze/tests/spec_tests.rs
expression: infer.ts
---
# Input
```js
export type WithSelectors<S> = S extends { getState: () => infer T }
? S & { use: { [K in keyof T]: () => T[K] } }
: never;

type A = number extends infer T ? T : never;

```


Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
type A = number extends infer T ? never : T;
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
source: crates/biome_js_analyze/tests/spec_tests.rs
expression: infer_incorrect.ts
---
# Input
```js
type A = number extends infer T ? never : T;

```

# Diagnostics
```
infer_incorrect.ts:1:43 lint/correctness/noUndeclaredVariables ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! The T variable is undeclared

> 1 │ type A = number extends infer T ? never : T;
│ ^
2 │


```


64 changes: 59 additions & 5 deletions crates/biome_js_semantic/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

use rustc_hash::FxHashMap;
use std::collections::{HashMap, VecDeque};
use std::mem;

use biome_js_syntax::AnyTsType;
use biome_js_syntax::{
AnyJsAssignment, AnyJsAssignmentPattern, AnyJsExpression, JsAssignmentExpression,
JsCallExpression, JsForVariableDeclaration, JsIdentifierAssignment, JsIdentifierBinding,
Expand Down Expand Up @@ -162,6 +164,7 @@ pub struct SemanticEventExtractor {
/// At any point this is the set of available bindings and
/// the range of its declaration
bindings: FxHashMap<TokenText, BindingInfo>,
infers: Vec<TsTypeParameterName>,
}

/// Holds the text range of the token when it is bound,
Expand Down Expand Up @@ -247,6 +250,7 @@ impl SemanticEventExtractor {
scopes: vec![],
next_scope_id: 0,
bindings: FxHashMap::default(),
infers: vec![],
}
}

Expand Down Expand Up @@ -304,7 +308,6 @@ impl SemanticEventExtractor {
| TS_INTERFACE_DECLARATION
| TS_ENUM_DECLARATION
| TS_TYPE_ALIAS_DECLARATION
| TS_FUNCTION_TYPE
| TS_DECLARE_FUNCTION_DECLARATION => {
self.push_scope(
node.text_range(),
Expand All @@ -321,8 +324,11 @@ impl SemanticEventExtractor {
false,
);
}

_ => {}
_ => {
if let Some(node) = AnyTsType::cast_ref(node) {
self.enter_any_type(&node);
}
}
}
}

Expand All @@ -348,6 +354,18 @@ impl SemanticEventExtractor {
Some(is_var)
}

fn enter_any_type(&mut self, node: &AnyTsType) {
if node.in_conditional_true_type() {
self.push_conditional_true_scope(node);
} else if let Some(node) = node.as_ts_function_type() {
self.push_scope(
node.syntax().text_range(),
ScopeHoisting::DontHoistDeclarationsToParent,
false,
);
}
}

fn enter_identifier_binding(&mut self, node: &JsSyntaxNode) -> Option<()> {
use JsSyntaxKind::*;
debug_assert!(matches!(
Expand All @@ -371,6 +389,13 @@ impl SemanticEventExtractor {
TS_TYPE_PARAMETER_NAME => {
let token = node.clone().cast::<TsTypeParameterName>()?;
let name_token = token.ident_token().ok()?;

if token.in_infer_type() {
self.infers.push(token);

return None;
}

let is_var = Some(false);
(name_token, is_var)
}
Expand Down Expand Up @@ -562,15 +587,26 @@ impl SemanticEventExtractor {
| JS_CATCH_CLAUSE
| JS_STATIC_INITIALIZATION_BLOCK_CLASS_MEMBER
| TS_DECLARE_FUNCTION_DECLARATION
| TS_FUNCTION_TYPE
| TS_INTERFACE_DECLARATION
| TS_ENUM_DECLARATION
| TS_TYPE_ALIAS_DECLARATION
| TS_MODULE_DECLARATION
| TS_EXTERNAL_MODULE_DECLARATION => {
self.pop_scope(node.text_range());
}
_ => {}
_ => {
if let Some(node) = AnyTsType::cast_ref(node) {
self.leave_any_type(&node);
}
}
}
}

fn leave_any_type(&mut self, node: &AnyTsType) {
if node.in_conditional_true_type() {
self.pop_scope(node.syntax().text_range());
} else if let Some(node) = node.as_ts_function_type() {
self.pop_scope(node.syntax().text_range());
}
}

Expand All @@ -580,6 +616,24 @@ impl SemanticEventExtractor {
self.stash.pop_front()
}

fn push_conditional_true_scope(&mut self, node: &AnyTsType) {
self.push_scope(
node.syntax().text_range(),
ScopeHoisting::DontHoistDeclarationsToParent,
false,
);

let infers = mem::take(&mut self.infers);
for infer in infers {
let name_token = infer.ident_token().ok();
let parent_kind = infer.syntax().parent().map(|parent| parent.kind());

if let (Some(name_token), Some(parent_kind)) = (name_token, parent_kind) {
self.push_binding_into_scope(None, &name_token, &parent_kind);
}
}
}

fn push_scope(&mut self, range: TextRange, hoisting: ScopeHoisting, is_closure: bool) {
let scope_id = self.next_scope_id;
self.next_scope_id += 1;
Expand Down
2 changes: 1 addition & 1 deletion crates/biome_js_semantic/src/semantic_model/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub(crate) struct SemanticModelScopeData {
pub(crate) children: Vec<usize>,
// All bindings of this scope (points to SemanticModelData::bindings)
pub(crate) bindings: Vec<usize>,
// Map pointing to the [bindings] vec of each bindings by its name
// Map pointing to the [bindings] vec of each bindings by its name
pub(crate) bindings_by_name: FxHashMap<TokenText, usize>,
// All read references of a scope
pub(crate) read_references: Vec<SemanticModelScopeReference>,
Expand Down
8 changes: 8 additions & 0 deletions crates/biome_js_semantic/src/tests/infer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
use crate::assert_semantics;

assert_semantics! {
ok_conditional_true_type_scope, "type A<T> = T extends string ? (/*START Scope*/number)/*END Scope*/: boolean;",
ok_conditional_true_type_infer_simple, "type A<T> = T extends infer /*@ Scope */T ? (/*START Scope*/number)/*END Scope*/: boolean;",
ok_conditional_true_type_infer_function, "type A<T> = T extends { getState: () => infer /*@ Scope */ T } ? (/*START Scope*/number)/*END Scope*/: boolean;",
ok_conditional_true_type_infer_nested, "type A = MyType extends (OtherType extends infer /*@ InnerScope */T ? (/*START InnerScope*/infer /*@ OuterScope */ U)/*END InnerScope*/ : InnerFalse) ? (/*START OuterScope*/OuterTrue)/*END OuterScope*/ : OuterFalse;",
}
1 change: 1 addition & 0 deletions crates/biome_js_semantic/src/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod assertions;
pub mod declarations;
mod functions;
mod infer;
mod references;
mod scopes;

Expand Down
4 changes: 2 additions & 2 deletions crates/biome_js_semantic/src/tests/scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ assert_semantics! {
ok_scope_static_initialization_block,
"class A {
static/*START A*/ {
const a/*@ A*/ = 2;
}/*END A*/
const a/*@ A*/ = 2;
}/*END A*/
};",
}

Expand Down
57 changes: 56 additions & 1 deletion crates/biome_js_syntax/src/type_ext.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use biome_rowan::AstNode;
use std::iter;

use crate::AnyTsType;
use crate::{AnyTsType, TsConditionalType, TsInferType, TsTypeParameterName};

impl AnyTsType {
/// Try to extract non `TsParenthesizedType` from `AnyTsType`
Expand Down Expand Up @@ -77,4 +78,58 @@ impl AnyTsType {
| AnyTsType::TsStringType(_)
)
}

/// Checks if `self` stands as the `true_type` of a conditional type in Typescript.
///
/// # Examples
///
/// ```rust
/// use biome_js_factory::make;
/// use biome_js_syntax::T;
/// use biome_js_syntax::AnyTsType;
///
/// let check_type = AnyTsType::TsNumberType(make::ts_number_type(make::token(T![number])));
/// let extends_type = AnyTsType::TsNumberType(make::ts_number_type(make::token(T![number])));
/// let true_type = AnyTsType::TsNumberType(make::ts_number_type(make::token(T![number])));
/// let false_type = AnyTsType::TsNumberType(make::ts_number_type(make::token(T![number])));
///
/// let conditional = make::ts_conditional_type(
/// check_type,
/// make::token(T![extends]),
/// extends_type,
/// make::token(T![?]),
/// true_type,
/// make::token(T![:]),
/// false_type,
/// );
///
/// assert!(!conditional.check_type().unwrap().in_conditional_true_type());
/// assert!(!conditional.extends_type().unwrap().in_conditional_true_type());
/// assert!(conditional.true_type().unwrap().in_conditional_true_type());
/// assert!(!conditional.false_type().unwrap().in_conditional_true_type());
/// ```
pub fn in_conditional_true_type(&self) -> bool {
self.parent::<TsConditionalType>()
.and_then(|parent| parent.true_type().ok())
.map_or(false, |ref true_type| true_type == self)
}
}

impl TsTypeParameterName {
/// Checks if `self` is the type being inferred in a TypeScript `TsInferType`.
///
/// # Examples
///
/// ```rust
/// use biome_js_factory::make;
/// use biome_js_syntax::T;
///
/// let infer = make::ts_infer_type(make::token(T![infer]), make::ts_type_parameter_name(make::ident("T"))).build();
/// assert!(infer.name().unwrap().in_infer_type());
/// ```
pub fn in_infer_type(&self) -> bool {
self.parent::<TsInferType>()
.and_then(|parent| parent.name().ok())
.map_or(false, |ref name| name == self)
}
}
4 changes: 4 additions & 0 deletions website/src/content/docs/internals/changelog.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
### JavaScript APIs
### Linter

#### Bug fixes

- Fix [#243](https://github.com/biomejs/biome/issues/243) a false positive case where the incorrect scope was defined for the `infer` type. in rule [noUndeclaredVariables](https://biomejs.dev/linter/rules/no-undeclared-variables/). Contributed by @denbezrukov

#### New features

- Add [noMisleadingInstantiator](https://biomejs.dev/linter/rules/no-mileading-instantiator) rule. The rule reports the misleading use of the `new` and `constructor` methods. Contributed by @unvalley
Expand Down
Loading