Update interface of revertibleRandom

[darkdrag00nv2]

Work towards #2712

Description

1. Introduced a new type FixedSizeUnsignedInteger
2. Update the interface of the revertibleRandom function to fun revertibleRandom<T: FixedSizeUnsignedInteger>([modulo: T]): T

Note that updating the interface is a breaking change as the existing usage of the function such as below fails.

let rand1 = revertibleRandom()  // error: cannot infer type parameter: `T`

Noteworthy Points

1. Updating the interface is a breaking change as pointed out in the description and also the issue Update revertibleRandom interface (FLIP 120) #2712 (comment)
2. Since we don't support type inference of function arguments, we will run into the same issues as the InclusiveRange constructor function: Fix incorrect type inference for InclusiveRange #2886

Basically:

let r = revertibleRandom<Int8>(0) // checker error, type mismatch - expected: Int8, Found - Int
let r = revertibleRandom<Int8>(Int8(0)) // success

3. The modulo operation hasn't been implemented in this PR. It'll be done in a future PR based on the discussion on the issue.

---

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Code follows the standards mentioned here
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[SupunS]

Changes look good so far!

Given we are introducing a new type, we should probably add some more high-level Cadence tests (you've already added low-level Go tests, which is great!), for usages of FixedSizeUnsignedIntegerType in various places, such as:

• As a type annotation for variable
• Constructing a value (valid/invalid)
• main function arguments/return type (for import/export)
• Storing to the storage and reading from it.
• Casting, sub-type assignment, etc.

[SupunS]

Given this is a breaking change, can you please rebase this to the feature/stable-cadence branch?

[darkdrag00nv2]

@SupunS The revertibleRandom function was added in the master branch as part of #2896. It is absent from the stable-cadence branch.

[turbolent]

Great work!

Looks good, only thing left is the removal of the unnecessary endianness conversion

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4c9fb2a) 80.11% compared to head (1def84b) 80.14%.

Files	Patch %	Lines
runtime/stdlib/random.go	95.90%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           feature/stable-cadence    #2943      +/-   ##
==========================================================
+ Coverage                   80.11%   80.14%   +0.02%     
==========================================================
  Files                         348      348              
  Lines                       81870    81999     +129     
==========================================================
+ Hits                        65590    65716     +126     
- Misses                      13958    13961       +3     
  Partials                     2322     2322              

Flag	Coverage Δ
unittests	80.14% <96.55%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tarakby]

@turbolent, @SupunS, @darkdrag00nv2 do you think we should add the optional parameter modulo as part of this PR?
The optional parameter could be added but the implementation of revertibleRandom would ignore it for now. This would allow me to open a follow-up PR and use the parameter to generate an output less than modulo.

Another option is to leave this PR as it is, open a new PR where the optional parameter is added, and I'll continue to commit on that new PR.
Feel free to choose either way, but it would be good to continue the work on this topic now that it's started.

[turbolent]

@tarakby either way works. The PR has the modulo parameter in the definition, but the implementation still ignores it: https://github.com/onflow/cadence/pull/2943/files#diff-a1bff80973b499df2d90ec34d5ef987dae71f400bc6a8fab5c6fc5856661fcf7R94. We can merge it as-is and address it in a follow-up PR

[turbolent]

Nice, great work!

[turbolent]

There's a data race in the tests: https://github.com/onflow/cadence/actions/runs/7121193421/job/19389931602?pr=2943, looking into it

[tarakby]

@turbolent, Nice! I didn't notice the parameter is already there. I'll create an issue for myself to work on the remaining part.

[darkdrag00nv2]

I believe this should labelled with Language Breaking Change as the interface is not backwards compatible.

[SupunS]

Not sure why this got closed. This should now be opened against the master (#2971)

[dsainati1]

Same, it seems as though it was closed automatically when feature/stable-cadence was merged into master. I can't re-open it for some reason though.