biome icon indicating copy to clipboard operation
biome copied to clipboard

πŸ’… Unused imports when the import is used inside JSDoc syntax

Open midnightveil opened this issue 1 year ago β€’ 9 comments

Environment information

CLI:
  Version:                      1.9.4
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_PATH:               unset
  BIOME_LOG_PREFIX_NAME:        unset
  BIOME_CONFIG_PATH:            unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           unset
  JS_RUNTIME_NAME:              unset
  NODE_PACKAGE_MANAGER:         unset

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

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  true
  GraphQL enabled:              false
  Recommended:                  true
  All:                          false
  Enabled rules:
  performance/noDelete
  suspicious/noCatchAssign
  suspicious/noUnsafeNegation
  complexity/useLiteralKeys
  suspicious/noClassAssign
  style/useImportType
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessEmptyExport
  suspicious/useNamespaceKeyword
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  suspicious/noAssignInExpressions
  a11y/useAriaActivedescendantWithTabindex
  suspicious/noDuplicateParameters
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noDuplicateSelectorsKeyframeBlock
  correctness/noUnknownProperty
  correctness/noUnusedLabels
  complexity/noUselessTernary
  correctness/noUnreachableSuper
  suspicious/noCompareNegZero
  suspicious/noExplicitAny
  correctness/noSwitchDeclarations
  a11y/noAutofocus
  correctness/noUnsafeOptionalChaining
  correctness/noConstAssign
  suspicious/noControlCharactersInRegex
  complexity/noUselessTypeConstraint
  style/noVar
  suspicious/noDoubleEquals
  suspicious/noRedundantUseStrict
  style/useLiteralEnumMembers
  suspicious/noGlobalIsNan
  suspicious/noEmptyInterface
  suspicious/noConstEnum
  suspicious/noMisleadingCharacterClass
  correctness/noPrecisionLoss
  a11y/noLabelWithoutControl
  suspicious/noRedeclare
  correctness/noStringCaseMismatch
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  suspicious/noFallthroughSwitchClause
  suspicious/noUnsafeDeclarationMerging
  complexity/noUselessThisAlias
  correctness/noUnreachable
  a11y/useKeyWithClickEvents
  suspicious/noDuplicateObjectKeys
  complexity/noThisInStatic
  complexity/useOptionalChain
  correctness/noInnerDeclarations
  suspicious/noDuplicateCase
  a11y/useValidAnchor
  complexity/useRegexLiterals
  correctness/noSelfAssign
  correctness/noInvalidBuiltinInstantiation
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  correctness/noInvalidDirectionInLinearGradient
  suspicious/noImportantInKeyframe
  a11y/useMediaCaption
  complexity/noUselessLabel
  complexity/noUselessCatch
  correctness/noUnsafeFinally
  a11y/useAriaPropsForRole
  correctness/noNonoctalDecimalEscape
  style/useEnumInitializers
  a11y/useHtmlLang
  suspicious/noDuplicateTestHooks
  complexity/noStaticOnlyClass
  style/useWhile
  complexity/useArrowFunction
  style/noInferrableTypes
  a11y/noNoninteractiveTabindex
  complexity/useSimpleNumberKeys
  correctness/useYield
  a11y/noInteractiveElementToNoninteractiveRole
  style/useNumericLiterals
  correctness/noUnnecessaryContinue
  suspicious/noApproximativeNumericConstant
  suspicious/noImportAssign
  suspicious/noLabelVar
  correctness/noGlobalObjectCalls
  suspicious/useDefaultSwitchClauseLast
  correctness/noUnknownUnit
  a11y/useAltText
  correctness/noEmptyCharacterClassInRegex
  suspicious/noSparseArray
  a11y/useIframeTitle
  complexity/noBannedTypes
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  style/useAsConstAssertion
  correctness/useJsxKeyInIterable
  style/useExportType
  suspicious/noDebugger
  complexity/noUselessLoneBlockStatements
  style/noArguments
  a11y/useValidAriaValues
  suspicious/noCommentText
  a11y/useFocusableInteractive
  correctness/noUnmatchableAnbSelector
  suspicious/noMisleadingInstantiator
  suspicious/noDuplicateJsxProps
  suspicious/noGlobalAssign
  a11y/noPositiveTabindex
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  security/noDangerouslySetInnerHtmlWithChildren
  a11y/useKeyWithMouseEvents
  suspicious/noExtraNonNullAssertion
  suspicious/noPrototypeBuiltins
  correctness/noRenderReturnValue
  correctness/useExhaustiveDependencies
  security/noGlobalEval
  style/noNonNullAssertion
  a11y/noRedundantRoles
  complexity/useFlatMap
  correctness/useIsNan
  style/useConst
  suspicious/noGlobalIsFinite
  suspicious/noSelfCompare
  suspicious/noShorthandPropertyOverrides
  suspicious/noAsyncPromiseExecutor
  suspicious/noDuplicateFontNames
  suspicious/noThenProperty
  suspicious/useGetterReturn
  security/noDangerouslySetInnerHtml
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  suspicious/noArrayIndexKey
  complexity/noWith
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  correctness/noChildrenProp
  correctness/noUnknownFunction
  correctness/noInvalidPositionAtImportRule
  suspicious/noConfusingLabels
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noConfusingVoidType
  suspicious/noFocusedTests
  a11y/useButtonType
  a11y/useSemanticElements
  a11y/noAriaUnsupportedElements
  correctness/noInvalidGridAreas
  correctness/noFlatMapIdentity
  a11y/noBlankTarget
  a11y/useHeadingContent
  correctness/useValidForDirection
  correctness/noVoidTypeReturn
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useGenericFontNames
  correctness/noUnknownMediaFeatureName
  a11y/useAnchorContent
  complexity/noUselessRename
  style/useNumberNamespace
  complexity/noUselessConstructor
  a11y/noAccessKey
  style/useExponentiationOperator
  style/noUnusedTemplateLiteral
  complexity/noUselessSwitchCase
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  suspicious/noDuplicateAtImportRules
  suspicious/useIsArray
  a11y/noHeaderScope
  complexity/noUselessFragments
  suspicious/noMisrefactoredShorthandAssign
  suspicious/noEmptyBlock
  complexity/noForEach
  correctness/noUnusedImports
  suspicious/noFunctionAssign

