`(==)` does not work correctly on records with field named `ctor`

[jvoigtlaender]

The current version of core (the current master branch) evaluates {ctor = [0]} == {ctor = [0]} to False.

@evancz, probably caused by the recent refactoring in elm-lang@6f5fbb8?

[process-bot]

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[process-bot]

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[jvoigtlaender]

Update: This now also happens with the released version 4.0.2 of core.

That is, the following program wrongly outputs False:

import Html

main = Html.text (toString ({ctor = [0]} == {ctor = [0]}))

while the following program continues to correctly output True:

import Html

main = Html.text (toString ({field = [0]} == {field = [0]}))

@evancz, just to let you know that the bug-fix release of core also introduced a bug. This one, which wasn't present in version 4.0.1.

[jvoigtlaender]

Update: This now also happens with the released version 4.0.2 of core.

That is, the following program wrongly outputs False:

import Html

main = Html.text (toString ({ctor = [0]} == {ctor = [0]}))

while the following program continues to correctly output True:

import Html

main = Html.text (toString ({field = [0]} == {field = [0]}))

@evancz, just to let you know that the bug-fix release of core also introduced a bug. This one, which wasn't present in version 4.0.1.

[evancz]

The bigger (timely) fix will be to reintroduce the hidden _ field to Elm records. This will serve as a runtime tag of whether something is an Elm record a union type or a JS value. That will fix some issues where toString will show JS values instead of <internal value> or something.

I feel like you found this by reading the new code, not by having a field with that name, so I'm not certain how urgent this is. That said, I suspect there is a stopgap fix that does not slow things down too much. Perhaps even just doing the non-pseudo-code of switching to eqHelp(ctor, ctor) to compare the values. A bit slower, but probably fine compared to the previous version.

[evancz]

The bigger (timely) fix will be to reintroduce the hidden _ field to Elm records. This will serve as a runtime tag of whether something is an Elm record a union type or a JS value. That will fix some issues where toString will show JS values instead of <internal value> or something.

I feel like you found this by reading the new code, not by having a field with that name, so I'm not certain how urgent this is. That said, I suspect there is a stopgap fix that does not slow things down too much. Perhaps even just doing the non-pseudo-code of switching to eqHelp(ctor, ctor) to compare the values. A bit slower, but probably fine compared to the previous version.

[jvoigtlaender]

No, I didn't find this by reading the code. I found it by trying to run the test suite. Note that it contains this (at https://github.com/elm-lang/core/blob/master/tests/Test/Equality.elm#L22):

        , test "ctor same" <| assert ({ctor = Just 3} == {ctor = Just 3})

From there, I was trying whether it has something to do especially with Just of Maybe, which was not the case...

Only after that, I checked the source code to see what other special handling goes on with the ctor field name, and encountered this other failure, with toString: https://github.com/elm-lang/core/issues/654.

As an aside, it's a pity that the test suite is currently failing on GitHub.

[jvoigtlaender]

No, I didn't find this by reading the code. I found it by trying to run the test suite. Note that it contains this (at https://github.com/elm-lang/core/blob/master/tests/Test/Equality.elm#L22):

        , test "ctor same" <| assert ({ctor = Just 3} == {ctor = Just 3})

From there, I was trying whether it has something to do especially with Just of Maybe, which was not the case...

Only after that, I checked the source code to see what other special handling goes on with the ctor field name, and encountered this other failure, with toString: https://github.com/elm-lang/core/issues/654.

As an aside, it's a pity that the test suite is currently failing on GitHub.

[evancz]

I'm working on the tests. You can try sleuthing too if you'd like.

[evancz]

I'm working on the tests. You can try sleuthing too if you'd like.

[jvoigtlaender]

I see you got the test suite passing already. A nice green batch on the repo page. Cool!

Maybe it's worth merging https://github.com/elm-lang/core/pull/655. It should pass now, given your earlier stopgap fix. And it might serve as a prophylaxis against any special casing surrounding ctor and equality testing creeping back in as an error at some point in the future.

Or if you think that's unlikely to become a problem again, that pull request could be simply closed.

[jvoigtlaender]

I see you got the test suite passing already. A nice green batch on the repo page. Cool!

Maybe it's worth merging https://github.com/elm-lang/core/pull/655. It should pass now, given your earlier stopgap fix. And it might serve as a prophylaxis against any special casing surrounding ctor and equality testing creeping back in as an error at some point in the future.

Or if you think that's unlikely to become a problem again, that pull request could be simply closed.

[evancz]

Given that people in JS world get pretty intense about what color that badge is, I don't think it makes sense to check in broken tests unfortunately. I'd expect a relatively long turn-around on the _ stopgap because it needs the compiler to change, so I would not want things to be "red" for that amount of time.

[evancz]

Given that people in JS world get pretty intense about what color that badge is, I don't think it makes sense to check in broken tests unfortunately. I'd expect a relatively long turn-around on the _ stopgap because it needs the compiler to change, so I would not want things to be "red" for that amount of time.

[jvoigtlaender]

If I understand correctly what you did two hours ago, the tests from https://github.com/elm-lang/core/pull/655 will not prevent the badge from being green. They were broken tests, but no longer.

[jvoigtlaender]

If I understand correctly what you did two hours ago, the tests from https://github.com/elm-lang/core/pull/655 will not prevent the badge from being green. They were broken tests, but no longer.

[evancz]

Ah, gotcha. I thought you meant about ctor problems with toString. I started the tests again. Will merge if green. Thanks!

[evancz]

Ah, gotcha. I thought you meant about ctor problems with toString. I started the tests again. Will merge if green. Thanks!