{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/2285","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/2285/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/2285/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/2285/events","html_url":"https://github.com/dotnet/coreclr/issues/2285","id":121357604,"number":2285,"title":"Problems with thousands separators when parsing strings to numbers","user":{"login":"varocarbas","id":4943723,"avatar_url":"https://avatars.githubusercontent.com/u/4943723?v=3","gravatar_id":"","url":"https://api.github.com/users/varocarbas","html_url":"https://github.com/varocarbas","followers_url":"https://api.github.com/users/varocarbas/followers","following_url":"https://api.github.com/users/varocarbas/following{/other_user}","gists_url":"https://api.github.com/users/varocarbas/gists{/gist_id}","starred_url":"https://api.github.com/users/varocarbas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/varocarbas/subscriptions","organizations_url":"https://api.github.com/users/varocarbas/orgs","repos_url":"https://api.github.com/users/varocarbas/repos","events_url":"https://api.github.com/users/varocarbas/events{/privacy}","received_events_url":"https://api.github.com/users/varocarbas/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-12-09T22:39:46Z","updated_at":"2015-12-10T13:20:14Z","closed_at":null,"body":"Since some months ago, I have been aware about the problems that the parsing methods (at least, ```Parse``` and ```TryParse```) of various numerical types (at least, ```Decimal``` and ```Double```) have when dealing with thousands separators (at least in .NET 4.5; in both C# and VB.NET). The following C# sample code illustrates the point well:\r\n```C#\r\nstring stringVal = \"1,1.1\";\r\ndecimal decVal = 0m;\r\nif (decimal.TryParse(stringVal, NumberStyles.Any, CultureInfo.InvariantCulture, out decVal))\r\n{\r\n    //This part will be reached, because decimal.TryParse thinks that 1,1.1 is a valid number\r\n}\r\n```\r\nSuch misbehaviour can be replicated under many different conditions. The basic idea is that the parsing approaches don't understand what the thousands separators really imply (i.e., groups of 3 numbers in the aforementioned example).\r\n\r\nToday, I focused my analysis of the code on the ```Decimal``` type and found ```ParseNumber``` (mscorlib\\src\\System\\Number.cs). The way in which this method treats the thousands separators (in all their forms) explains perfectly the observed behaviour: it focus the analysis on valid/invalid characters and doesn't bring into account the size of the group at any point.\r\n\r\nMy proposal consists in forcing the corresponding methods to trigger an error when the thousands separators aren't in the right positions. My intention is firstly focusing on the ```Decimal``` type (although the aforementioned method is most likely used by other types too). After having ready a good enough correction for ```Decimal```, I will analyse the remaining types in detail and perform additional changes if required.\r\n\r\nThe correction will consist in analysing the input string; in case of including thousands separators, their locations would be checked. If this analysis fails, no conversion to number would be performed. In principle, I am planning to take advantage from the current implementation (more specifically, a loop analysing all the characters) and rely on “low level” approaches; although I have still to think carefully about the exact algorithm.\r\n"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/163612249","html_url":"https://github.com/dotnet/coreclr/issues/2285#issuecomment-163612249","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/2285","id":163612249,"user":{"login":"varocarbas","id":4943723,"avatar_url":"https://avatars.githubusercontent.com/u/4943723?v=3","gravatar_id":"","url":"https://api.github.com/users/varocarbas","html_url":"https://github.com/varocarbas","followers_url":"https://api.github.com/users/varocarbas/followers","following_url":"https://api.github.com/users/varocarbas/following{/other_user}","gists_url":"https://api.github.com/users/varocarbas/gists{/gist_id}","starred_url":"https://api.github.com/users/varocarbas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/varocarbas/subscriptions","organizations_url":"https://api.github.com/users/varocarbas/orgs","repos_url":"https://api.github.com/users/varocarbas/repos","events_url":"https://api.github.com/users/varocarbas/events{/privacy}","received_events_url":"https://api.github.com/users/varocarbas/received_events","type":"User","site_admin":false},"created_at":"2015-12-10T13:20:14Z","updated_at":"2015-12-10T13:20:14Z","body":"@mikedn When I use ```Decimal.TryParse``` (or an equivalent method of other numerical type), I expect to get true when the given string represents a valid number (on account of the given type and culture) and false otherwise. \"1,1.2\" or \"1,,,,,5\" (yes, it also accepts this last one) or similar are not valid numbers, but the current implementation might recognise them as such (i.e., bug). What is the difference between these examples and \"111*12\"? This is not a valid number either, but is it less valid than the aforementioned one? Do you prefer to always extract all the relevant information (i.e., numbers) and only output false when strictly required (no numbers at all)? Then why is “111*12” not accepted too?\r\n\r\nThe most ironic part is that I usually ignore thousands separators (not too typical in Spain, my country; neither in the kind of fields where I am more experienced). I knew about this \"issue\" some months ago while answering a SO question of someone being very interested in triggerin an error in these cases. After this question (which BTW was solved by relying on manually pre-checking the format), I have seen at least 2 additional ones on the same lines. Thus, I am quite sure that this behaviour is not what some people expect and that they plainly don't trust the in-built parsing methods.\r\n\r\nRegarding what you say of \"doesn't appear to be anything in the documentation...\", note that the method which deals with these actions (the one I am mentioning in my first post) precisely includes a comment explaining the supported situations, where properly accounting for the locations of thousands separators is not being mentioned (as said, the method doesn't even deal with the required information). In any case, I don't see any relevance on this issue; other than for defining the difference between bug in the code (i.e., a mistake in the actual implementation) and bug in the underlying logic (i.e., pursued goal not right enough).\r\n\r\nLastly, I want to highlight that the required correction would have a really low impact. I have already analysed this specific bit of code and can include this functionality by just accounting for a couple of additional variables in a loop. On the other hand, one of the outputs of carefully analysing this method might even be a slight improvement of its performance.\r\n"}}