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

parseFloat accepts only a string as an argument #17203

Closed
marcinkowal2015 opened this issue Jul 14, 2017 · 12 comments
Closed

parseFloat accepts only a string as an argument #17203

marcinkowal2015 opened this issue Jul 14, 2017 · 12 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@marcinkowal2015
Copy link

TypeScript Version: 2.4.1

Code

var baz = 42;
console.log(parseFloat(baz)); //42

Expected behavior:
No error. Console logs 42
Actual behavior:
error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

also:
Code

var foo = {
    toString: function () { return "500"; }
}
console.log(parseFloat(foo)); //500

Expected behavior:
No error. Console logs 500
Actual behavior:
error TS2345: Argument of type '{ toString: () => string; }' is not assignable to parameter of type 'string'

According to the ECMA specs there is not any requirement that value passed to the parseFloat function must be a string. I would say even more, the behavior of this function is well defined for object with have either valueOf or toString functions. In such case correct code in Javascript is not a correct one in Typescript. This error occured when in our project we were rewritting js code to typescript, and such apperance of invoking of parseFloat with number occured. There is an easy workaround for this, however still it is not correct

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jul 14, 2017

According to the ECMA specs there is not any requirement that value passed to the parseFloat function must be a string.

https://stackoverflow.com/questions/41750390/what-does-all-legal-javascript-is-legal-typescript-mean

I would say even more, the behavior of this function is well defined for object with have either valueOf or toString functions

Literally every object in JS that wasn't created from Object.create(null) satisfies this requirement. It seems rather wrong to call parseFloat({x: 3}) even though ({x: 3}).toString isn't undefined

such apperance of invoking of parseFloat with number occured

I'm surprised that calling parseFloat on a number isn't considered a problem? At best it's fishy, at worst it means the entirely wrong thing is happening.

@marcinkowal2015
Copy link
Author

Firstly, thanks for the link! It has clarified a lot.

I'm surprised that calling parseFloat on a number isn't considered a problem? At best it's fishy, at worse it means the entirely wrong thing is happening.

100% agreed, however well it 'happened' when we didn't had typescript. We were lucky because still the behavior is well defined for such case. In long run ofc we want to fix occurance of such invoking, because it is fishy. However still, according to the specs the parseFloat is possible to handle such cases, and in TypeScript you are not able to do that.

@jcready
Copy link

jcready commented Jul 14, 2017

@RyanCavanaugh the spec says to cast the input parameter to string: http://www.ecma-international.org/ecma-262/6.0/#sec-parsefloat-string

@RyanCavanaugh
Copy link
Member

Of course the spec defines the behavior. Math.max says cast the inputs to number; that doesn't mean Math.max(window, ["hello"]) is something TypeScript should allow.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 14, 2017
@marcinkowal2015
Copy link
Author

I would disagree a bit. In case with Math.max we have straight situation, any of values is NaN -> return NaN. However in case with parseFloat we don't have such a thing. If the spec something like said: 'If arg is not type of string return NaN' it would be perfectly fine to just assume that the argument can be a string only. Even if the behavior was defined for numbers, it is fine imho to just say, ok this function has no sense for numbers. However we have here defined special behavior if passed object has a valueOf or toString method. So by throwing away this behavior the function itself is very different. All I mean is that if the case was that for all object it is Nan, I am perfectly fine with behavior as it is, but in current form this is more like some utility function 'strictParseFloat' which take a use of some cases of defined parseFloat.

@RyanCavanaugh
Copy link
Member

However we have here defined special behavior if passed object has a valueOf or toString method.

Again, all objects have these methods. The coercion via valueOf / toString is how every coercion in the spec works.

Is the line somewhat hazy? Yes. Is the correct rule "allow every coercion" ? Clearly no.

@jcready
Copy link

jcready commented Jul 14, 2017

It seems like TS should then require that the function passed to Array.filter return a boolean to be consistent.

@RyanCavanaugh
Copy link
Member

@jcready used to work like that #5850

@RyanCavanaugh
Copy link
Member

Really just a duplicate of #4002. Everything old is new again

@jcready
Copy link

jcready commented Jul 14, 2017

So then why is this "Working as Intended" while that was changed? It seems like a nearly identical use-case, no?

@jcready
Copy link

jcready commented Jul 14, 2017

I would argue that for built-ins like parseFloat, isNaN, etc. that the input parameters should be any. The main reason for type checking input parameters for a function is to prevent runtime errors, but in these cases we know that no matter what input you pass to these functions you will not get a runtime error.

@aluanhaddad
Copy link
Contributor

@jcready I think a primary reason for type checking is to prevent bugs. Not all bugs manifest as runtime errors.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants