don't coerce out of bounds numbers (#219)

* don't coerce out of bounds numbers

* changeset

* more number handling tweaks

- fix handling of negative numbers
- infer string if the number would change at all

---------

Co-authored-by: Theo Ephraim <[email protected]>

Author: philmillman

.changeset/purple-teams-hear.md

@@ -0,0 +1,5 @@
+---
+"@env-spec/parser": patch
+---
+
+Fixed a bug where values that resembled very large numbers were being improperly coerced

packages/env-spec-parser/src/helpers.ts

@@ -1,11 +1,30 @@
-const VALID_NUMBER_REGEX = /^(0|([1-9][0-9]*))?(\.[0-9]+)?$/;
-export function autoCoerce(valStr) {
+// this regex helps avoid any weird numbers
+// - leading zeros
+// - scientific notation
+// - multiple decimal points
+const VALID_NUMBER_REGEX = /^-?(0|([1-9][0-9]*))?(\.[0-9]+)?$/;
+
+export function autoCoerce(valStr: string): string | number | boolean | undefined {
   if (valStr === 'true') return true;
   if (valStr === 'false') return false;
   if (valStr === 'undefined') return undefined;
   // not handling `null` because its a JS specific thing... we'll try to avoid it altogether
 
-  // special check to avoid weird number coercion like `01`, `1e10`
-  if (VALID_NUMBER_REGEX.test(valStr)) return Number(valStr);
+  // the regex check filters out a lot of ambiguous/weird cases
+  if (VALID_NUMBER_REGEX.test(valStr)) {
+    const num = Number(valStr);
+
+    // Check if we can convert back to a string without any changes
+    // this could be loss in precision or just formatting (extra zeros)
+    const backToString = String(num);
+    if (backToString !== valStr) return valStr;
+
+    // if numbers are too large, we'll leave them as strings
+    if (Number.isInteger(num) && !Number.isSafeInteger(num)) return valStr;
+    if (num > Number.MAX_SAFE_INTEGER || num < Number.MIN_SAFE_INTEGER) return valStr;
+
+    // otherwise we can return the number
+    return num;
+  }
   return valStr;
 }

packages/env-spec-parser/test/values.test.ts

@@ -91,14 +91,29 @@ describe('boolean handling', basicValueTests([
 describe('number handling', basicValueTests([
   ['0', 0],
   ['123', 123],
+  ['-123', -123],
   ['123.456', 123.456],
-  ['.0', 0],
-  ['.01230', 0.0123],
+  ['-123.456', -123.456],
+  ['0.0123', 0.0123],
+  // if the number is not converted cleanly back to a string, we leave as a string
+  ['.0', '.0'],
+  ['.01230', '.01230'],
+  ['1.230', '1.230'],
+  // number-ish but not quite numbers
   ['123.456.789', '123.456.789'],
   ['10e3', '10e3'],
   ['001', '001'],
+  ['01', '01'],
   ['123.', '123.'],
   ['123..', '123..'],
+  ['Infinity', 'Infinity'],
+  // numbers that would lose precision are treated as strings
+  ['92183090832018209318123781721.12231', '92183090832018209318123781721.12231'],
+  ['1.23123412341234123414352345234523452345234523452345234523452345234523452345', '1.23123412341234123414352345234523452345234523452345234523452345234523452345'],
+  // max safe integer is still a number
+  [(Number.MAX_SAFE_INTEGER).toString(), Number.MAX_SAFE_INTEGER],
+  // but anything larger is treated as a string
+  [(Number.MAX_SAFE_INTEGER + 1).toString(), (Number.MAX_SAFE_INTEGER + 1).toString()],
 ]));
 
 describe('function calls', basicValueTests([

packages/varlock-website/src/content/docs/env-spec/reference.mdx

@@ -151,6 +151,7 @@ Values are interpreted similarly for config item values, decorator values, and v
 #### Unquoted values
 - Will coerce `true`, `false`, `undefined` -- `@foo=false`
 - Will coerce numeric values -- `@int=123 @float=123.456`
+  - if the number is too large, would lose precision, or would change formatting, it will remain a string
 - May be interpreted as a function call (see below)
 - Otherwise will be treated as a string
 - May not contain other characters depending on the context:

packages/varlock-website/src/content/docs/reference/data-types.mdx

@@ -32,17 +32,28 @@ The internal coercion/validation process looks like:<br/>
 
 
 ### Default behavior
-When no `@type` is specified, a type will be inferred where possible - for static values, and some functions that return a known type. Otherwise the type will default to `string`.
+When no `@type` is specified, a type will be inferred where possible - for static values, and some functions that return a known type. Note that the use of quotes matters. Otherwise the type will default to `string`.
 
 ```env-spec
-INFERRED_STRING="foo"
-INFERRED_NUMBER=123
+INFERRED_STRING_QUOTED="foo"
+INFERRED_STRING_UNQUOTED=foo
+INFERRED_NUMBER=123     # infers number type
+QUOTED_NUM_STRING="123" # remains a string unless @type=number is used
 INFERRED_BOOLEAN=true
+
+# return type of some functions can be inferred
 CONCAT_INFERS_STRING=`concat-${SOMEVAR}-will-be-string`
+FN_INFER_BOOLEAN=eq($VAR1, $VAR2)
 DEFAULTS_TO_STRING_FN=fnThatCannotInferType()
+
+# with no other info, we default to string
 DEFAULTS_TO_STRING=
 ```
 
+Note that numeric values that would lose precision, or change any formatting (like leading/trailing zeros), will be treated as strings unless explicitly adding `@type=number`.
+
+In any slightly ambiguous situation, it is better to explicitly add a `@type` decorator.
+
 
 ## Built-in data types