Skip to content
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
20 changes: 20 additions & 0 deletions .changeset/lovely-sloths-chew.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
"@biomejs/biome": patch
---

Improved the detection of the rule `noUnnecessaryConditions`. Now the rule isn't triggered for variables that are mutated inside a module.

This logic deviates from the original rule, hence `noUnnecessaryConditions` is now marked as "inspired".

In the following example, `hey` starts as `false`, but then it's assigned to a string. The rule isn't triggered inside the `if` check.

```js
let hey = false;

function test() {
hey = "string";
}

if (hey) {}

```
Comment thread
ematipico marked this conversation as resolved.
5 changes: 5 additions & 0 deletions .changeset/upset-cameras-walk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@biomejs/biome": patch
---

Improved the type inference engine, by resolving types for variables that are assigned to multiple values.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@ declare_lint_rule! {
/// }
/// ```
///
/// Contrary to the source rule, this rule doesn't trigger bindings that are assigned to multiple
/// values. In the following example, the variable `greeting` is assigned to multiple values; hence
/// it can't be inferred to a truthy or falsy value.
///
/// ```ts
/// let greeting = false;
///
/// function changeGreeting() {
/// greeting = "Hello World!"
/// }
///
/// if (greeting) {} // rule not triggered here
///
/// ```
///
///
/// ### Valid
///
/// ```ts
Expand Down Expand Up @@ -71,7 +87,7 @@ declare_lint_rule! {
version: "2.1.4",
name: "noUnnecessaryConditions",
language: "js",
sources: &[RuleSource::EslintTypeScript("no-unnecessary-condition").same()],
sources: &[RuleSource::EslintTypeScript("no-unnecessary-condition").inspired()],
recommended: false,
severity: Severity::Warning,
domains: &[RuleDomain::Project],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// should not generate diagnostics

let hey = false;

function test() {
hey = "string"
}

if (hey) {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
source: crates/biome_js_analyze/tests/spec_tests.rs
expression: validAssignment.ts
---
# Input
```ts
// should not generate diagnostics

let hey = false;

function test() {
hey = "string"
}

if (hey) {}

```
8 changes: 8 additions & 0 deletions crates/biome_js_type_info/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,14 @@ pub trait TypeResolver {
]))))))
}

/// Register a new type that is a union between `current_type` and `ty`
fn union_with(&mut self, current_type: TypeReference, ty: TypeReference) -> TypeId {
self.register_type(Cow::Owned(TypeData::Union(Box::new(Union(Box::new([
current_type,
ty,
]))))))
}

// #endregion
}

