fix(eslint-plugin): use consistent naming for asserting types and casting values #10472

ronami · 2024-12-07T17:14:56Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unsafe-type-assertion] Remove the term "cast" from error message report #10458
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR addresses #10458 and makes consistent use of asserting vs casting.

There is some inconsistency for casting values: to cast (no-extra-boolean-cast), to coerce (e.g. no-implicit-coercion, MDN), to convert (upcoming no-unnecessary-type-conversion). To my understanding, these all mean the same thing.

I've used consistent language for casting a value (currently using to cast). Please let me know if this sounds OK or if I should change it back.

typescript-eslint · 2024-12-07T17:14:58Z

Thanks for the PR, @ronami!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-12-07T17:15:17Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`f003891`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/675dcf12358ef90008b5f44b
😎 Deploy Preview	https://deploy-preview-10472--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-12-07T17:19:36Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f003891. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

kirkwaiblinger

Noting that there are a fair number of problematic usages of "cast" remaining that we'll want to address still, either here or in a followup

kirkwaiblinger · 2024-12-07T18:28:38Z

packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.mdx

@@ -176,7 +176,7 @@ Also, if you would like to ignore all primitives types, you can set `ignorePrimi

 {/* insert option description */}

-Whether to ignore expressions that coerce a value into a boolean: `Boolean(...)`.
+Whether to ignore expressions that cast a value into a boolean: `Boolean(...)`.


Let's leave this this as "coerce".

ronami · 2024-12-07T19:02:06Z

Noting that there are a fair number of problematic usages of "cast" remaining that we'll want to address still, either here or in a followup

I've just realized my VSCode had match-case and match-whole-word locked in (🙈). I think I've managed to update every usage of cast that refers to a type assertion.

Thanks!

packages/eslint-plugin/docs/rules/explicit-function-return-type.mdx

kirkwaiblinger · 2024-12-14T08:22:19Z

Looks like there's still two more places where "cast" is used in user-facing strings...

typescript-eslint/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

Line 71 in d09d36d

'An explicit comparison or type cast is required.',

typescript-eslint/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

Line 103 in d09d36d

'Explicitly cast value to a boolean (`Boolean(value)`)',

... and then also here, though I'm honestly not sure why we even have this:

typescript-eslint/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx

Lines 171 to 201 in d09d36d

    
           ## Fixes and Suggestions 
        
           This rule provides following fixes and suggestions for particular types in boolean context: 
        
           - `boolean` - Always allowed - no fix needed. 
        
           - `string` - (when `allowString` is `false`) - Provides following suggestions: 
        
             - Change condition to check string's length (`str` → `str.length > 0`) 
        
             - Change condition to check for empty string (`str` → `str !== ""`) 
        
             - Explicitly cast value to a boolean (`str` → `Boolean(str)`) 
        
           - `number` - (when `allowNumber` is `false`): 
        
             - For `array.length` - Provides **autofix**: 
        
               - Change condition to check for 0 (`array.length` → `array.length > 0`) 
        
             - For other number values - Provides following suggestions: 
        
               - Change condition to check for 0 (`num` → `num !== 0`) 
        
               - Change condition to check for NaN (`num` → `!Number.isNaN(num)`) 
        
               - Explicitly cast value to a boolean (`num` → `Boolean(num)`) 
        
           - `object | null | undefined` - (when `allowNullableObject` is `false`) - Provides **autofix**: 
        
             - Change condition to check for null/undefined (`maybeObj` → `maybeObj != null`) 
        
           - `boolean | null | undefined` - Provides following suggestions: 
        
             - Explicitly treat nullish value the same as false (`maybeBool` → `maybeBool ?? false`) 
        
             - Change condition to check for true/false (`maybeBool` → `maybeBool === true`) 
        
           - `string | null | undefined` - Provides following suggestions: 
        
             - Change condition to check for null/undefined (`maybeStr` → `maybeStr != null`) 
        
             - Explicitly treat nullish value the same as an empty string (`maybeStr` → `maybeStr ?? ""`) 
        
             - Explicitly cast value to a boolean (`maybeStr` → `Boolean(maybeStr)`) 
        
           - `number | null | undefined` - Provides following suggestions: 
        
             - Change condition to check for null/undefined (`maybeNum` → `maybeNum != null`) 
        
             - Explicitly treat nullish value the same as 0 (`maybeNum` → `maybeNum ?? 0`) 
        
             - Explicitly cast value to a boolean (`maybeNum` → `Boolean(maybeNum)`) 
        
           - `any` and `unknown` - Provides following suggestions: 
        
             - Explicitly cast value to a boolean (`value` → `Boolean(value)`)

kirkwaiblinger

couple minor comments

codecov · 2024-12-14T08:29:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.78%. Comparing base (2c8a75e) to head (f003891).
Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10472      +/-   ##
==========================================
+ Coverage   86.76%   86.78%   +0.02%     
==========================================
  Files         444      445       +1     
  Lines       15311    15365      +54     
  Branches     4457     4475      +18     
==========================================
+ Hits        13285    13335      +50     
- Misses       1672     1675       +3     
- Partials      354      355       +1

Flag	Coverage Δ
unittest	`86.78% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...slint-plugin/src/rules/no-unsafe-type-assertion.ts	`100.00% <ø> (ø)`
...gin/src/rules/non-nullable-type-assertion-style.ts	`100.00% <ø> (ø)`
...t-plugin/src/rules/prefer-reduce-type-parameter.ts	`100.00% <ø> (ø)`
...int-plugin/src/rules/strict-boolean-expressions.ts	`100.00% <ø> (ø)`

