Http4s: Order[ContentCoding] failure

[rossabaker]

2020-04-13T03:25:16.4857959Z [info] order laws must hold for Order[ContentCoding]
2020-04-13T03:25:16.4858291Z [error] x order.antisymmetry (41 ms)
2020-04-13T03:25:16.4858573Z [error]  Falsified after 0 passed tests.
2020-04-13T03:25:16.4858823Z [error]  > Labels of failing property:Expected: true
2020-04-13T03:25:16.4859069Z [error]  Received: false> ARG_0: ContentCoding(t, QValue(0.0))
2020-04-13T03:25:16.4859312Z [error]  > ARG_1: ContentCoding(t, QValue(0.0))
2020-04-13T03:25:16.4859557Z [error]  > ARG_2: org.scalacheck.GenArities$$Lambda$20955/1421781070@14088a8f
2020-04-13T03:25:16.4859794Z [error]  The seed is 2kpw5tJ8tADijqPv2GG3pUWPhjzpJUnaypQufFSWHBB=

[ashwinbhaskar]

can more context be added to this issue? I would like to pick it up but I am not sure what is the issue about.

[rossabaker]

Antisymmetry says that If ARG_0 <= ARG_1, and ARG_1 <= ARG_0, then ARG_0 == ARG1. This didn't happen. To make the failure repeatable, you can call setSeed("2kpw5tJ8tADijqPv2GG3pUWPhjzpJUnaypQufFSWHBB") on the property.

The ordering instance calls toLowerCase, but the equals method calls equalsIgnoreCase.

I think the right change in 0.21 would be to change that ordering instance to use compareIgnoreCase instead of equality on toLowerCase. The former does a round trip through toUpper and toLower on each character, and is also faster if there is a difference early in the string.

I think none of this matters in the real world: these charsets should always be ASCII, where these weird differences don't happen. That's probably a non-ASCII t character causing us problems here.

[ashwinbhaskar]

@rossabaker thank you for the explanation:) Where are the order law tests? git grep "order laws must hold for" does not find any occurrences.

[rossabaker]

They're defined in OrderLaws, which have some supertypes that reflect the hierarchy. You shouldn't need to change the test -- just the comparisons in the code.

[ashwinbhaskar]

@rossabaker

To make the failure repeatable, you can call setSeed("2kpw5tJ8tADijqPv2GG3pUWPhjzpJUnaypQufFSWHBB") on the property.

How do I do this?

[rossabaker]

Oh, good question. You can call prop { ... }.setSeed(...) on the regular property tests, but I had to look up how to do it in the discipline tests. This exposes the failure:

checkAll("Order[ContentCoding] for #3328", OrderTests[ContentCoding].order)(Parameters(seed = Seed.fromBase64("2kpw5tJ8tADijqPv2GG3pUWPhjzpJUnaypQufFSWHBB=").toOption))

[ashwinbhaskar]

@rossabaker From java documentation for toLowerCase() :

This method is locale sensitive, and may produce unexpected results if used for strings that are 
intended to be interpreted locale independently. Examples are programming language identifiers, 
protocol keys, and HTML tags. For instance, "TITLE".toLowerCase() in a Turkish locale returns 
"t\u0131tle", where '\u0131' is the LATIN SMALL LETTER DOTLESS I character. 

To obtain correct results for locale insensitive strings, use toLowerCase(Locale.ENGLISH).

I think toLowerCase(Locale.ENGLISH) should be the solution here. The tests pass with the seed value. WDYT?

[ashwinbhaskar]

@rossabaker To add to the above point, equalsIgnoreCase compares each character using Character.toLowerCase and Character.toUpperCase. From the java doc of Character.toLowerCase:

Note that Character.isLowerCase(Character.toLowerCase(ch)) does not always return true for some ranges of characters, particularly those that are symbols or ideographs.

In general, String.toLowerCase() should be used to map characters to lowercase. String case mapping methods have several benefits over Character case mapping methods. String case mapping methods can perform locale-sensitive mappings, context-sensitive mappings, and 1:M character mappings, whereas the Character case mapping methods cannot.

Note: This method cannot handle supplementary characters. To support all Unicode characters, including supplementary characters, use the toLowerCase(int) method.

[rossabaker]

See #3518 for what I believe to be the real reason.

That fix is a hack, because we used String instead of CIString in the model and need to preserve it. I think in 1.0, we want to use CIString, and correctness will follow.