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

💅 noUselessElse does not respect else if conditional #4354

Closed
1 task done
aabmets opened this issue Oct 21, 2024 · 5 comments
Closed
1 task done

💅 noUselessElse does not respect else if conditional #4354

aabmets opened this issue Oct 21, 2024 · 5 comments
Labels
S-Needs triage Status: this issue needs to be triaged

Comments

@aabmets
Copy link

aabmets commented Oct 21, 2024

Environment information

CLI:
  Version:                      1.9.4
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_PATH:               unset
  BIOME_LOG_PREFIX_NAME:        unset
  BIOME_CONFIG_PATH:            unset
  NO_COLOR:                     unset
  TERM:                         unset
  JS_RUNTIME_VERSION:           "v22.9.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         unset

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:                  true
  GraphQL enabled:              false
  Recommended:                  true
  All:                          false
  Enabled rules:
  suspicious/noCatchAssign
  complexity/useLiteralKeys
  complexity/noMultipleSpacesInRegularExpressionLiterals
  suspicious/useNamespaceKeyword
  complexity/noUselessEmptyExport
  suspicious/noAssignInExpressions
  suspicious/noDuplicateSelectorsKeyframeBlock
  suspicious/noDuplicateParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noMisplacedAssertion
  style/useTemplate
  correctness/noUnusedLabels
  correctness/noUnreachableSuper
  suspicious/noCompareNegZero
  a11y/noAutofocus
  correctness/noUnsafeOptionalChaining
  correctness/noConstAssign
  suspicious/noControlCharactersInRegex
  style/noVar
  suspicious/noDoubleEquals
  suspicious/noEmptyInterface
  suspicious/noConstEnum
  correctness/noPrecisionLoss
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  a11y/useKeyWithClickEvents
  suspicious/noDuplicateObjectKeys
  suspicious/noUnsafeDeclarationMerging
  correctness/noInnerDeclarations
  correctness/useArrayLiterals
  style/noUselessElse
  correctness/noInvalidDirectionInLinearGradient
  suspicious/noImportantInKeyframe
  complexity/noUselessLabel
  complexity/noUselessCatch
  a11y/useAriaPropsForRole
  style/useCollapsedElseIf
  correctness/noNonoctalDecimalEscape
  suspicious/noDuplicateTestHooks
  style/noDoneCallback
  complexity/noStaticOnlyClass
  complexity/noUselessUndefinedInitialization
  a11y/noInteractiveElementToNoninteractiveRole
  suspicious/noLabelVar
  correctness/noUnnecessaryContinue
  suspicious/noApproximativeNumericConstant
  correctness/noEmptyCharacterClassInRegex
  correctness/noUnknownUnit
  suspicious/noSparseArray
  a11y/useIframeTitle
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  correctness/useJsxKeyInIterable
  style/useExportType
  complexity/noUselessLoneBlockStatements
  style/noArguments
  a11y/useValidAriaValues
  suspicious/noGlobalAssign
  suspicious/noCommentText
  suspicious/noThenProperty
  suspicious/useGetterReturn
  a11y/noPositiveTabindex
  style/useNamingConvention
  correctness/noRenderReturnValue
  correctness/useExhaustiveDependencies
  security/noGlobalEval
  style/noYodaExpression
  a11y/noRedundantRoles
  correctness/noUnusedVariables
  suspicious/noSelfCompare
  suspicious/noAsyncPromiseExecutor
  security/noDangerouslySetInnerHtml
  style/useNodejsImportProtocol
  suspicious/noArrayIndexKey
  complexity/noWith
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  suspicious/noConfusingLabels
  correctness/noChildrenProp
  correctness/noUnknownFunction
  correctness/noInvalidPositionAtImportRule
  style/noShoutyConstants
  a11y/noAriaUnsupportedElements
  correctness/noFlatMapIdentity
  a11y/noBlankTarget
  a11y/useHeadingContent
  correctness/useValidForDirection
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useGenericFontNames
  style/useBlockStatements
  style/noNegationElse
  complexity/useSimplifiedLogicExpression
  correctness/noUnusedFunctionParameters
  style/noUnusedTemplateLiteral
  style/useExponentiationOperator
  style/noNamespaceImport
  correctness/noUndeclaredVariables
  suspicious/useAwait
  style/noNamespace
  suspicious/noDuplicateAtImportRules
  complexity/noUselessFragments
  complexity/noUselessStringConcat
  correctness/noUnusedImports
  suspicious/noEmptyBlock
  suspicious/noFunctionAssign
  performance/noDelete
  suspicious/noUnsafeNegation
  a11y/useValidLang
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  correctness/noConstantCondition
  a11y/useAriaActivedescendantWithTabindex
  style/useExplicitLengthCheck
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noUnknownProperty
  complexity/noUselessTernary
  suspicious/noExplicitAny
  correctness/noSwitchDeclarations
  complexity/noUselessTypeConstraint
  suspicious/noRedundantUseStrict
  style/useLiteralEnumMembers
  suspicious/noGlobalIsNan
  suspicious/noSkippedTests
  suspicious/noMisleadingCharacterClass
  a11y/noLabelWithoutControl
  style/useDefaultSwitchClause
  correctness/noStringCaseMismatch
  suspicious/noRedeclare
  suspicious/noFallthroughSwitchClause
  complexity/noUselessThisAlias
  correctness/noUnreachable
  complexity/noThisInStatic
  complexity/useOptionalChain
  suspicious/noDuplicateCase
  style/noParameterAssign
  a11y/useValidAnchor
  complexity/useRegexLiterals
  correctness/noSelfAssign
  correctness/noInvalidBuiltinInstantiation
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  style/useThrowNewError
  suspicious/noEmptyBlockStatements
  a11y/useMediaCaption
  correctness/noUnsafeFinally
  style/useNodeAssertStrict
  style/useEnumInitializers
  a11y/useHtmlLang
  correctness/noUndeclaredDependencies
  style/useWhile
  suspicious/noConsole
  complexity/useArrowFunction
  style/noInferrableTypes
  a11y/noNoninteractiveTabindex
  complexity/useSimpleNumberKeys
  correctness/useYield
  style/useNumericLiterals
  suspicious/noImportAssign
  suspicious/useDefaultSwitchClauseLast
  correctness/noGlobalObjectCalls
  style/noParameterProperties
  a11y/useAltText
  performance/noBarrelFile
  suspicious/noSuspiciousSemicolonInJsx
  complexity/noBannedTypes
  style/useThrowOnlyError
  suspicious/noPrototypeBuiltins
  style/useAsConstAssertion
  suspicious/noDebugger
  suspicious/noMisleadingInstantiator
  complexity/noVoid
  a11y/useFocusableInteractive
  correctness/noUnmatchableAnbSelector
  suspicious/noDuplicateJsxProps
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  performance/noReExportAll
  a11y/useKeyWithMouseEvents
  security/noDangerouslySetInnerHtmlWithChildren
  suspicious/noExtraNonNullAssertion
  suspicious/noShorthandPropertyOverrides
  style/useConst
  style/noNonNullAssertion
  complexity/useFlatMap
  correctness/useHookAtTopLevel
  correctness/useIsNan
  suspicious/noGlobalIsFinite
  style/useConsistentBuiltinInstantiation
  suspicious/noDuplicateFontNames
  complexity/noExcessiveCognitiveComplexity
  a11y/noDistractingElements
  style/useConsistentArrayType
  style/useForOf
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  complexity/useDateNow
  suspicious/noConfusingVoidType
  suspicious/noFocusedTests
  a11y/useButtonType
  a11y/useSemanticElements
  correctness/noInvalidGridAreas
  style/useFilenamingConvention
  style/useShorthandAssign
  suspicious/useErrorMessage
  correctness/noConstantMathMinMaxClamp
  correctness/noUnusedPrivateClassMembers
  correctness/noVoidTypeReturn
  correctness/noUnknownMediaFeatureName
  a11y/useAnchorContent
  complexity/noUselessRename
  style/useNumberNamespace
  complexity/noUselessConstructor
  a11y/noAccessKey
  complexity/noUselessSwitchCase
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  suspicious/useNumberToFixedDigitsArgument
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  suspicious/useIsArray
  a11y/noHeaderScope
  suspicious/noMisrefactoredShorthandAssign
  complexity/noForEach
  suspicious/noClassAssign
  style/noImplicitBoolean
  style/useImportType