Workspace:
  Open Documents:               0

Rule name

lint/correctness/noUnusedImports

Playground link

https://biomejs.dev/playground/?indentStyle=space&lintRules=all&files.main.js=aQBtAHAAbwByAHQAIAB7AFMAbwBtAGUAQwBsAGEAcwBzAH0AIABmAHIAbwBtACAAIgAuAC8AbwB0AGgAZQByAGYAaQBsAGUALgBqAHMAIgA7AAoACgAvACoAKgAKACAAKgAgAEAAcABhAHIAYQBtACAAewBTAG8AbQBlAEMAbABhAHMAcwB9ACAAYQAKACAAKgAvAAoAZQB4AHAAbwByAHQAIABmAHUAbgBjAHQAaQBvAG4AIAB4ACgAYQApACAAewAKACAAIAByAGUAdAB1AHIAbgAgAGEALgBiACAAKwAgADUAOwAKAH0A

Expected result

As the import is referenced in the JSDoc, it should not be flagged as an unused import.

Removing this import causes tsc to complain (rightfully) that it doesn't know what type this is. (tsc checking JS files). This happens a lot when you are passing a certain type to a function, but never need to create it. You never explicitly reference the typename but access its properties.

Code of Conduct

  • [X] I agree to follow Biome's Code of Conduct

midnightveil avatar Dec 02 '24 06:12 midnightveil

Thanks, this seems like a valid request.

Please be aware though that we currently don’t do any analysis on JsDoc comments, so this is unlikely to be picked up soon unless someone wishes to contribute such support.

As a workaround, I would suggest moving the import itself into a comment as well. I believe tsc supports some syntax to do this.

arendjr avatar Dec 02 '24 07:12 arendjr

