Closed samhh closed 5 months ago
I don't understand what you mean.
If I understand correctly, the code is invalid because you use the namespace as a type.
This invalid code could be reported by a rule. Why this code should be reported by noUnusedImports
?
This invalid code could be reported by a rule. Why this code should be reported by
noUnusedImports
?
I believe what OP wants to say is that this doesn't necessarily make the code invalid because we can define an ambient type with the same name in some other .d.ts
file, which will make the name valid in a type position. And therefore the unused imported namespace should be reported by noUnusedImports
.
I believe what OP wants to say is that this doesn't necessarily make the code invalid because we can define an ambient type with the same name in some other
.d.ts
file, which will make the name valid in a type position. And the imported unused namespace should be reported bynoUnusedImports
.
Thanks for the clarification. It makes sense. This is related to how the semantic model binds references to declarations.
However, taking a look at the TypeScript playground, it seems that TypeScript binds Option<string>
to the Option
namespace.
I am unsure what we should do in this case because our semantic model matches the TypeScript semantic model in this particular case.
Some interesting findings:
In VSCode with the TypeScript language server on, if the ambient type is properly defined and discovered, the language server will resolve the type correctly (to the ambient one) and will mark the imported namespace as unused.
But if there isn't an ambient type or it is not discovered, it will fail to resolve the type and issue an error. Meanwhile the imported namespace is still marked as unused.
[!NOTE] In order for that ambient type to be discovered by the language server, one can either 1) open that ambient type definition file in the editor, 2) use the tripple slash directive
/// <reference types="..." />
.
However, the TypeScript AST viewer doesn't support a multi-file set-up, so we cannot see for sure whether introducing an ambient type will change the semantic model.
Anyway, it seems to me that project-wise type resolution is involved in the analysis so this might not be a semantic model only issue.
I also came up with another simpler example without the need of ambient types to demonstrate the issue:
import * as A from "./module";
type A = string;
export const a: A = "";
And interestingly, the TypeScript language server will not mark the namespace import as unused and instead it will mark it as the type alias defined below. This seems a bug of TypeScript to me:
Biome, however, can correctly mark the namespace import as unused with the above code: https://biomejs.dev/playground/?lintRules=all&code=aQBtAHAAbwByAHQAIAAqACAAYQBzACAAQQAgAGYAcgBvAG0AIAAiAC4ALwBtAG8AZAB1AGwAZQAiADsACgAKAHQAeQBwAGUAIABBACAAPQAgAHMAdAByAGkAbgBnADsACgAKAGUAeABwAG8AcgB0ACAAYwBvAG4AcwB0ACAAYQA6ACAAQQAgAD0AIAAiACIAOwA%3D, but it will trigger a false positive of lint/suspicious/noRedeclare.
@Sec-ant Thanks for the investigation ā¤ļø
I think we could improve the semantic model in order to not bind a reference in an ambient context to an import namespace.
This means that in the following code, Ns
will not reference the import namespace Ns
, i twill be reported as an unresolved reference.
import * as Ns from ""
type X = Ns; // unresolved ref
Note that we should still allow namespace access like:
import * as Ns from ""
type X = Ns.Type; // valid, here `Ns` is bound to the import namesapce `Ns`
Environment information
Rule name
noUnusedImports
Playground link
https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB0AHkAcABlACAAKgAgAGEAcwAgAEEAbQBiAGkAZQBuAHQAIABmAHIAbwBtACAAJwAuAC8AbQBvAGQAdQBsAGUALgBqAHMAJwAKAAoAdAB5AHAAZQAgAEIAYQByACAAPQAgAEEAbQBiAGkAZQBuAHQA
Expected result
The provided snippet cannot possibly typecheck unless
Ambient
is an ambient type, because theAmbient
we import is a namespace. Therefore the lint rule should flag the namespace import as unused. The same applies toimport *
, where it's semi-correctly flagged as only being used in a type context byuseImportType
. Arguably it should be fixed in the context of each as it seems like a parser issue.In practice this is an issue in codebases making use of ambient types and namespace imports, such as some fp-ts/similar codebases. For example:
An ambient somewhere to avoid repetitive non-namespaced type imports:
And a module somewhere:
Code of Conduct