Workspace:
  Open Documents:               0

Rule name

noUselessElse

Playground link

https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB1AHQAaQBsAHMAIABmAHIAbwBtACAAIgAuAC8AdQB0AGkAbABzACIAOwAKAAoAZgB1AG4AYwB0AGkAbwBuACAAdwByAGkAdABlAEYAaQBsAGUAcwBXAGgAZQBuAFAAZQByAG0AaQB0AHQAZQBkACgAcgBlAHQAdQByAG4ARQBhAHIAbAB5ADoAIABiAG8AbwBsAGUAYQBuACkAOgAgAHMAdAByAGkAbgBnACAAewAKACAAIABpAGYAIAAoAHIAZQB0AHUAcgBuAEUAYQByAGwAeQApACAAewAKACAAIAAgACAAcgBlAHQAdQByAG4AIAAiAEYAaQBsAGUAIAB3AHIAaQB0AGUAIABzAGsAaQBwAHAAZQBkACIAOwAKACAAIAB9ACAAZQBsAHMAZQAgAGkAZgAgACgAdQB0AGkAbABzAC4AdQBzAGUAcgBIAGEAcwBQAGUAcgBtAGkAcwBzAGkAbwBuAHMAKAApACkAIAB7AAoAIAAgACAAIAB1AHQAaQBsAHMALgB3AHIAaQB0AGUARgBpAGwAZQBzACgAKQA7AAoAIAAgACAAIAByAGUAdAB1AHIAbgAgACIARgBpAGwAZQBzACAAdwByAGkAdAB0AGUAbgAgAHMAdQBjAGMAZQBzAHMAZgB1AGwAbAB5ACIAOwAKACAAIAB9AAoAIAAgAHIAZQB0AHUAcgBuACAAIgBOAG8AdAAgAHAAZQByAG0AaQB0AHQAZQBkACAAdABvACAAdwByAGkAdABlACAAZgBpAGwAZQBzACIAOwAKAH0ACgA%3D

