Loosen lib types for string methods that internally call %Symbol.*% methods - #64442
graphemecluster (graphemecluster) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new replacement signatures introduce implicit-any regressions and the custom-symbol inference behavior lacks focused tests.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: None
What changed in this PR
This PR broadens String well-known-symbol method typings to preserve custom argument and return types.
Changes:
- Adds generic custom-symbol overloads for matching, replacing, searching, and splitting.
- Expands
matchAll/replaceAllinputs and revises documentation. - Updates affected compiler and call-hierarchy baselines.
| File | Description |
|---|---|
tsc/testdata/baselines/reference/fourslash/callHierarchy/callHierarchyInPropDeclarationOfExportedDefaultClass1.callHierarchy.txt |
Updates shifted library spans. |
tsc/testdata/baselines/reference/fourslash/callHierarchy/callHierarchyIncomingCallsObjectLiteralMethodInStringLiteralComputedProperty.callHierarchy.txt |
Updates split hierarchy output. |
tsc/testdata/baselines/reference/fourslash/callHierarchy/callHierarchyIncomingCallsObjectLiteralMethodInIdentifierComputedProperty.callHierarchy.txt |
Updates split hierarchy output. |
tsc/testdata/baselines/reference/fourslash/callHierarchy/callHierarchyIncomingCallsObjectLiteralMethodInExpressionComputedProperty.callHierarchy.txt |
Updates split hierarchy output. |
tsc/testdata/baselines/reference/fourslash/callHierarchy/callHierarchyIncomingCallsNoCrashArrayPush.callHierarchy.txt |
Updates shifted library spans. |
tsc/testdata/baselines/reference/conformance/useRegexpGroups.types |
Records generic match overload. |
tsc/testdata/baselines/reference/conformance/parserRealSource11.types |
Records revised replace and split types. |
tsc/testdata/baselines/reference/conformance/parserRealSource11.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/conformance/parserharness.types |
Records consolidated replace signature. |
tsc/testdata/baselines/reference/conformance/parserharness.symbols |
Updates replace symbols. |
tsc/testdata/baselines/reference/conformance/parserArgumentList1.types |
Records lost callback contextual types. |
tsc/testdata/baselines/reference/conformance/parserArgumentList1.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/conformance/parserArgumentList1.errors.txt |
Adds callback implicit-any diagnostics. |
tsc/testdata/baselines/reference/conformance/parser630933.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/stringMatchAll.types |
Records expanded matchAll signatures. |
tsc/testdata/baselines/reference/compiler/stringMatchAll.symbols |
Records added matchAll declaration. |
tsc/testdata/baselines/reference/compiler/regExpWithSlashInCharClass.types |
Records revised replace types. |
tsc/testdata/baselines/reference/compiler/regExpWithSlashInCharClass.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/compiler/regexMatchAll.types |
Updates parameter naming. |
tsc/testdata/baselines/reference/compiler/regexMatchAll-esnext.types |
Updates parameter naming. |
tsc/testdata/baselines/reference/compiler/overloadResolutionOverNonCTLambdas.types |
Records revised replace overloads. |
tsc/testdata/baselines/reference/compiler/overloadResolutionOverNonCTLambdas.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/compiler/noImplicitAnyInContextuallyTypesFunctionParamter.types |
Records revised replace overloads. |
tsc/testdata/baselines/reference/compiler/noImplicitAnyInContextuallyTypesFunctionParamter.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/compiler/narrowingWithNonNullExpression.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/initializedDestructuringAssignmentTypes.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/flatArrayNoExcessiveStackDepth.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/firstMatchRegExpMatchArray.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/extendGlobalThis.types |
Records generic split overload. |
tsc/testdata/baselines/reference/compiler/doYouNeedToChangeYourTargetLibraryES2016Plus.types |
Records generic match overload. |
tsc/testdata/baselines/reference/compiler/controlFlowPropertyDeclarations.types |
Records revised replace types. |
tsc/testdata/baselines/reference/compiler/controlFlowPropertyDeclarations.symbols |
Updates replace declarations. |
tsc/testdata/baselines/reference/compiler/bestChoiceType.types |
Records generic match overload. |
tsc/internal/bundled/libs/lib.es5.d.ts |
Revises core string signatures and docs. |
tsc/internal/bundled/libs/lib.es2021.string.d.ts |
Expands replaceAll overloads. |
tsc/internal/bundled/libs/lib.es2020.symbol.wellknown.d.ts |
Revises RegExp.matchAll documentation. |
tsc/internal/bundled/libs/lib.es2020.string.d.ts |
Expands String.matchAll. |
tsc/internal/bundled/libs/lib.es2015.symbol.wellknown.d.ts |
Generalizes well-known-symbol dispatch. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| * @param replaceValue A value to be passed into {@linkcode searchValue}. | ||
| */ | ||
| replace(searchValue: { [Symbol.replace](string: string, replacer: (substring: string, ...args: any[]) => string): string; }, replacer: (substring: string, ...args: any[]) => string): string; | ||
| replace<This, T, R>(this: This, searchValue: { [Symbol.replace](string: This, replaceValue: T): R; }, replaceValue: T): R; |
There was a problem hiding this comment.
I think a while back we decided not to thread through this parameters in methods with callbacks because of perf costs. Maybe it matters less now.
|
TypeScript Bot (@typescript-bot) test it |
|
Daniel Rosenwasser (@DanielRosenwasser) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
Hey Daniel Rosenwasser (@DanielRosenwasser), the results of running the DT tests are ready. Everything looks the same! |
This is intended as a more general fix to #54387.
Out of
String.{replace,match}{,All},.searchand.split, modify or add signatures involving custom objects implementing well-known symbol methods, the parameter and return types of which are now preserved (yes, per the spec they just directly call the raw value and return the raw result. There is no type coercion involved, so I think it is acceptable to define them like this).The addition of
replaceAllandmatchAllsignatures ensures their definitions are parallel to their equivalent withoutAll.The wordings of the documentation are adjusted accordingly.
Fixes #54387
For historical reference, most of this PR was extracted from #60249 (generic RegExp is excluded of course).Fixes #61448