Expand Down
74 changes: 65 additions & 9 deletions crates/biome_module_graph/src/js_module_info/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ use std::{borrow::Cow, collections::BTreeSet, sync::Arc};
use biome_js_semantic::{SemanticEvent, SemanticEventExtractor};
use biome_js_syntax::{
AnyJsCombinedSpecifier, AnyJsDeclaration, AnyJsExportDefaultDeclaration, AnyJsExpression,
AnyJsImportClause, JsForVariableDeclaration, JsFormalParameter, JsIdentifierBinding,
JsRestParameter, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken, JsVariableDeclaration,
TsIdentifierBinding, TsTypeParameter, TsTypeParameterName, inner_string_text,
AnyJsImportClause, JsAssignmentExpression, JsForVariableDeclaration, JsFormalParameter,
JsIdentifierBinding, JsRestParameter, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken,
JsVariableDeclaration, TsIdentifierBinding, TsTypeParameter, TsTypeParameterName,
inner_string_text,
};
use biome_js_type_info::{
BindingId, FunctionParameter, GLOBAL_RESOLVER, GLOBAL_UNKNOWN_ID, GenericTypeParameter,
Expand Down Expand Up @@ -573,34 +574,58 @@ impl JsModuleInfoCollector {
for index in 0..self.bindings.len() {
let binding = &self.bindings[index];
if let Some(node) = self.binding_node_by_start.get(&binding.range.start()) {
let name = binding.name.clone();
let scope_id = scope_id_for_range(scope_by_range, binding.range);
let ty = self.infer_type(&node.clone(), &name, scope_id);
let ty = self.infer_type(&node.clone(), binding.clone(), scope_id);
self.bindings[index].ty = ty;
}
}
}

fn has_writable_reference(&self, binding: &JsBindingData) -> bool {
binding
.references
.iter()
.any(|reference| reference.is_write())
}

fn get_writable_references(&self, binding: &JsBindingData) -> Vec<JsBindingReference> {
binding
.references
.iter()
.filter(|reference| reference.is_write())
.cloned()
.collect()
}

fn infer_type(
&mut self,
node: &JsSyntaxNode,
binding_name: &Text,
binding: JsBindingData,
scope_id: ScopeId,
) -> TypeReference {
let binding_name = &binding.name.clone();
for ancestor in node.ancestors() {
if let Some(decl) = AnyJsDeclaration::cast_ref(&ancestor) {
return if let Some(typed_bindings) = decl
let ty = if let Some(typed_bindings) = decl
.as_js_variable_declaration()
.and_then(|decl| self.variable_declarations.get(decl.syntax()))
{
typed_bindings
let ty = typed_bindings
.iter()
.find_map(|(name, ty)| (name == binding_name).then(|| ty.clone()))
.unwrap_or_default()
.unwrap_or_default();

if self.has_writable_reference(&binding) {
self.widen_binding_from_writable_references(scope_id, &binding, &ty)
} else {
ty
}
} else {
let data = TypeData::from_any_js_declaration(self, scope_id, &decl);
self.reference_to_owned_data(data)
};

return ty;
} else if let Some(declaration) = AnyJsExportDefaultDeclaration::cast_ref(&ancestor) {
let data =
TypeData::from_any_js_export_default_declaration(self, scope_id, &declaration);
Expand Down Expand Up @@ -640,6 +665,37 @@ impl JsModuleInfoCollector {
TypeReference::unknown()
}

/// Widen the type of binding from its writable references.
fn widen_binding_from_writable_references(
&mut self,
scope_id: ScopeId,
binding: &JsBindingData,
ty: &TypeReference,
) -> TypeReference {
let references = self.get_writable_references(binding);
let mut ty = ty.clone();
for reference in references {
let Some(node) = self.binding_node_by_start.get(&reference.range_start) else {
continue;
};
for ancestor in node.ancestors().skip(1) {
if let Some(assignment) = JsAssignmentExpression::cast_ref(&ancestor)
&& let Ok(right) = assignment.right()
{
let data = TypeData::from_any_js_expression(self, scope_id, &right);
let assigned_type = self.reference_to_owned_data(data);
ty = ResolvedTypeId::new(
self.level(),
self.union_with(ty.clone(), assigned_type),
)
.into();
}
}
}

ty
}

/// After the first pass of the collector, import references have been
/// resolved to an import binding. But we can't store the information of the
/// import target inside the `ResolvedTypeId`, because it resides in the
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
---
source: crates/biome_module_graph/tests/snap/mod.rs
expression: content
---
# `index.ts` (Not imported by resolver)

## Source

```ts
let hey = false;
function f() {
hey = true;
}
```

## Module Info

```
Exports {
No exports
}
Imports {
No imports
}
```

## Registered types

```
Module TypeId(0) => bool: false

Module TypeId(1) => bool: true

Module TypeId(2) => unknown

Module TypeId(3) => Module(0) TypeId(0) | Module(0) TypeId(1)

Module TypeId(4) => void

Module TypeId(5) => sync Function "f" {
accepts: {
params: []
type_args: []
}
returns: Module(0) TypeId(4)
}
```

# Module Resolver

## Registered types

```
Full TypeId(0) => bool: false

Full TypeId(1) => bool: true

Full TypeId(2) => unknown

Full TypeId(3) => Module(0) TypeId(0) | Module(0) TypeId(1)

Full TypeId(4) => void

Full TypeId(5) => sync Function "f" {
accepts: {
params: []
type_args: []
}
returns: Module(0) TypeId(4)
}
```
Loading