Avoid using Opt.auto to avoid overflows going silent

[smelc]

Changelog

- description: |
    Forbid incorrect values in parsers of many Int-like options. Previously those values would overflow and be turned into a random valid value. If this breaks your use case, this means your use case wasn't doing what you expected.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

We use auto a lot in parsing. auto relies under the hood on Read instances which are known to have problems. Some instances accept wrong values and return out of thin air values. This PR fixes this.

Fixes #860

How to trust this PR

• Try this PR with cardano-testnet: tested cabal test cardano-testnet-test with cardano-node at e9ab511272a7694196778007211991a9984557e8 and cardano-cli at 44ca6ed
• Run git grep auto "*.hs" and observe that all the remaining calls are in a byron file (I didn't change those)

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated. See Running tests for more details
• Self-reviewed the diff

[carbolymer]

I like the direction, but wordReader needs some polishing. 😃

[smelc]

@carbolymer> I did the changes 👍

Given your last comment, I assume you're fine with me deploying this to many more code locations? (please don't approve though yet, because I'll extend this PR).

[Jimbo4350]

LGTM! Now just replace all calls to auto.

[smelc]

This one is important to review

[carbolymer]

looks fine to me 👌🏻

[Jimbo4350]

We can improve these tests with hedgehog generators. Example:

diff --git a/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs b/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
index 1bf19f8b8..c72c8a093 100644
--- a/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
+++ b/cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
@@ -1,3 +1,5 @@
+{-# LANGUAGE RankNTypes #-}
+{-# LANGUAGE ScopedTypeVariables #-}
 {-# LANGUAGE TypeApplications #-}
 
 module Test.Cli.Parser
@@ -12,11 +14,14 @@ import           Cardano.CLI.EraBased.Options.Common (integralParsecParser,
 import           Data.Bits (Bits)
 import           Data.Data (Typeable)
 import           Data.Either (isLeft, isRight)
+import           Data.Proxy
 import           Data.Word (Word16)
 import qualified Text.Parsec as Parsec
 
-import           Hedgehog (Property, assert)
+import           Hedgehog
 import           Hedgehog.Extras (propertyOnce)
+import qualified Hedgehog.Gen as Gen
+import qualified Hedgehog.Range as Range
 
 -- | Execute me with:
 -- @cabal test cardano-cli-test --test-options '-p "/integral reader/"'@
@@ -45,9 +50,8 @@ hprop_integral_reader = propertyOnce $ do
 -- @cabal test cardano-cli-test --test-options '-p "/pair integral reader/"'@
 hprop_pair_integral_reader :: Property
 hprop_pair_integral_reader = propertyOnce $ do
-  assert $ isRight $ parse @Word "(0, 0)"
-  assert $ isRight $ parse @Word "   (   0  ,   0   )"
-  assert $ isRight $ parse @Word " (18446744073709551615   ,    18446744073709551614   )   "
+  validArbitraryTuple <- forAll $ genNumberTuple (Proxy :: Proxy Word)
+  assert $ isRight $ parse @Word validArbitraryTuple
 
   assert $ isLeft $ parse @Word "(0, 0, 0)"
   assert $ isLeft $ parse @Word "(-1, 0)"
@@ -64,3 +68,18 @@ hprop_pair_integral_reader = propertyOnce $ do
     case Parsec.runParser pairIntegralParsecParser () "" s of
       Left parsecError -> Left $ show parsecError
       Right x -> Right x
+
+genNumberTuple
+  :: forall integral
+   . Integral integral
+  => Show integral
+  => Proxy integral -> Gen String
+genNumberTuple _ = do
+  x :: integral <- Gen.integral (Range.linear 0 100)
+  y :: integral <- Gen.integral (Range.linear 0 100)
+  space1 <- genArbitrarySpace
+  space2 <- genArbitrarySpace
+  return $ "(" ++ space2 ++ show x ++ space1 ++ "," ++ space2 ++ show y ++ space1 ++ ")"
+
+genArbitrarySpace :: Gen String
+genArbitrarySpace = Gen.string (Range.linear 0 5) (return ' ')

[Jimbo4350]

We should as much as possible avoid unit tests.

[smelc]

I took your suggestion and split the function into two, since the positive tests need to be run with property, while the negative tests need to run with propertyOnce 👍

[Jimbo4350]

while the negative tests need to run with propertyOnce

Why? You could also write a generator to inject a random character (or multiple characters) between the brackets for the negative tests.

[smelc]

Yes but I would need to rule out generators that would still yield a valid value (e.g. if the generated character is a number) and I believe writing a good generator for negative tests it overkill for testing this parser. So I'll stick to the manual negative tests to speed up merging.

[Jimbo4350]

LGTM however you can improve your tests further using hedgehog generators.

[Jimbo4350]

You should use https://hackage.haskell.org/package/hedgehog-1.5/docs/Hedgehog-Internal-Gen.html#v:word instead.

[smelc]

👍 Done with:

  w <- forAll $ Gen.word $ Gen.linear minBound maxBound
  assert $ isRight $ parse @Word (show w)

[Jimbo4350]

You can write a generator that injects a random character (or characters) into the string generated by genNumberTuple. This would provide better coverage.

[Jimbo4350]

These are already tested in hprop_integral_pair_reader_negative

[smelc]

Ooops, good catch. Correcting 👍

[carbolymer]

it would be better to check the exact value in case of Rights here

Suggested change

	assert $ isRight $ parse @Word "42"
	parse @Word "42" === Right 42

[smelc]

Good point, changing 👍

[carbolymer]

It would be better to write this as

Suggested change

	assert $ isLeft $ parse @Word "18446744073709551616"
	assertWith isLeft $ parse @Word "18446744073709551616"

assert does not report the value on assertion failure

[smelc]

Changed to (for example) assertWith (parse @Word "18446744073709551616") isLeft

[carbolymer]

LGTM