Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_semantic): correct the export determination when a variable and an interface had the same name #4468

Merged
merged 9 commits into from
May 16, 2023
Merged

fix(rome_js_semantic): correct the export determination when a variable and an interface had the same name #4468

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0f477f1
fix: correct the export determination caused by ts interface
unvalley May 10, 2023
4e37b09
chore: CHANGELOG.md
unvalley May 13, 2023
7e73b71
chore: typo
unvalley May 14, 2023
1f6a472
chore: add other changes to linter section
unvalley May 14, 2023
c1c1d53
refactor: updated property and added comment for DecalredBinding struct
unvalley May 14, 2023
85d6300
refactor: renamed to BiningInfo and added comment
unvalley May 14, 2023
c80655b
chore: add more comments
unvalley May 15, 2023
ce2deaa
refactor: comments
unvalley May 15, 2023
8f7bea2
Merge branch 'main' into fix-4353
unvalley May 15, 2023
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
### Editors
### Formatter
### Linter

- Fix an issue that [`noUnusedVariables`](https://docs.rome.tools/lint/rules/nounusedvariables/) rule did not correctly detect exports when a variable and an `interface` had the same name [#4468](https://github.com/rome/tools/pull/4468)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add the #### Other changes heading


### Parser
### VSCode
### JavaScript APIs
Expand Down
8 changes: 8 additions & 0 deletions ...rome_js_analyze/tests/specs/correctness/noUnusedVariables/validClassAndInterfaceExport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/* should not generate diagnostics */
class A {}

interface A {
foo: string;
}

export { A };
18 changes: 18 additions & 0 deletions ...js_analyze/tests/specs/correctness/noUnusedVariables/validClassAndInterfaceExport.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: validClassAndInterfaceExport.ts
---
# Input
```js
/* should not generate diagnostics */
class A {}

interface A {
foo: string;
}

export { A };

```


8 changes: 8 additions & 0 deletions crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariableAndExport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/* should not generate diagnostics */
const a = "";

interface a {
foo: string;
}

export { a };
18 changes: 18 additions & 0 deletions .../rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariableAndExport.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: validVariableAndExport.ts
---
# Input
```js
/* should not generate diagnostics */
const a = "";

interface a {
foo: string;
}

export { a };

```


106 changes: 79 additions & 27 deletions crates/rome_js_semantic/src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,20 @@ impl SemanticEvent {
/// }
/// }
/// ```
#[derive(Default)]
#[derive(Default, Debug)]
pub struct SemanticEventExtractor {
stash: VecDeque<SemanticEvent>,
scopes: Vec<Scope>,
next_scope_id: usize,
/// At any point this is the set of available bindings and
/// the range of its declaration
bindings: FxHashMap<SyntaxTokenText, TextRange>,
bindings: FxHashMap<SyntaxTokenText, DeclaredBinding>,
}

#[derive(Debug)]
struct DeclaredBinding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a brief documentation of the struct, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply renamed DeclaredBinding to BindingInfo. And I added some comment to it but the confusion is might be resolved by naming.

text_range: TextRange,
syntax_kind: JsSyntaxKind,
}

#[derive(Debug)]
Expand Down Expand Up @@ -205,7 +211,7 @@ struct Scope {
references: HashMap<SyntaxTokenText, Vec<Reference>>,
/// All bindings that where shadowed and will be
/// restored after this scope ends.
shadowed: Vec<(SyntaxTokenText, TextRange)>,
shadowed: Vec<(SyntaxTokenText, DeclaredBinding)>,
/// If this scope allows declarations to be hoisted
/// to parent scope or not
hoisting: ScopeHoisting,
Expand Down Expand Up @@ -368,32 +374,33 @@ impl SemanticEventExtractor {
};

let parent = node.parent()?;
match parent.kind() {
let parent_kind = parent.kind();
match parent_kind {
JS_VARIABLE_DECLARATOR => {
if let Some(true) = is_var {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(0);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
} else {
self.push_binding_into_scope(None, &name_token);
self.push_binding_into_scope(None, &name_token, &parent_kind);
};
self.export_variable_declarator(node, &parent);
}
JS_FUNCTION_DECLARATION | JS_FUNCTION_EXPORT_DEFAULT_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_function_declaration(node, &parent);
}
JS_FUNCTION_EXPRESSION => {
self.push_binding_into_scope(None, &name_token);
self.push_binding_into_scope(None, &name_token, &parent_kind);
self.export_function_expression(node, &parent);
}
JS_CLASS_DECLARATION | JS_CLASS_EXPORT_DEFAULT_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_declaration(node, &parent);
}
JS_CLASS_EXPRESSION => {
self.push_binding_into_scope(None, &name_token);
self.push_binding_into_scope(None, &name_token, &parent_kind);
self.export_class_expression(node, &parent);
}
JS_BINDING_PATTERN_WITH_DEFAULT
Expand All @@ -405,7 +412,7 @@ impl SemanticEventExtractor {
| JS_ARRAY_BINDING_PATTERN
| JS_ARRAY_BINDING_PATTERN_ELEMENT_LIST
| JS_ARRAY_BINDING_PATTERN_REST_ELEMENT => {
self.push_binding_into_scope(None, &name_token);
self.push_binding_into_scope(None, &name_token, &parent_kind);

let possible_declarator = parent.ancestors().find(|x| {
!matches!(
Expand All @@ -428,26 +435,26 @@ impl SemanticEventExtractor {
}
TS_TYPE_ALIAS_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_declaration(node, &parent);
}
TS_ENUM_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_declaration(node, &parent);
}
TS_INTERFACE_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_declaration(node, &parent);
}
TS_MODULE_DECLARATION => {
let hoisted_scope_id = self.scope_index_to_hoist_declarations(1);
self.push_binding_into_scope(hoisted_scope_id, &name_token);
self.push_binding_into_scope(hoisted_scope_id, &name_token, &parent_kind);
self.export_declaration(node, &parent);
}
_ => {
self.push_binding_into_scope(None, &name_token);
self.push_binding_into_scope(None, &name_token, &parent_kind);
}
}

Expand Down Expand Up @@ -616,38 +623,77 @@ impl SemanticEventExtractor {
// Match references and declarations
for (name, mut references) in scope.references {
// If we know the declaration of these reference push the correct events...
if let Some(declaration_at) = self.bindings.get(&name) {
for reference in references {
if let Some(declared_binding) = self.bindings.get(&name) {
let declaration_at = declared_binding.text_range;
let declaration_syntax_kind = declared_binding.syntax_kind;

for reference in &references {
let declaration_before_reference =
declaration_at.start() < reference.range().start();
let e = match (declaration_before_reference, &reference) {
(true, Reference::Read { range, .. }) => SemanticEvent::Read {
range: *range,
declared_at: *declaration_at,
declared_at: declaration_at,
scope_id: scope.scope_id,
},
(false, Reference::Read { range, .. }) => SemanticEvent::HoistedRead {
range: *range,
declared_at: *declaration_at,
declared_at: declaration_at,
scope_id: scope.scope_id,
},
(true, Reference::Write { range }) => SemanticEvent::Write {
range: *range,
declared_at: *declaration_at,
declared_at: declaration_at,
scope_id: scope.scope_id,
},
(false, Reference::Write { range }) => SemanticEvent::HoistedWrite {
range: *range,
declared_at: *declaration_at,
declared_at: declaration_at,
scope_id: scope.scope_id,
},
};
self.stash.push_back(e);

match reference {
Reference::Read { is_exported, .. } if is_exported => {
Reference::Read { is_exported, .. } if *is_exported => {
let find_exportable_declration = scope.shadowed.iter().find_map(
Conaclos marked this conversation as resolved.
Show resolved Hide resolved
|(shadowed_ident_name, shadowed_declration)| {
if shadowed_ident_name != &name {
return None;
}

match (
declaration_syntax_kind,
shadowed_declration.syntax_kind,
) {
(
JsSyntaxKind::JS_VARIABLE_DECLARATOR,
JsSyntaxKind::TS_INTERFACE_DECLARATION,
)
| (
JsSyntaxKind::TS_INTERFACE_DECLARATION,
JsSyntaxKind::JS_VARIABLE_DECLARATOR,
)
| (
JsSyntaxKind::JS_CLASS_DECLARATION,
JsSyntaxKind::TS_INTERFACE_DECLARATION,
)
| (
JsSyntaxKind::TS_INTERFACE_DECLARATION,
JsSyntaxKind::JS_CLASS_DECLARATION,
) => Some(shadowed_declration),
_ => None,
}
},
);
if let Some(decl) = find_exportable_declration {
self.stash.push_back(SemanticEvent::Exported {
range: decl.text_range,
});
}

self.stash.push_back(SemanticEvent::Exported {
range: *declaration_at,
range: declaration_at,
});
}
_ => {}
Expand All @@ -669,7 +715,6 @@ impl SemanticEventExtractor {
}

// Remove all bindings declared in this scope

for binding in scope.bindings {
self.bindings.remove(&binding.name);
}
Expand Down Expand Up @@ -747,16 +792,22 @@ impl SemanticEventExtractor {
&mut self,
hoisted_scope_id: Option<usize>,
name_token: &JsSyntaxToken,
parent_syntax_kind: &JsSyntaxKind,
) {
let name = name_token.token_text_trimmed();

let declaration_range = name_token.text_range();

// insert this name into the list of available names
// and save shadowed names to be used later
let shadowed = self
.bindings
.insert(name.clone(), declaration_range)
.insert(
name.clone(),
DeclaredBinding {
text_range: declaration_range,
syntax_kind: *parent_syntax_kind,
Copy link
Contributor

Choose a reason for hiding this comment

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

The name syntax_kind is a bit misleading because I thought we were saving the syntax kind of the node, but instead, we are passing the parent syntax kind. Should we change the name and document it a bit why we save the parent syntax kind?

Copy link
Contributor Author

@unvalley unvalley May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right. I also updated the property name from syntax_kind to declaration_kind. I think this change can resolve why the property is needed. How do you feel about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

},
)
.map(|shadowed_range| (name.clone(), shadowed_range));

let current_scope_id = self.current_scope_mut().scope_id;
Expand All @@ -774,6 +825,7 @@ impl SemanticEventExtractor {

scope.bindings.push(Binding { name: name.clone() });
scope.shadowed.extend(shadowed);

let scope_started_at = scope.started_at;

self.stash.push_back(SemanticEvent::DeclarationFound {
Expand Down