... and 15 files with indirect coverage changes

ronami · 2024-12-14T08:56:04Z

Looks like there's still two more places where "cast" is used in user-facing strings...

typescript-eslint/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

Line 71 in d09d36d

'An explicit comparison or type cast is required.',

typescript-eslint/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

Line 103 in d09d36d

'Explicitly cast value to a boolean (`Boolean(value)`)',

... and then also here, though I'm honestly not sure why we even have this:

typescript-eslint/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx

Lines 171 to 201 in d09d36d

## Fixes and Suggestions

This rule provides following fixes and suggestions for particular types in boolean context:

- `boolean` - Always allowed - no fix needed.

- `string` - (when `allowString` is `false`) - Provides following suggestions:

- Change condition to check string's length (`str` → `str.length > 0`)

- Change condition to check for empty string (`str` → `str !== ""`)

- Explicitly cast value to a boolean (`str` → `Boolean(str)`)

- `number` - (when `allowNumber` is `false`):

- For `array.length` - Provides **autofix**:

- Change condition to check for 0 (`array.length` → `array.length > 0`)

- For other number values - Provides following suggestions:

- Change condition to check for 0 (`num` → `num !== 0`)

- Change condition to check for NaN (`num` → `!Number.isNaN(num)`)

- Explicitly cast value to a boolean (`num` → `Boolean(num)`)

- `object | null | undefined` - (when `allowNullableObject` is `false`) - Provides **autofix**:

- Change condition to check for null/undefined (`maybeObj` → `maybeObj != null`)

- `boolean | null | undefined` - Provides following suggestions:

- Explicitly treat nullish value the same as false (`maybeBool` → `maybeBool ?? false`)

- Change condition to check for true/false (`maybeBool` → `maybeBool === true`)

- `string | null | undefined` - Provides following suggestions:

- Change condition to check for null/undefined (`maybeStr` → `maybeStr != null`)

- Explicitly treat nullish value the same as an empty string (`maybeStr` → `maybeStr ?? ""`)

- Explicitly cast value to a boolean (`maybeStr` → `Boolean(maybeStr)`)

- `number | null | undefined` - Provides following suggestions:

- Change condition to check for null/undefined (`maybeNum` → `maybeNum != null`)

- Explicitly treat nullish value the same as 0 (`maybeNum` → `maybeNum ?? 0`)

- Explicitly cast value to a boolean (`maybeNum` → `Boolean(maybeNum)`)

- `any` and `unknown` - Provides following suggestions:

- Explicitly cast value to a boolean (`value` → `Boolean(value)`)

I've noticed these, too, but to my understanding, the meaning in those cases is actually to cast rather than to assert.

They all require the developer to make a runtime change rather than a type-only assertion. For example, these two reports (1, 2) include a suggestion to "cast" a value to a boolean with Boolean(value).

I think these are similar to the coerce meaning in #10472 (comment), and also to the meaning of casting/coercing in no-extra-boolean-cast.

I'm not sure which terminology to use in those cases. There are some places we use the term coercing, some casting, and here (open PR) we use converting.

kirkwaiblinger · 2024-12-14T09:50:19Z

I've noticed these, too, but to my understanding, the meaning in those cases is actually to cast rather than to assert.

They all require the developer to make a runtime change rather than a type-only assertion. For example, these two reports (1, 2) include a suggestion to "cast" a value to a boolean with Boolean(value).

I think these are similar to the coerce meaning in #10472 (comment), and also to the meaning of casting/coercing in no-extra-boolean-cast.

I'm not sure which terminology to use in those cases. There are some places we use the term coercing, some casting, and here (open PR) we use converting.