Just stumbled over this issue after I migrated to Biome these days. Another relevant JSDoc syntax accessing types is: {@link TypeName}, {@link TypeName.member}, {@link TypeName#member} (https://tsdoc.org/pages/tags/link/ and https://jsdoc.app/tags-inline-link)

It seems with https://github.com/biomejs/biome/pull/5599 Biome made the required step to do JSDoc type resolving. Is there anything more needed for this issue here, or does #5599 cover it already?

Danielku15 avatar Apr 14 '25 13:04 Danielku15

@Danielku15 What #5599 achieved is that we can now look up JSDoc comments of imported symbols. I don't think that's really what this issue is about, since it's about marking imported symbols as used when a local JSDoc comment makes a reference to them.

To implement the necessary support raised in this issue, noUnusedImports should be extended to scan the module for JSDoc comments, and interpret them, keeping in mind the syntaxes you mentioned, to see if there are any references to the imported symbols. Neither of these things are really helped by #5599 unfortunately.

arendjr avatar Apr 14 '25 14:04 arendjr

Thanks for the insights. I was searching the repository related to existing bits on JSDoc parsing and handling and the new PR looked promising 😁

I'm very new to Rust so contributing would be tough without some foundation in-place πŸ˜“ From a gut-feeling:

  1. The AST would need extension to hold the related comments (maybe moving the new JsdocComment and related bits to biome_js_syntax makes sense).
  2. Parse the JsDoc syntax into dedicated AST nodes according to spec. To minimize the performance impact on existing projects it could be a new option to enable JSDoc/TSDoc parsing. This should likely happen in biome_js_parser
  3. On the semantic model (somehow) the type references need to be bound and registered.
  4. The no_unused_imports rule would pick up the newly registered references from the semantic model.

Are my assumptions correct? Do you have any hints which bits of the code I could explore deeper to learn about the required bits?

Danielku15 avatar Apr 14 '25 15:04 Danielku15

@Danielku15 I don't think it's as much work as that, to be honest.

Our CST (it's not an AST) already has the comments, so no extensions are necessary there. JsdocComment::try_from(node: &JsSyntaxNode) can be used for extracting JSDoc comments from any existing syntax nodes, it just means that noUnusedImports needs to traverse almost every node, because JSDoc comments may exist almost anywhere in the document.

I also don't think I would write a full blown parser for JSDoc comments either, since the format is pretty relaxed to begin with. Just scan the comment lines for the tag you're looking for, and if found, interpret the following part per spec.

As for the semantic model, I think I wouldn't involve it here. Just let the rule create a hash map with symbols discovered in JSDoc type references, and before letting noUnusedImports signal a diagnostic, let it check the map to see if the symbol is in there. We can always try to generalise things if more rules want access to this kind of information, but I wouldn't try to complicate it from the start.

And this way, we probably can keep the performance sufficiently under control that we don't need to create additional options for it. Or if we do need it, it can simply be kept on the rule itself.

arendjr avatar Apr 14 '25 17:04 arendjr

Time to learn some Rust 😁 Lets see how far I get

Danielku15 avatar Apr 14 '25 19:04 Danielku15

@arendjr

As for the semantic model, I think I wouldn't involve it here. Just let the rule create a hash map with symbols discovered in JSDoc type references, and before letting noUnusedImports signal a diagnostic, let it check the map to see if the symbol is in there. We can always try to generalise things if more rules want access to this kind of information, but I wouldn't try to complicate it from the start.

I got a bit stuck on where I could collect and store the types as part of the NoUnusedImports rule itself. With ctx.root() I could get the AST (a copy at least) and traverse it. But as run is called for every matched node I need some mechanism to collect and store the list of types once per file before the actual import nodes are analyzed and diagnostics are created.

Is there a good way to traverse the CST once per file and remember some stuff for usage in run and diagnostics?

I also don't think I would write a full blown parser for JSDoc comments either, since the format is pretty relaxed to begin with. Just scan the comment lines for the tag you're looking for, and if found, interpret the following part per spec.

I created a bunch of test files to begin with. The syntax can get quite complex for JSDoc https://jsdoc.app/tags-type but maybe some existing parser for TypeScript type nodes can help there in future.

issue_4677_jsdoc_syntax.js
// See https://github.com/biomejs/biome/issues/4677

// https://jsdoc.app/tags-type

import SymbolName from "mod";
import MultipleTypes from "mod";
import ArraysGeneric from "mod";
import ArraysBrackets from "mod";
import AnyGenerics from "mod";
import ObjectProperties from "mod";
import Nullable from "mod";
import NonNullable from "mod";
import VarArgs from "mod";
import OptionalParameter from "mod";

/**
 * @type {SymbolName}
 */
let testSymbolName;

/**
 * @type {(number|MultipleTypes)}
 */
let testMultipleTypes;

/**
 * @type {Array.<ArraysGeneric>}
 */
let testArraysGeneric;

/**
 * @type {ArraysBrackets[]}
 */
let testArraysBrackets;

/**
 * @type {Object.<string, AnyGenerics>}
 */
let testAnyGenerics;

/**
 * @type {{a:number, b:ObjectProperties}}
 */
let testObjectProperties;

/**
 * @type {?Nullable}
 */
let testNullable;

/**
 * @type {!NonNullable}
 */
let testNonNullable;

/**
 * @param {...VarArgs} params
 */
function testVarArgs(params) {}

/**
 * @param {OptionalParameter=} optional
 */
function testOptionalParameter(optional) {}
issue_4677_jsdoc.js
// See https://github.com/biomejs/biome/issues/4677

// some types for the different locations
import TypeOnFunctionParam from "mod";
import TypeOnClassMethodParam from "mod";
import TypeOnClassConstructorParam from "mod";
import TypeOnClassField from "mod";
import TypeOnGlobalVariable from "mod";
import TypeOnFunctionVariable from "mod";
import TypeOnTypeDef from "mod";

/**
 * @typedef {TypeOnTypeDef} TestTypeOnTypeDef 
 */

/**
 * @param param {TypeOnFunctionParam}
 */
function testTypeOnFunction(param) {}

class TestTypeOnClassMethodParam {
	/**
	 * @param {TypeOnClassMethodParam} param
	 */
	method(param) {}
}

class TestTypeOnClassConstructorParam {
	/**
	 * @param {TypeOnClassConstructorParam} param
	 */
	constructor(param) {}
}

class TestTypeOnClassField {
	constructor(param) {
        /**
         * @type {TypeOnClassField}
         */
        this.var = 10;
    }
}

/**
 * @type {TypeOnGlobalVariable}
 */
let testTypeOnGlobalVariable;

function testTypeOnFunctionVariable() {
	/**
	 * @type {TypeOnFunctionVariable}
	 */
	let testTypeOnFunctionVariable;
}

issue_4677_tsdoc_syntax.ts
// See https://github.com/biomejs/biome/issues/4677

import type SyntaxNormal from "mod";
import type SyntaxWithDotMember from "mod";
import type SyntaxWithHashMember from "mod";
import type SyntaxWithDotMemberAndPipeDescription from "mod";
import type SyntaxWithHashMemberAndPipeDescription from "mod";
import type SyntaxWithDotMemberAnSpaceDescription from "mod";
import type SyntaxWithHashMemberAndSpaceDescription from "mod";

/**
 * {@link SyntaxNormal}
 */
function testSyntaxNormal() { }

/**
 * {@link SyntaxWithDotMember.member}
 */
function testSyntaxWithDotMember() { }

/**
 * {@link SyntaxWithHashMember#member}
 */
function testSyntaxWithHashMember() { }

/**
 * {@link SyntaxWithDotMemberAndPipeDescription.member|Description}
 */
function testSyntaxWithDotMemberAndPipeDescription() { }

/**
 * {@link SyntaxWithHashMemberAndPipeDescription#member|Description}
 */
function testSyntaxWithHashMemberAndPipeDescription() { }

/**
 * {@link SyntaxWithDotMemberAnSpaceDescription.member Description}
 */
function testSyntaxWithDotMemberAnSpaceDescription() { }

/**
 * {@link SyntaxWithHashMemberAndSpaceDescription#member Description}
 */
function testSyntaxWithHashMemberAndSpaceDescription() { }
issue_4677_tsdoc.ts
// See https://github.com/biomejs/biome/issues/4677

// some types for the different locations
import type LinkOnFunction from "mod";

import type LinkOnVariable from "mod";
import type LinkOnClass from "mod";

import type LinkOnClassField from "mod";
import type LinkOnClassMethod from "mod";
import type LinkOnClassConstructor from "mod";
import type LinkOnClassAccessor from "mod";
import type LinkOnClassIndexer from "mod";

import type LinkOnInterface from "mod";
import type LinkOnInterfaceField from "mod";
import type LinkOnInterfaceMethod from "mod";
import type LinkOnInterfaceIndexer from "mod";

/**
 * {@link LinkOnFunction}
 */
function testLinkOnFunction() { }

/**
 * {@link LinkOnVariable}
 */
const testLinkOnVariable = 3;

/**
 * {@link LinkOnClass}
 */
class TestLinkOnClass { }

class TestLinkOnClassField {
    /**
     * {@link LinkOnClassField}
     */
    field: number;
}

class TestLinkOnClassMethod {
    /**
     * {@link LinkOnClassMethod}
     */
    method(): void { }
}

class TestLinkOnClassConstructor {
    /**
     * {@link LinkOnClassConstructor}
     */
    constructor() { }
}

class TestLinkOnClassAccessor {
    /**
     * {@link LinkOnClassAccessor}
     */
    get accessor(): number { return 0 }
}

class TestLinkOnClassIndexer {
    /**
     * {@link LinkOnClassIndexer}
     */
    [index: number]: string;
}

/**
 * {@link LinkOnInterface}
 */
interface TestLinkOnInterface { }

interface TestLinkOnInterfaceField {
    /**
     * {@link LinkOnInterfaceField}
     */
    field: string;
}

interface TestLinkOnInterfaceMethod {
    /**
     * {@link LinkOnInterfaceMethod}
     */
    method(): string;
}

interface TestLinkOnInterfaceIndexer {
    /**
     * {@link LinkOnInterfaceIndexer}
     */
    [index: number]: string;
}

Danielku15 avatar Apr 17 '25 17:04 Danielku15

Is there a good way to traverse the CST once per file and remember some stuff for usage in run and diagnostics?

Oh yes, sounds like that might indeed be what you need here. It's essentially what the semantic model itself does as well, but instead of using that model you'll just create your own small model and use that instead. For another example of how to do that, see the EarlyReturnDetectionVisitor in use_hook_at_top_level.rs.

And yes, you're right the type annotations can get somewhat complex indeed, maybe those require a tiny parser indeed. I was more referring to extracting the tags themselves. That part you can do pretty ad-hoc, I think, and then extract the type value, and restrict your type parser to only interpreting the right values.

arendjr avatar Apr 17 '25 17:04 arendjr

A first proposal is now available via https://github.com/biomejs/biome/pull/5698. Bear with me on the style and proper syntax bits of Rust. My very first piece of work in Rust πŸ₯³

TBH it was mostly a matter of adapting what I've learned from looking how things are done left and right. Still learning πŸ˜…

Danielku15 avatar Apr 17 '25 21:04 Danielku15

Since https://github.com/biomejs/biome/pull/5698 was merged, I think the issue should be closed now?

tidefield avatar Jun 26 '25 13:06 tidefield

I think no, since the issue still reproduces on the playground in the description

siketyan avatar Jun 26 '25 13:06 siketyan

Ok this is interesting because the PR included snapshot tests that would cover the issue reproduced in playground. I can take a look at this issue to see if there's remaining work to do.

tidefield avatar Jun 26 '25 14:06 tidefield

The unhandled remaining case is exported clauses.

In the visitor implementation, I tried printing the variables out and got

event: Enter([email protected])
node: [email protected]
token: [email protected] "function" [] [Whitespace(" ")]

for an exported function.

The same function, if not exported, will result to

event: Enter([email protected])
node: [email protected]
token: [email protected] "function" [Newline("\n"), Newline("\n"), Comments("/**\n * @param {TypeOn ..."), Newline("\n")] [Whitespace(" ")]

I think the issue might be outside of the visitor implementation. I'm gonna have to dive deeper.

tidefield avatar Jun 26 '25 18:06 tidefield

I think I managed to make a fix in https://github.com/biomejs/biome/pull/6565.

tidefield avatar Jun 26 '25 20:06 tidefield

Fixed by #5698 and #6565.

arendjr avatar Jun 27 '25 07:06 arendjr