Expected result

Biome should not complain about useless else, if it's an else if, because returning early in an if conditional chain is a valid programming approach to minimize the amount of conditional checks and it allows to de-nest if statements, reducing indentation levels and making the code easier to read.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@aabmets aabmets added the S-Needs triage Status: this issue needs to be triaged label Oct 21, 2024
@ematipico
Copy link
Member

ematipico commented Oct 21, 2024

Your point is valid, but so is the rule, that is why it belongs to the style group. It enforces a certain coding style. The rule is doing exactly what it is meant to, so it's not a bug.

I would argue that it shouldn't be recommended, but that's a breaking change. We might consider it for v2.

Feel free to open a discussion

@ematipico ematipico closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2024
@Conaclos
Copy link
Member

The rule is intended to report this kind of code because your code is equivalent to:

import utils from "./utils";

function writeFilesWhenPermitted(returnEarly: boolean): string {
  if (returnEarly) {
    return "File write skipped";
  }
  if (utils.userHasPermissions()) {
    utils.writeFiles();
    return "Files written successfully";
  }
  return "Not permitted to write files";
}

returning early in an if conditional chain is a valid programming approach to minimize the amount of conditional checks and it allows to de-nest if statements

I really don't understand your point.
An if-chain avoids extra conditional checks regardless of whether there is an early return or not.
The rule ensures that there is a single way of writing a code with early return and promote de-nesting.

@aabmets
Copy link
Author

aabmets commented Oct 21, 2024

@ematipico @Conaclos
What should I do, if I want Biome to throw noUselessElse when else is used, but I specifically want to permit else if in my project? (Yes I know I could just start another if block, but that is not my question).

@ematipico
Copy link
Member

You can suppress the rule for that code branch:

if (foo) {
	return false;
} // biome-ignore lint/style/noUselessElse: better reading 
else if () {
	return false;
}

@Conaclos
Copy link
Member

Conaclos commented Oct 21, 2024

For now there is no way of getting the behavior you want (except by using ignore comments). We could consider adding noUselessElseIf and ignoring else if in noUselessElse (This could allow us to reference ESLint and Clippy rules as sources instead of inspirations). We could make the change in the next release (Biome 2.0) that allows making some breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Needs triage Status: this issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants