Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
Description
Before You File a Proposal Please Confirm You Have Done The Following...
- I havesearched for related issues and found none that match my proposal.
- I have searched thecurrent rule list and found no rules that match my proposal.
- I haveread the FAQ and my problem is not listed.
Relevant Package
eslint-plugin
My proposal is suitable for this project
- I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).
Description
Sorry for the delay in filing this—better late than never! As I dove in, I realized this was much more complex than expected, which led to some rabbit holes (alongside the usual distractions of life).
Long writeup, even after trimming things down for conciseness. Feel free to skim or skip to thesummary at the bottom to start with.
Background
In#8765, we decided to leave aTODO to handle unnamed class/interface members properly—specifically, waiting for V8'schecker.isTypeAssignableTo to properly check index signatures against inherited ones. (Relevant discussion)
What we don't yet check
After looking more into it, I've realizedno-misused-promises not only doesn't check index signatures against inherited signatures, but also doesn't check index signatures in almost any case.
E.g. this passes without anyno-misused-promises errors being flagged:
interfaceInvalidInterface{[syncKey:string]:()=>void;[asyncFetcher: `fetch${string}`]:()=>Promise<void>;// this should error but doesn'tasyncMethod():Promise<void>;// this should error but doesn'tasyncProperty:()=>Promise<void>;// this should error but doesn't}interfaceInvalidSubInterfaceextendsInvalidInterface{anotherAsyncMethod():Promise<void>;// this should error but doesn'tanotherAsyncProperty:()=>Promise<void>;// this should error but doesn't[asyncKey:string]:()=>Promise<void>;// this should error but doesn't}constinvalidMethodObject:Record<string,()=>void>={asyncMethod(){returnPromise.resolve();},// this should error but doesn't}
So, an implementation of checking index signatures for misused Promises would ideally catch these other cases as well, even though the original TODO in#8765 was specifically about checking a type's index signatures against inherited index signatures.
For a more specific and comprehensive list of what's not yet checked:
What's not yet checked by `no-misused-promises`
- Classes
- Methods vs index signatures within the class, and vs inherited index signatures
- Function-type properties vs index signatures within the class, and vs inherited index signatures
- Index signatures vs each other within the class, and vs inherited index signatures
- Index signatures vsinherited methods/properties (e.g. a
void-returning index shouldn't be allowed in a class that inherits aPromise-returning method)
- Interfaces (same as Classes)
- Type literals
- Index signatures vsmethods, properties, and other indexes within the type literal
- Note: We don't yet have any
no-misused-promisesrule listeners that run onTSTypeLiterals at all.- This makes senseif not checking index signatures, because (a) type literals can't inherit from another type, and (b) can't contain duplicate identifiers. So there's no way to have a misused Promise within a type literal except when an index signature is involved (unless I'm missing something).
- Objects
- Methods vs index signatures in the object's contextual type
What we do already check
no-misused-promises does already catch one case of Promise-void mismatches involving index signatures:object properties (as opposed toobject methods).
Thanks tochecker.getContextualType (which we also use for methods, but for some reason doesn't behave the same for them),checksVoidReturn.properties will flag these:
functionuseSyncFetcherObj(o:{[syncFetcherKey:string]:()=>void}):void{o.fetchSomething();}useSyncFetcherObj({fetchSomethingAsyncProperty:()=>Promise.resolve()});// Promise-returning function provided to property where a void return was expected.typeSyncFetcherObj=Record<string,()=>void>;constasyncFetcherPropertyObj:SyncFetcherObj={fetchSomethingAsyncProperty:()=>Promise.resolve(),// Promise-returning function provided to property where a void return was expected.};
But not these:
useSyncFetcherObj({asyncfetchSomethingAsyncMethod(){returnPromise.resolve();}});// this should error but doesn'tconstasyncFetcherMethodObj:SyncFetcherObj={asyncfetchSomethingAsyncMethod(){awaitPromise.resolve();},// this should error but doesn't};
Where to go from here
The practical impact of this change is unclear—beyond technical completeness, it's not obvious if users would benefit enough to justify it. Implementation is possible, but comes with tradeoffs:
Complexity / maintainability
Checking index signatures is trickier than expected, because the TypeScript compiler API doesn't expose all the relevant functions we'd need. On top of likely needing a new option for whether to check index signatures, we'd need to usechecker.isTypeAssignableTo to manually implement functions ourselves like these:
New file: `packages/eslint-plugin/src/util/indexSignatureUtils.ts`
importtype{ParserServicesWithTypeInformation}from'@typescript-eslint/utils';import*astsutilsfrom'ts-api-utils';import*astsfrom'typescript';import{getTypeOfPropertyName}from'./getTypeOfPropertyName';import{isStaticMember}from'./isStaticMember';import{returnsVoidAndNotThenable}from'./returnTypeUtils';exportinterfaceHeritageIndexInfo{heritageType:ts.Type;indexInfo:ts.IndexInfo;}exportfunctiongetHeritageIndexInfos({ checker, heritageTypes,}:{checker:ts.TypeChecker;heritageTypes:ts.Type[];}):HeritageIndexInfo[]{returnheritageTypes.flatMap(heritageType=>{constindexInfos=checker.getIndexInfosOfType(heritageType);returnindexInfos.map(indexInfo=>({ heritageType, indexInfo}));});}exportfunctionisApplicableIndexInfoForKeyType({ checker, indexInfo, keyType,}:{checker:ts.TypeChecker;keyType:ts.Type;indexInfo:ts.IndexInfo;}):boolean{const{keyType:targetIndexKeyType}=indexInfo;returnisApplicableIndexKeyType({ checker,sourceIndexKeyType:keyType, targetIndexKeyType,});}/** * Implemented based on `checker.isApplicableIndexType` (not exposed in the public API). */exportfunctionisApplicableIndexKeyType({ checker, sourceIndexKeyType, targetIndexKeyType,}:{checker:ts.TypeChecker;sourceIndexKeyType:ts.Type;targetIndexKeyType:ts.Type;}):boolean{if(checker.isTypeAssignableTo(sourceIndexKeyType,targetIndexKeyType)){returntrue;}if(targetIndexKeyType===checker.getStringType()&&checker.isTypeAssignableTo(sourceIndexKeyType,checker.getNumberType())){// A 'string' index signature applies to types assignable to 'string' or 'number'returntrue;}// TS checks for numeric string types and numeric literal names like this, but this might be more// trouble than it's worth for us to implement?// if (// targetIndexKeyType === checker.getNumberType() &&// (isNumericStringType(checker, sourceIndexKeyType) ||// isNumericLiteralName(sourceIndexKeyType))// ) {// // A 'number' index signature applies to types assignable to 'number', `${number}`, and numeric string literal types// return true;// }returnfalse;}functionisNumericStringType(checker:ts.TypeChecker,type:ts.Type):boolean{if(!tsutils.isTemplateLiteralType(type)){returnfalse;}// return type.texts.every(text => tsutils.isTypeFlagSet(checker.getTypeAtLocation(text), ts.TypeFlags.NumberLike)) && type.types.every(t => tsutils.isTypeFlagSet(t, ts.TypeFlags.NumberLike));return(tsutils.isTypeFlagSet(type,ts.TypeFlags.String)&&tsutils.isTypeFlagSet(type,ts.TypeFlags.NumberLike));}functionisNumericLiteralName(type:ts.Type):boolean{// TODO: Implement this functionreturn(tsutils.isStringLiteralType(type)&&Number(type.value).toString()===type.value);}exportfunctionisApplicableIndexInfoForNamedMember({ checker, indexInfo, member, memberName, services,}:{checker:ts.TypeChecker;indexInfo:ts.IndexInfo;member:ts.ClassElement|ts.TypeElement;memberName:ts.PropertyName;services:ParserServicesWithTypeInformation;}):boolean{if(isStaticIndexInfo(indexInfo)&&!isStaticMember({ member, services})){returnfalse;}constmemberNameType=getTypeOfPropertyName(checker,memberName);returnisApplicableIndexInfoForKeyType({ checker, indexInfo,keyType:memberNameType,});}exportfunctiongetIndexSignatureDeclarationKeyType({ checker, indexSignatureDeclaration,}:{checker:ts.TypeChecker;indexSignatureDeclaration:ts.IndexSignatureDeclaration;}):ts.Type{// eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- Index signatures always have exactly one parameterconsttypeNode=indexSignatureDeclaration.parameters[0].type!;returnchecker.getTypeAtLocation(typeNode);}exportfunctionindexInfoReturnsVoidAndNotThenable(checker:ts.TypeChecker,indexInfo:ts.IndexInfo,):boolean{const{ declaration, type}=indexInfo;if(!declaration){returnfalse;}returnreturnsVoidAndNotThenable(checker,declaration,type);}exportfunctionisStaticIndexInfo(indexInfo:ts.IndexInfo):boolean{constmodifiers=indexInfo.declaration?.modifiers;if(modifiers==null){returnfalse;}returnmodifiers.some(modifier=>modifier.kind===ts.SyntaxKind.StaticKeyword,);}
Test case coverage
There are a lot of cases and combinations to cover:
Cases to cover in rule listeners and test cases
- Checking against methods/properties/indexeswithin a type, and againstinherited ones
- Both types of inheritance:
extendsclauses andimplementsclauses - Node types: Classes and class expressions, interfaces, type literals, objects
- Both index signatures and Records (although I think they are treated the same under the hood, so we wouldn't need extra code to handle Records; just more test cases)
- Inheriting bothsync and async types (e.g. declaring a void-returning index signature within a class that inherits an async method should error)
- Handlingunions of types (both for index key types and index value types)
So, good test coverage would both bloatpackages/eslint-plugin/tests/rules/no-misused-promises.test.ts even more than it already is (2651 lines already), and be quite a lot of cognitive overhead. I've already written about 1000 lines of test cases (and can share them if needed, but wanted to get your thoughts first).
Summary / my thoughts
Although the original TODO was written while thinking we just needed to check index signatures within a class/interface against inherited index signatures, it turns out we don't yet catch most Promise-void mismatches involving index signatures, even without considering inheritance.
Given the complexity involved, and the lack of user demand so far, my inclination is to leave things as-is for now, aside from updating the existing TODO comment.
Appreciate your patience on this! It turned into a bigger deep dive than expected, but hopefully, this breakdown makes it easier to decide the best path forward.
🌱
Additional Info
No response