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

Type issue in RLEEncoder #448

Open
Brian-McBride opened this issue Nov 23, 2021 · 3 comments
Open

Type issue in RLEEncoder #448

Brian-McBride opened this issue Nov 23, 2021 · 3 comments

Comments

@Brian-McBride
Copy link

Looking through the code, there is this line (repeated a few times)

https://github.com/automerge/automerge/blob/08a456cc2ddc79d8c2f35953db0f1fd8ed418d0f/backend/encoding.js#L695

  • We have sum which is a number (defined above as let sum = 0.
  • sumShift can be undefined. This can be solved earlier in the code with const { count, sumValues, sumShift = 0 } = options;
  • firstValue is returned from decoder.readValue() that has way too many types: string, number, null, undefined, void

A little cleanup in the decoder reduces the return types to string | number | null filtering down to the line in question where firstValue can be of a number or string type.

anyString >>> anyNumber has a lot of side effects. It really just depends if the value can be cast to a number. If it can be cast, it will work as expected. If it can't it will return 0. Returning zero isn't probably correct as sum will not be incremented as a number would. Worse though, if sum += sumShift is falsey, then we end up with sum adding a string to 0. Now we aren't a number but 0some-text-string.

Am I wrong here? Or does the RLEEncoder basically incorrectly generate the sum of strings when using sumValues?

@Brian-McBride
Copy link
Author

Brian-McBride commented Nov 23, 2021

I don't know what sum is used for downstream (yet). But my bet would be that the length of the string is what should be used? That or all strings should be zero (never allowing for automatic casting risking NaN)

Maybe something like a method:

  calcSum(value) {
    if (typeof value === 'number') return value;
    if (value === null) return 0;
    return value.length;
  }

Used in the three places where this same calculation is being done

if (sumValues) sum += sumShift ? this.calcSum(firstValue) >>> sumShift : this.calcSum(firstValue);

Or whatever the value of a string should be in this case.

@Brian-McBride
Copy link
Author

By the way, the reason there is a string at all comes from readRawValue()

https://github.com/automerge/automerge/blob/08a456cc2ddc79d8c2f35953db0f1fd8ed418d0f/backend/encoding.js#L889-L903
Where readPrefixedString() returns a string type.

As a sidenote, readRecord and readRawValues both say "do not call from outside the class", but it is called outside the class (inside the RLEEncoder class).

@ept
Copy link
Member

ept commented Nov 25, 2021

Hi @Brian-McBride, I believe sumValues should only ever be true on encoders/decoders with a number type (int or uint). It shouldn't ever be adding strings. We could make it throw an exception if you try to pass sumValues && this.type !== 'int' && this.type !== 'uint'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants