Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String.replace() : typescript displays wrong message when 2nd argument is a number #13889

Closed
abenhamdine opened this issue Feb 5, 2017 · 10 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@abenhamdine
Copy link

abenhamdine commented Feb 5, 2017

TypeScript Version: 2.1.5

Code

const myString = 'foo';
const myNumberThatShouldBeAString = 123;
myString.replace('a123', myNumberThatShouldBeAString);

Expected behavior:

Error on 2nd argument with message "Argument of type Number is not assignable to parameter of type 'String'.

Actual behavior:

Error on 1st argument with message "Argument of type '"a123"' is not assignable to parameter of type 'RegExp'."

Indeed, there's no signature for String.replace() in lib.es6.d.ts with number as 2nd argument, so Typescript should find error on the latter, and not on the first argument, which matches one of the signatures.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 5, 2017

this is a byproduct of the overload resolution process. The compiler does not know what was the "best" match given your arguments, so it reports the error on the last signature. the assumption here is that it is more general. it just happens not to work here.

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Feb 5, 2017
@jeffreymorlan
Copy link
Contributor

Instead of lib.d.ts's current overloads

interface String {
    replace(searchValue: string, replaceValue: string): string;
    replace(searchValue: string, replacer: (substring: string, ...args: any[]) => string): string;
    replace(searchValue: RegExp, replaceValue: string): string;
    replace(searchValue: RegExp, replacer: (substring: string, ...args: any[]) => string): string;
}

it could declare the method using union types:

interface String {
    replace(searchValue: string | RegExp, replaceValue: string | ((substring: string, ...args: any[]) => string)): string;
}

which would give more accurate error messages.

@abenhamdine
Copy link
Author

abenhamdine commented Feb 6, 2017

@mhegazy

The compiler does not know what was the "best" match given your arguments

Precisely, shouldn't Typescript try to find the best match signature instead of pick the last one ?

In this example, first argument match one signature, so Typescript could assume that the intended target is that signature, and give an error message related to that one.

@RyanCavanaugh
Copy link
Member

Defining "best" outside of trivial specific examples turns out to be very difficult

@abenhamdine
Copy link
Author

Yes I understand perfectly, but just reporting the error according to the last signature sounds weird no ? 😉
Why the last ? 😶 Seems simplistic compared to the very sophisticated tool that Typescript is.

@alexandertrefz
Copy link

alexandertrefz commented Feb 10, 2017

Maybe it would be better to create a more generic error when there is a signature mismatch, along the lines of 'No matching signature for funcname found.' ?

In my experience the signature mismatch errors are more confusing than helpful anyway, the way they currently are.

@abenhamdine
Copy link
Author

abenhamdine commented Feb 10, 2017

Maybe it would be better to create a more generic error when there is a signature mismatch, along the lines of 'No matching signature for funcname found.' ?
In my experience the signature mismatch errors are more confusing than helpful anyway, the way they currently are.

Yes. While not ideal, it would be better than actual message.

@abenhamdine
Copy link
Author

@RyanCavanaugh should I open another issue for a feature request to ask for a more generic error message ? Or could I just edit the title of this one ? What would you prefer?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 23, 2017

Yes I understand perfectly, but just reporting the error according to the last signature sounds weird no ? 😉
Why the last ? 😶 Seems simplistic compared to the very sophisticated tool that Typescript is.

The last signature is picked deliberately. Since overload resolution is order dependent in TS, the last signature is usually more general than previous ones, see addEventListener. In this case it just happens that signatures are disjoint, but this is not the general case.

I believe the error in this case is not very useful but it is useful in other cases, giving up on it completely is not the correct path.

For this specific case, the function definition should be updated to use union types, instead of multiple signatures. this should make the error experience much better.

@abenhamdine
Copy link
Author

abenhamdine commented Feb 23, 2017

Ok, I understand, thx a lot for all your patience & explanation !
I let this issue open to track for updating function definition.

@mhegazy mhegazy closed this as completed Apr 21, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

5 participants