[Cosmos] fix errors after using noFallthroughCasesInSwitch default configuration

[bzhang0]

This fixes #11435. Let me know if there's anything I missed! I followed the instructions in the description and made sure that rushx build worked, but I'm not quite sure if this implementation is correct.

A couple comments + what I did:

• I removed the "noFallthroughCasesInSwitch: false" rule from tsconfig.json
• I updated any resulting errors after running rushx build
• I'm not quite sure why package.json was updated (I ran rush update and rush rebuild while on this branch before pushing, maybe this is the case), so let me know if this is an issue.

Hopefully this works! Thank you for your time.

[ghost]

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

[deyaaeldeen]

I think the change makes sense, thanks!

Could you please revert the changes in package.json?

[witemple-msft]

Would this not change the result? If we previously were falling through in all of these hash calculations, then the function was designed with that in mind. We can't just insert breaks and expect the function to work the same way.

[witemple-msft]

Is it possible to disable this check on a file-by-file basis? Maybe we can just exclude murmurHash from the no-fallthrough test.

[witemple-msft]

Seems to be a requested feature in the compiler, but not implemented: microsoft/TypeScript#28306

[jeremymeng]

Would this not change the result? If we previously were falling through in all of these hash calculations, then the function was designed with that in mind. We can't just insert breaks and expect the function to work the same way.

For these changed switches, it would only fall through to the default case, which doesn't exist, right?

[deyaaeldeen]

@witemple-msft from the first glance, this change does not seem to change the result because the switched on variable does not get re-assigned in any of the case blocks and as @jeremymeng pointed out, there is no default block to fall into.

Also for context, this module is copied from an old commit from https://github.com/pid/murmurHash3js where the code is originally written in JS.

[witemple-msft]

@deyaaeldeen, @jeremymeng Unless I'm very much misunderstanding something, cases fall through to the next case, not to the default case. Adding these breaks changes that. For example, on this line:

https://github.com/Azure/azure-sdk-for-js/pull/20212/files#diff-04ced214ef2fc99cc81a5e736787de978f0c17eded96683838acbc23815c4e71R498

The switch statement actually executes all the cases 9 through 1, not only case 9. Adding the breaks means that the steps from cases 8-1 just aren't executed anymore. Am I missing something?

This TSPG demonstrates a simpler example: https://www.typescriptlang.org/play?#code/FAYw9gdgzgLgBADwFxwgVwLYCMCmAnOAXjgCYAGAbmFEijABscA6esAcwAoAiAZQHcAljBAALARDZIuAGkQBKKsCiDhIuBwRy4Ab2Bw4IAIZQccAIxkke-QdoNmrTlwtcF1oydKXr+8NHss7NzkrlS+xqYAzN42tv6MgU7RoT5wWHg4hgDWYQYRcAAsMTZ+dAmO3EUp+gAmOABmhmj0MFaxpQEVXAAiDU0tKQC+QA

[jeremymeng]

@witemple-msft you are right! TIL. I was probably thinking that they were like multiple if statements but they are not. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch

[deyaaeldeen]

I am questioning my understanding of imperative programming and I blame FP!

@bzhang0 Perhaps you could rewrite

switch (remainder) {
    case 3:
      k1 ^= bytes[j + 2] << 16;

    case 2:
      k1 ^= bytes[j + 1] << 8;

    case 1:
      k1 ^= bytes[j];
      k1 = _x86Multiply(k1, c1);
      k1 = _x86Rotl(k1, 15);
      k1 = _x86Multiply(k1, c2);
      h1 ^= k1;
  }

to

if (remainder === 3) {
      k1 ^= bytes[j + 2] << 16;
}
if (remainder === 2 || remainder === 3) {
      k1 ^= bytes[j + 1] << 8;
}
if (remainder >= 1 || remainder <= 3) {
      k1 ^= bytes[j];
      k1 = _x86Multiply(k1, c1);
      k1 = _x86Rotl(k1, 15);
      k1 = _x86Multiply(k1, c2);
      h1 ^= k1;
  }

I think it is common to use fallthrough switch statements in writing hash algorithms for performance reasons and I wonder if my (uglier) suggestion would degrade the performance much.

Anyway, I am fine leaving the code as is since it was copied from somewhere else and wait until TS provides support for controlling this behavior per module.

[bzhang0]

I've reverted package.json and updated the switch case in murmurHash.ts as well! Thanks for the suggestion @deyaaeldeen! Let me know if there's anything else I can do.

[jeremymeng]

@bzhang0 @deyaaeldeen I'd prefer keeping the original code if this is adopted from somewhere else.

[witemple-msft]

I agree with Jeremy. Since this is a module we vendor in from elsewhere I think the risk of messing with it outweighs the benefit of being able to say we have this flag on everywhere. Apologies to @bzhang0 for the confusion here.

@ramya-rao-a can we make an exception to the rule for cosmos in this case?

[ramya-rao-a]

can we make an exception to the rule for cosmos in this case?

Fine by me. Can we summarize the reason in #11435 and close it?

[witemple-msft]

I left a note there and closed the issue, so I'll close this as well. Again, sorry to @bzhang0 for the confusion and thank you for helping us at least dig into this!

[ramya-rao-a]

Thanks for your work here @bzhang0, we definitely got to learn some learning here :)