fix(eslint-plugin): [prefer-nullish-coalescing] treat any/unknown as non-nullable #8262
Conversation
|
Thanks for the PR, @auvred! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I'm good with this solution 👍 I wonder though, shouldn't we recommend switching to nullish coalescing for any/unknown too, with an option to turn that off, just like primitives? Because any/unknown are also nullable, it doesn't make no sense to use nullish coalescing. |
| @@ -309,7 +309,7 @@ export default createRule<Options, MessageIds>({ | |||
| ): void { | |||
| const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); | |||
| const type = checker.getTypeAtLocation(tsNode.left); | |||
| if (!isNullableType(type)) { | |||
| if (!isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)) { | |||
There was a problem hiding this comment.
@Josh-Cena threading #8262 (comment):
I'm good with this solution 👍 I wonder though, shouldn't we recommend switching to nullish coalescing for any/unknown too, with an option to turn that off, just like primitives? Because any/unknown are also nullable, it doesn't make no sense to use nullish coalescing.
Hmm, good question. I think if the type is explicitly not known then it'd be risky to suggest making any concrete changes to its handling. I'd be in support of this as an opt-in in for v6 and maybe an opt-out for v7.
There was a problem hiding this comment.
@dacarley threading #8262 (comment):
Has anyone confirmed that this change fixes #8261 and #8276? If not, I may try to confirm it tomorrow.
If confirmed, can the final question about nullish coalescing be deferred into a new PR?
- ✅ Bug: [prefer-nullish-coalescing] <internal error when linting on binary expression (||) with destruct tuple and object with rename property> #8261 https://deploy-preview-8262--typescript-eslint.netlify.app/play/#ts=5.3.3&fileType=.ts&code=PTAEEkDNQTwewK6gMZwLZoKYDsAuoBLbUSOAGzLgHciBzUOAJ1AEMATNu0AAxYGcUcbH1zdQuOKEaZcCRsQBuLMgkwNopOABpQAQQqEBAeQDSAKBDiYABzUAxOJIC8oANoAjR2UwtsOgN6glOx0AFygnuQ%2BxAC%2BALoA3BZgkAjYyLgEQiSOABQAlOEOkv7JoOXSsvJukMp8mAFBcCHYtOG1ZPWg8WUxZmap6ZnZmgWgpRUycsSuHfWNwZyt7XVq8aBmfWaowvgeLIwLzUttrN1xoC6j%2Bf07IhGXJKvbQvfIj3OYL7tNLfQuLFAAB8gQ8QSgEkA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6KL2jjokcGAC%2BILUA&tsconfig=&tokens=false
- ✅ Bug: [prefer-nullish-coalescing] TypeError: Cannot read properties of undefined (reading 'some') when left-hand-side is unknown #8276
https://deploy-preview-8262--typescript-eslint.netlify.app/play/#ts=5.3.3&fileType=.ts&code=FAEwpgxgNghgTmABBA9gOwM4BdEDcZQCuYAXIoWgNZooDuaA3MPkUgD5uIDeAvk0A&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6KL2jjokcGAC%2BILUA&tsconfig=&tokens=false
|
I can't comment on the orignal PR but I don't really like the change introduce in #8089 The only reason is that it fixes an issue that I don't understand because any assertion on unkown or any is effectively useless, you can look at the inferred type here: https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwDYEMDOa4EE4G8BQccAJijCgPwBccArgHYDW9EA7vQNz4C+++okWHHQBPegjgAzBghgBLCPTgxgaGAAoAlHkJwkweCho4AvHHrBW2LVyJQDtKEpQA6UuS68gA
|
|
Uh oh! @ArnaudBarre, the image you shared is missing helpful alt text. Check #8262 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
…non-nullable (typescript-eslint#8262) * fix(eslint-plugin): [prefer-nullish-coalescing] treat any/unknown as non-nullable * chore: rm unrelated changes * test: add declarations of 'y' variable
PR Checklist
Overview
#8089 introduced change in the
isNullableTypefunction. It started treatinganyandunknownas nullables, it worked forno-unnecesary-type-assertion, but not forprefer-nullish-coalescing.prefer-nullish-coalescingexpectsisNullableTypeto return true only fornullandundefined