Gotcha, yeah, I see where you're coming from. My spiel on this is: within a pure JS context, one can (just barely) get away with using "cast" as in no-extra-boolean-cast. IMO it's still not a good term for what it's describing, but it is at least unambiguous, since "cast" has no other possible meaning within JS. But, throw TS into the mix, and "cast" means different things to different people, and even sometimes different things to the same people in different contexts 😆. Therefore, I don't think we should use it for either the "type assertion" or "type conversion/coersion" meanings at all in a TS context.

So, then, to your specific question, I think we should change those terms in SBE, and we should change them to "convert"/"conversion". My vague idea of "coercion" vs "conversion" is that "coercion" is what happens when the language performs a type conversion implicitly, usually because objects of incompatible types are interacting, like true + ''. So I'd think of Boolean(x) as a conversion but not a coercion (but all coercions are conversions). Don't know if that's quite right, though; @Josh-Cena is generally our expert on these sorts of things!

Incidentally, regarding #10472 (comment), that does imply that the word "convert" would be most in line with my previous paragraph. But, the option name is ignoreBooleanCoercion, so I think our hands are tied to use "coerce" in the description.

https://github.com/typescript-eslint/typescript-eslint/pull/10472/files/34610c6a07b3edc3acde8ae158a0f7472ced0f53#diff-a1ede3fb0351c7f188d7e9fcc7b931a865ebb2a0197b2323b3e104623812f76dR175-R179

ronami · 2024-12-14T15:14:37Z

I've noticed these, too, but to my understanding, the meaning in those cases is actually to cast rather than to assert.
They all require the developer to make a runtime change rather than a type-only assertion. For example, these two reports (1, 2) include a suggestion to "cast" a value to a boolean with Boolean(value).
I think these are similar to the coerce meaning in #10472 (comment), and also to the meaning of casting/coercing in no-extra-boolean-cast.
I'm not sure which terminology to use in those cases. There are some places we use the term coercing, some casting, and here (open PR) we use converting.

Gotcha, yeah, I see where you're coming from. My spiel on this is: within a pure JS context, one can (just barely) get away with using "cast" as in no-extra-boolean-cast. IMO it's still not a good term for what it's describing, but it is at least unambiguous, since "cast" has no other possible meaning within JS. But, throw TS into the mix, and "cast" means different things to different people, and even sometimes different things to the same people in different contexts 😆. Therefore, I don't think we should use it for either the "type assertion" or "type conversion/coersion" meanings at all in a TS context.

So, then, to your specific question, I think we should change those terms in SBE, and we should change them to "convert"/"conversion". My vague idea of "coercion" vs "conversion" is that "coercion" is what happens when the language performs a type conversion implicitly, usually because objects of incompatible types are interacting, like true + ''. So I'd think of Boolean(x) as a conversion but not a coercion (but all coercions are conversions). Don't know if that's quite right, though; @Josh-Cena is generally our expert on these sorts of things!

Incidentally, regarding #10472 (comment), that does imply that the word "convert" would be most in line with my previous paragraph. But, the option name is ignoreBooleanCoercion, so I think our hands are tied to use "coerce" in the description.

https://github.com/typescript-eslint/typescript-eslint/pull/10472/files/34610c6a07b3edc3acde8ae158a0f7472ced0f53#diff-a1ede3fb0351c7f188d7e9fcc7b931a865ebb2a0197b2323b3e104623812f76dR175-R179

Yes! I can't agree more about how confusing "to cast" is, especially with TS, as it can have so many meanings.

I've updated the PR and adjusted the "to cast" usage in SBE, too; thanks!

kirkwaiblinger

Love it! Thank you for doing this!! This had been bugging me for a while 😆

JoshuaKGoldberg

Lovely!

@kirkwaiblinger

