🐛 noUnusedVariables fails to detect class exports if there is an interface with the same name as the exported class

[In3luki]

Environment information

CLI:
  Version:                      12.0.0
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  ROME_LOG_DIR:                 unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.12.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/9.2.0"

Rome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    true

Workspace:
  Open Documents:               0

Discovering running Rome servers...

What happened?

Minimal reproduction:

class MyClass {}

interface MyClass {
    foo: string;
}

export { MyClass };

It works fine without the interface:

class MyClass {}

export { MyClass };

Expected result

It should not throw an error.

Code of Conduct

• I agree to follow Rome's Code of Conduct

[unvalley]

I'll fix this.

[ematipico]

@unvalley are you still interested? let us know if you need any help

[unvalley]

@ematipico
Thanks, I was a little busy in April, I wrote about what I was thinking and it would be helpful if you could take a look.

• A file to be changed: https://github.com/rome/tools/blob/main/crates/rome_js_semantic/src/events.rs

In conclusion, what I am struggling with is the question of how to allow the existence of a class and interface when they are bound by the same name, without treating them as shadows.

The determination of whether a certain binding has been exported is made in the SemanticEventExtractor.pop_scope() method. Here, assuming that the name of the binding is unique, the name is used as a key to retrieve the binding from the bindings property of the SemanticEventExtractor.

tools/crates/rome_js_semantic/src/events.rs

Line 619 in 1d86eec

if let Some(declaration_at) = self.bindings.get(&name) {

In this case, the declaration_at obtained from the name we get here is expected to be multiple values. However, as you can see from the definition of bindings, it currently returns only a single value. This needs to be fixed.

As a solution, I’m considering changing the value of bindings in SemanticEventExtractor as follows:

#[derive(Default)]
pub struct SemanticEventExtractor {
    stash: VecDeque<SemanticEvent>,
    scopes: Vec<Scope>,
    next_scope_id: usize,
    /// ~~
    /// Changed: the value TextRange to Vec<BindingInfo>
    bindings: FxHashMap<SyntaxTokenText, Vec<BindingInfo>>,
}

struct BindingInfo {
    text_range: TextRange,
    syntax_kind: JsSyntaxKind,
}

It seems to me that if we look at the SyntaxKind and only allow it to push to the value vector of bindings if it is a class or interface, we can avoid the problem of conflicts with shadowed. (I plan to allow push to vector only in cases where interface and class are bound with the same name, just like this one. So, the handling is only for such a case, which seems to be a bit wasteful in memory usage..)




If you have any suggestions or tips, please let me know.

[ematipico]

Your suggested change makes sense to me. It's a bit weird because, from a pure JavaScript point of view, because a binding should be declared only once, but here we are also working with TypeScript, so it makes sense to change the idea of the semantic model.

Regarding the technical solution, it makes sense. There's to consider that this issue is not only related to classes but variables too:

const a = "";

interface a {};

export { a }