Disallow truthiness/nullishness checks on syntax that never varies on it #59217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: ember__component/v3 Package: hapi__joi Package: karma-browserstack-launcher Package: ember__component Package: rsvp Package: ember/v2 Package: react-native-joi Package: ember/v3 Package: ember |
|
Wow, not entirely related but the new "build mode continues on error" makes the self test super noisy, it's like some files aren't getting emitted from our repo or something and that really breaks analysis. I assume because we use noEmitOnError (probably we shouldn't?) |
|
I think the I don't see a bug in this case; is this just a bug in the rule? |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
Yeah, I forgot ternaries, plus some other bugs. A lot of good finds in there though. @typescript-bot test top800 |
jakebailey
reviewed
Jul 10, 2024
| @@ -920,6 +920,14 @@ export const enum RelationComparisonResult { | |||
| ReportsMask = ReportsUnmeasurable | ReportsUnreliable, | |||
| } | |||
|
|
|||
| /** @internal */ | |||
| export const enum PredicateSemantics { | |||
There was a problem hiding this comment.
I think Ternary would actually cover this, if you didn't want to come up with something new.
jakebailey
reviewed
Jul 10, 2024
| @@ -8705,6 +8705,7 @@ declare namespace ts { | |||
| * ``` | |||
| */ | |||
| function getJSDocCommentsAndTags(hostNode: Node): readonly (JSDoc | JSDocTag)[]; | |||
| function isBinaryLogicalOperator(token: SyntaxKind): boolean; | |||
There was a problem hiding this comment.
Do we want /** @internal */ on this? (I know it's a draft but before I forget)
|
@RyanCavanaugh Here are the results of running the top 800 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
Not done looking at all of these yet, but so far a 100% correctness rate... AbdullahAlfaraj/Auto-Photoshop-StableDiffusion-Plugin Definitely a bug: <div key={`${node_id}_${name}__${index}` ?? void 0}>ariakit/ariakit Definitely a bug: horizontal = itemObject.orientation === "horizontal" ?? horizontal;bitwarden/clients Definitely a bug: map((options) => options?.trustedDeviceOption != null ?? false),ChatGPTNextWeb/ChatGPT-Next-Web Definitely a bug: if (historyMsgLength > modelConfig?.max_tokens ?? 4000) {clash-verge-rev/clash-verge-rev Definitely a bug: setTheme({ ...theme_setting } || {});dream-num/univer Definitely bugs (4!): // Two instances of this:
undos: [] || undoMutations,
let fontFamily = localeService.t(`fontFamily.${(`${value}` ?? '').replace(/\s/g, '')}`);
<span className={styles.uiPluginSheetsFontFamilyItem} style={{ fontFamily: value }}>
{localeService.t(`fontFamily.${(`${value}` ?? '').replace(/\s/g, '')}`)}
</span>ethers-io/ethers.js Definitely a subtle parenthesization bug: if (isError(error, "CANCELLED") || isError(error, "BAD_DATA") ||
isError(error, "NETWORK_ERROR" || isError(error, "UNSUPPORTED_OPERATION"))) {facebook/lexical Definitely a bug: {...prevStyles} || {},gpbl/react-day-picker Definitely a bug: if (monthsDiff < numberOfMonths ?? 1) {graphile/crystal Definitely a bug: const nonNullableAttribute = this.codec.attributes
? Object.entries(this.codec.attributes).find(
([_attributeName, spec]) =>
!spec.via && !spec.expression && spec.notNull,
)?.[0]
: null ?? pk?.attributes[0];labring/FastGPT Definitely three bugs: <QuestionTip ml={1} label={publishT('QPM Tips' || '')}></QuestionTip>
// Second instance of above
<QuestionTip ml={1} label={publishT('QPM Tips' || '')}></QuestionTip>
<QuestionTip
ml={1}
label={t('support.outlink.share.Response Quote tips' || '')}
></QuestionTip>Licoy/ChatGPT-Midjourney Definitely a bug: if (historyMsgLength > modelConfig?.max_tokens ?? 4000) {material-components/material-web Definitely a bug: methodsTable.addRow([
`\`${rowObj.name}\``,
rowObj.parameters.map((p) => `\`${p.name}\``).join(', ') || '_None_',
`\`${rowObj.returns}\`` ?? '`void`',
rowObj.description ?? '',
]);material-shell/material-shell Definitely a bug: const msWorkspaceIsInFloatLayout =
metaWindow.msWindow?.msWorkspace.layout.state.key === 'float' ?? false;microsoft/azuredatastudio Definitely two bugs: this._logger.warn('Failed to delete token from server.' + e.message ?? e);
// and
const verbose = !!options.verbose ?? (!!process.env['CI'] || !!process.env['BUILD_ARTIFACTSTAGINGDIRECTORY']);microsoft/vscode Many bugs! uselessReplaceKey5: '@ham' || '',
// 3 instances of this
assert.ok(!'Unexpected value ' + basename(value.resource.fsPath));
const contentHeight = (editor._getViewModel()?.getLineCount()! * lineHeight) ?? editor.getContentHeight();
singlePerResource: () => !this.getCustomEditorCapabilities(contributedEditor.id)?.supportsMultipleEditorsPerDocument ?? true
tooltip: (`${exitMessage} ` ?? '') + nls.localize('launchFailed.exitCodeOnlyShellIntegration', 'Disabling shell integration in user settings might help.')
return !!this._chatWidget?.rawValue?.hasFocus() ?? false
return this.viewModel?.welcomeExperience === WelcomeExperience.ForWorkspace ?? true;
return !item?.handle.startsWith('vscode-command:') ?? false;
this._logger.warn('Failed to delete token from server.' + e.message ?? e);
const verbose = !!options.verbose ?? (!!process.env['CI'] || !!process.env['BUILD_ARTIFACTSTAGINGDIRECTORY']);motion-canvas/motion-canvas Definitely a bug: throw 'Unexpected Status: ' + result.statusCode ?? 'NO_STATUS';naver/billboard.js Definitely a bug, but super unclear what they meant here: if (isArcishData && "id") {twofer: function getScrollPosition(node: HTMLElement) {
return {
x: (window.pageXOffset ?? window.scrollX ?? 0) + node.scrollLeft ?? 0,
y: (window.pageYOffset ?? window.scrollY ?? 0) + node.scrollTop ?? 0
};
}openblocks-dev/openblocks width: ${(props) =>
props.placement === "right"
? "calc(100% - 96px)"
: "bottom"
? "calc(100% - 112px)"
: "calc(100% - 136px"};
flex-grow: 1;and const LabelWrapper = styled.div<{ placement: ControlPlacement }>`
flex-shrink: 0;
width: ${(props) => (props.placement === "right" ? "96px" : "bottom" ? "112px" : "136px")};
`; |
|
(wiping sweat from brow) They're literally all bugs portainer/portainer Definitely a bug: Name: `${node.metadata?.name}${isApi ? 'api' : ''}` ?? ''react-page/react-page Definitely a bug: return useNodeProps(nodeId, (node) =>
isRow(node)
? node.cells?.length > 0 ?? false
: (node?.rows?.length ?? 0) > 0 ?? false
);reactjs/react.dev Definitely a bug (what exactly were they trying to do here?): throw new Error(
'Expected name, title, permalink, and children for ' + name ??
title ??
permalink ??
'unknown'
);rollingscopes/rsschool-app Definitely a bug: await this.courseRepository.query(
`select "name", "id", 'event' as "type" from "event" where "id" = ANY($1)
union
select "name", "id", 'task' as "type" from "task" where "id" = ANY($2)`,
[fetchEvents.size > 0 ? [...fetchEvents] : null, [...fetchTasks] || null],
);Shopify/react-native-skia Definitely a bug: ps.textAlign =
value.textAlign !== undefined
? { value: value.textAlign }
: undefined ?? ps.textAlign;supabase/supabase Definitely 6 bugs here: const isNotOrgWithPartnerBilling = !subscription?.billing_via_partner ?? true;
// and
const isAcknowledged = typeof window !== 'undefined' ? localStorage?.getItem(key) === 'true' ?? false : false
// and
function checkFeature(feature: Feature, features?: Feature[]) {
return !features?.includes(feature) ?? true
}
// and 2 instances of this:
layout: 'vertical' || 'flex',
// and
<img
src={`${BASE_PATH}` + menu.icon ?? `/img/icons/menu/${id}.svg`}
className="w-5 rounded"
/>tailwindlabs/headlessui Definitely a bug: let isParentDisabled = parent?.getAttribute('disabled') === '' ?? falsetremorlabs/tremor Two bugs with eight manifestations here: disabled={!hasScroll?.left ?? true}
disabled={!hasScroll?.right ?? true}
// six instances of this exact line:
const prev = previousValues ? previousValues[dataKey] : undefined;
const percentage = ((value - prev) / prev) * 100 ?? undefined;Uniswap/web3-react This appears to be less of a mechanical error and more of just a wat. This isn't how React actually works; unclear if there's relevant syntactic processing going on here or what. // to ensure connectors remain fresh, we condition re-renders on loaded, isActive and chainId
void loaded && isActive && chainIdVendicated/Vencord Definitely a bug: wrapSort(comparator: Function, row: any) {
return row.type === 5
? -UserAffinitiesStore.getUserAffinity(row.user.id)?.affinity ?? 0
: comparator(row);
},vercel/ai-chatbot Definitely a bug, though I don't know what they really meant here const fileName = window.prompt('Enter file name' || '', suggestedFileName)withfig/autocomplete Definitely a bug: const name = ("FileSystemArn" ? elm["FileSystemArn"] : elm) as string;zu1k/bs-core Definitely a bug: <Description name={`${t('book.md5') ?? 'MD5'}: `}>
{(
<Button
colorScheme="gray"
variant="ghost"
size="xs"
leftIcon={<LinkIcon />}
onClick={() => {
navigator.clipboard.writeText(md5?.toLowerCase() ?? 'Unknown');
setMd5Copied(true);
setTimeout(() => {
setMd5Copied(false);
}, 2000);
}}
>
{md5Copied ? t('copied') : md5}
</Button>
) ||
t('book.unknown') ||
'Unknown'}
</Description> |
|
Coallating the errors that people seem to be making here. 100.0% of this code is extremely indicative of a logic error of some sort.
|
sandersn
reviewed
Jul 11, 2024
|
@typescript-bot pack this |
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
…oSuspectTruthyChecks
RyanCavanaugh
force-pushed
the
noSuspectTruthyChecks
branch
from
062703b to
701b54a
Compare
jakebailey
approved these changes
Jul 20, 2024
|
I've barely skimmed the code, but I looked at the error messages and tests and both are in a good place. |
sandersn
approved these changes
Jul 22, 2024
| @@ -17,6 +17,7 @@ enum E { x } | |||
|
|
|||
| var a: any; | |||
| >a : any | |||
| > : ^^^ | |||
There was a problem hiding this comment.
What happened that updated the types baselines like this?
There was a problem hiding this comment.
This is a goofy quirk of the types baselines; in typeWriter.ts:
// Distinguish `errorType`s from `any`s; but only if the file has no errors.So this types file changed because computedPropertyNames48_ES5.errors.txt was emitted.
There was a problem hiding this comment.
(Honestly, I wonder if it's worth the special case; I noticed it separately while working on something else)
DanielRosenwasser
approved these changes
Jul 22, 2024
|
Actually - if you could make sure all of the outer expression cases at least have tests that'd be ideal. |
|
@typescript-bot pack this |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
sandersn
added a commit
to sandersn/DefinitelyTyped
that referenced
this pull request
Jul 23, 2024
From a recent TS PR : microsoft/TypeScript#59217
6 tasks
|
Could there be a tsconfig option to disable this feature? Below are some patterns that no longer work after this change: "hardcoded value" ?? /* not working yet */ realValue()
"TODO fix this" && { not: "fully working yet" }
"force 'A' until TICKET-123 resolves things" || condition() ? "A" : "B"
{ width: 123 ?? /* Doesn't work */ `calc(100vh - var(--TOP_NAV_HEIGHT, 0px) + ${TOKENS.SPACING[4]}px)` }Comments could be used, but then disabled code is harder to separate from commented text: "hardcoded value" /* not working yet `realValue()` */
/* TODO fix this */ { not: "fully working yet" }
/* force 'A' until TICKET-123 resolves things */ true || condition() ? "A" : "B"
{ width: 123 /* Doesn't work `` `calc(100vh - var(--TOP_NAV_HEIGHT, 0px) + ${TOKENS.SPACING[4]}px)` `` */ }Other ergonomic considerations:
An advantage of ESLint's This feature would be more useful to me if it were restricted to
|
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by @mjbvz's suggestion to disallow
if (/regex/), this PR implements a series of checks that mostly overlap with ESLint'sno-constant-binary-expressionrule, which found some great stuff in their v8.14.The rule here is that it's an error to check for the truthiness of an expression which has "static truthy semantics", that is, always evaluates to the same truthiness. Examples:
if ([someArr]) {is illegal (array literals are always truthy)const foo = { ...someObj } || { }is illegal (object literals are always truthy, even those only spread innull)The expressions
false,true,0, and1are excluded from this check, since code likewhile (true) {,while(1) {,do { } while (false),if (true || i_am_just_debugging) {are common and unlikely to represent programmer errors.The same rule is applied to nullishness (the thing
??checks for). Because??'s precedence can be surprising, this finds a huge number of "missing parens" bugs.This check is entirely syntactic, because with
noUncheckedIndexAccessoff, "hidden" nulls/undefined are idiomatically observed in a variety of places:A breakdown of all the user test hits can be found at #59217 (comment) (part 1) and #59217 (comment) (part 2).
There are a lot, but 100% of these appear to be legitimate bugs in code, so I don't think it's necessary to add a flag for this.