##### [v8.18.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8181-2024-12-16) ##### 🩹 Fixes - **scope-manager:** visit params decorator before nest scope ([#10475](typescript-eslint/typescript-eslint#10475)) - **eslint-plugin:** \[no-unnecessary-condition] better message when comparing between literal types ([#10454](typescript-eslint/typescript-eslint#10454)) - **eslint-plugin:** use consistent naming for asserting types and casting values ([#10472](typescript-eslint/typescript-eslint#10472)) - **eslint-plugin:** \[no-unnecessary-boolean-literal-compare] flag values of a type parameter with boolean type constraints ([#10474](typescript-eslint/typescript-eslint#10474)) - **eslint-plugin:** handle string like index type ([#10460](typescript-eslint/typescript-eslint#10460)) - **eslint-plugin:** \[no-unnecessary-template-expression] don't report when an expression includes comment ([#10444](typescript-eslint/typescript-eslint#10444)) ##### ❤️ Thank You - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@kirkwaiblinger

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.18.0 | 8.18.1 | | npm | @typescript-eslint/parser | 8.18.0 | 8.18.1 | ## [v8.18.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8181-2024-12-16) ##### 🩹 Fixes - **scope-manager:** visit params decorator before nest scope ([#10475](typescript-eslint/typescript-eslint#10475)) - **eslint-plugin:** \[no-unnecessary-condition] better message when comparing between literal types ([#10454](typescript-eslint/typescript-eslint#10454)) - **eslint-plugin:** use consistent naming for asserting types and casting values ([#10472](typescript-eslint/typescript-eslint#10472)) - **eslint-plugin:** \[no-unnecessary-boolean-literal-compare] flag values of a type parameter with boolean type constraints ([#10474](typescript-eslint/typescript-eslint#10474)) - **eslint-plugin:** handle string like index type ([#10460](typescript-eslint/typescript-eslint#10460)) - **eslint-plugin:** \[no-unnecessary-template-expression] don't report when an expression includes comment ([#10444](typescript-eslint/typescript-eslint#10444)) ##### ❤️ Thank You - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

use consistent naming for asserting types and casting values

34610c6

ronami changed the title ~~Use consistent naming for asserting types and casting values~~ fix(eslint-plugin): Use consistent naming for asserting types and casting values Dec 7, 2024

ronami changed the title ~~fix(eslint-plugin): Use consistent naming for asserting types and casting values~~ fix(eslint-plugin): use consistent naming for asserting types and casting values Dec 7, 2024

ronami marked this pull request as ready for review December 7, 2024 17:25

kirkwaiblinger requested changes Dec 7, 2024

View reviewed changes

ronami added 3 commits December 7, 2024 20:56

address remaining wrong usages of 'cast'

cf0d494

don't change 'coerce' into 'cast'

d4fbfec

leftovers and snapshots

ef89183

rename documentation variable to match one another

5e067a0

ronami commented Dec 7, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/explicit-function-return-type.mdx Outdated Show resolved Hide resolved

ronami requested a review from kirkwaiblinger December 7, 2024 19:45

kirkwaiblinger reviewed Dec 14, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Dec 14, 2024

ronami requested a review from kirkwaiblinger December 14, 2024 09:06

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 14, 2024

replace 'cast' terminology in sbe to use 'convert' instead

06056b8

remove unnecessary angle-bracket examples

f003891

kirkwaiblinger approved these changes Dec 14, 2024

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 14, 2024

kirkwaiblinger requested a review from JoshuaKGoldberg December 14, 2024 19:15

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 14, 2024

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 14, 2024

JoshuaKGoldberg approved these changes Dec 14, 2024

View reviewed changes

JoshuaKGoldberg merged commit 984f177 into typescript-eslint:main Dec 14, 2024
67 checks passed

controversial mentioned this pull request Dec 16, 2024

fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): use consistent naming for asserting types and casting values #10472

fix(eslint-plugin): use consistent naming for asserting types and casting values #10472

ronami commented Dec 7, 2024 •

edited

Loading

typescript-eslint bot commented Dec 7, 2024

netlify bot commented Dec 7, 2024 •

edited

Loading

nx-cloud bot commented Dec 7, 2024 •

edited

Loading

kirkwaiblinger left a comment

kirkwaiblinger Dec 7, 2024

ronami commented Dec 7, 2024 •

edited

Loading

kirkwaiblinger commented Dec 14, 2024

kirkwaiblinger left a comment

codecov bot commented Dec 14, 2024 •

edited

Loading

ronami commented Dec 14, 2024 •

edited

Loading

kirkwaiblinger commented Dec 14, 2024

ronami commented Dec 14, 2024

kirkwaiblinger left a comment

JoshuaKGoldberg left a comment

fix(eslint-plugin): use consistent naming for asserting types and casting values #10472

fix(eslint-plugin): use consistent naming for asserting types and casting values #10472

Conversation

ronami commented Dec 7, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Dec 7, 2024

netlify bot commented Dec 7, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Dec 7, 2024 • edited Loading

☁️ Nx Cloud Report

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Dec 7, 2024

Choose a reason for hiding this comment

ronami commented Dec 7, 2024 • edited Loading

kirkwaiblinger commented Dec 14, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2024 • edited Loading

Codecov Report

ronami commented Dec 14, 2024 • edited Loading

kirkwaiblinger commented Dec 14, 2024

ronami commented Dec 14, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ronami commented Dec 7, 2024 •

edited

Loading

netlify bot commented Dec 7, 2024 •

edited

Loading

nx-cloud bot commented Dec 7, 2024 •

edited

Loading

ronami commented Dec 7, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

ronami commented Dec 14, 2024 •

edited

Loading