Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💅 noUnusedImports removes any comments above the import #3401

Closed
1 task done
robintown opened this issue Jul 9, 2024 · 5 comments · Fixed by #4479
Closed
1 task done

💅 noUnusedImports removes any comments above the import #3401

robintown opened this issue Jul 9, 2024 · 5 comments · Fixed by #4479
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature S-Needs discussion Status: needs a discussion to understand criteria

Comments

@robintown
Copy link
Contributor

Environment information

CLI:
  Version:                      1.8.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.12.2"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "yarn/1.22.22"

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

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  false
  Recommended:                  true
  All:                          false
  Enabled rules:
  performance/noDelete
  suspicious/noCatchAssign
  suspicious/noUnsafeNegation
  complexity/useLiteralKeys
  style/useImportType
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessEmptyExport
  suspicious/useNamespaceKeyword
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  correctness/noConstantCondition
  a11y/useAriaActivedescendantWithTabindex
  suspicious/noAssignInExpressions
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noDuplicateParameters
  style/useTemplate
  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
  suspicious/noRedeclare
  correctness/noStringCaseMismatch
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  suspicious/noFallthroughSwitchClause
  suspicious/noUnsafeDeclarationMerging
  correctness/noUnreachable
  a11y/useKeyWithClickEvents
  suspicious/noDuplicateObjectKeys
  complexity/noUselessThisAlias
  complexity/noThisInStatic
  complexity/useOptionalChain
  correctness/noInnerDeclarations
  style/noParameterAssign
  suspicious/noDuplicateCase
  a11y/useValidAnchor
  complexity/useRegexLiterals
  correctness/noSelfAssign
  style/noUselessElse
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  nursery/noUnusedFunctionParameters
  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
  a11y/useAltText
  correctness/noEmptyCharacterClassInRegex
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noSparseArray
  a11y/useIframeTitle
  complexity/noBannedTypes
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  style/useAsConstAssertion
  correctness/useJsxKeyInIterable
  style/useExportType
  complexity/noUselessLoneBlockStatements
  suspicious/noPrototypeBuiltins
  suspicious/noMisleadingInstantiator
  suspicious/noDebugger
  style/noArguments
  a11y/useValidAriaValues
  suspicious/noCommentText
  suspicious/noThenProperty
  suspicious/noDuplicateJsxProps
  suspicious/noGlobalAssign
  a11y/noPositiveTabindex
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  security/noDangerouslySetInnerHtmlWithChildren
  a11y/useKeyWithMouseEvents
  suspicious/noExtraNonNullAssertion
  suspicious/useGetterReturn
  correctness/noRenderReturnValue
  correctness/useExhaustiveDependencies
  security/noGlobalEval
  style/useConst
  a11y/noRedundantRoles
  complexity/useFlatMap
  correctness/useIsNan
  correctness/noUnusedVariables
  suspicious/noGlobalIsFinite
  suspicious/noSelfCompare
  suspicious/noAsyncPromiseExecutor
  security/noDangerouslySetInnerHtml
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  suspicious/noArrayIndexKey
  complexity/noWith
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  correctness/noChildrenProp
  suspicious/noConfusingLabels
  suspicious/noConfusingVoidType
  suspicious/noFocusedTests
  a11y/useButtonType
  a11y/noAriaUnsupportedElements
  correctness/noFlatMapIdentity
  a11y/noBlankTarget
  a11y/useHeadingContent
  correctness/useValidForDirection
  correctness/noUnusedPrivateClassMembers
  correctness/noVoidTypeReturn
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useAnchorContent
  complexity/noUselessRename
  correctness/noInvalidNewBuiltin
  style/useNumberNamespace
  complexity/noUselessConstructor
  a11y/noAccessKey
  style/useExponentiationOperator
  style/noUnusedTemplateLiteral
  complexity/noUselessSwitchCase
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  suspicious/useIsArray
  a11y/noHeaderScope
  complexity/noUselessFragments
  suspicious/noMisrefactoredShorthandAssign
  complexity/noForEach
  correctness/noUnusedImports
  suspicious/noClassAssign
  suspicious/noFunctionAssign

Workspace:
  Open Documents:               0

Done in 0.06s.

Rule name

lint/correctness/noUnusedImport

Playground link

https://biomejs.dev/playground/?lintRules=all&importSortingEnabled=false&code=LwAqACAAQwBvAHAAeQByAGkAZwBoAHQAIAAyADAAMgA0ACAAKgAvAAoACgBpAG0AcABvAHIAdAAgAHsAIAB0AGgAaQBuAGcAIAB9ACAAZgByAG8AbQAgACcAbQB5AHAAYQBjAGsAYQBnAGUAJwA7AA%3D%3D

Expected result

Click the console tab to view the suggested fix. It removes the copyright comment in addition to the import. I would expect it to leave any comments intact.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Sec-ant
Copy link
Member

Sec-ant commented Jul 9, 2024

This was an intended behavior introduced in 4f1e9cd

Part of the reason of this behavior was to fix the unstable code actions that PR #2237 was trying to fix. However that fix turned out to be not complete and we may address it with another approach (#3383). So we can revert that change if the new approach turns out to work.

Another reason is that some directive comments like // @ts-expect-error and JSDocs/TSDocs should be removed along with the code, otherwsie they could be left in a wrong place. I don't know if we have better ways to deal with such cases if we decide to not remove the comments. Maybe we can mark some types of comments as removable?

But I agree that removing copyright/banner comments seems a little disruptive, especially for code actions that happen to the import statements which usually appear in the top of the file.

@Sec-ant Sec-ant added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Needs discussion Status: needs a discussion to understand criteria S-Enhancement Status: Improve an existing feature labels Jul 9, 2024
@robintown
Copy link
Contributor Author

Interesting, perhaps a quick fix to this issue would be to mark the fix as "unsafe" if it would remove a comment, to encourage manual review.

@Sec-ant
Copy link
Member

Sec-ant commented Jul 9, 2024

Interesting, perhaps a quick fix to this issue would be to mark the fix as "unsafe" if it would remove a comment, to encourage manual review.

Yeah but that would be a breaking change so it won't be very quick. As a workaround, you can override the safety of the code actions: https://biomejs.dev/linter/#configure-the-rule-fix

@Conaclos
Copy link
Member

We could only remove leading comments that are not separated by a blank line from the import.
However, this will require to examine the trivia. As an example:

// Comment to keep

- // Commnet to remove
- import { Unused } from ""

@theogravity
Copy link

theogravity commented Sep 16, 2024

Encountered this. We have comments like // Do not remove this comment: resource-imports that we use with our template generators to add imports underneath during scaffolding that sometimes gets removed.

https://github.com/theogravity/fastify-starter-turbo-monorepo/blob/main/apps/backend/src/api/routes.ts

https://github.com/theogravity/fastify-starter-turbo-monorepo/blob/main/apps/backend/turbo/generators/actions/api-route.generator.ts#L57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature S-Needs discussion Status: needs a discussion to understand criteria
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants