links icon indicating copy to clipboard operation
links copied to clipboard

Sorting record members first on the length of the key

Open rajdakin opened this issue 8 months ago • 2 comments

This PR fixes #1210 and the order of the fn_params member of the function descriptor.

The implementation of this fix adds a string map which has a different compare function: it first sorts keys on length, then on the content. This way, the string 10 is sorted after the string 2. This map is then used in Row types.

rajdakin avatar Mar 24 '25 15:03 rajdakin

The root issue is that StringMap.fold iterates the map in the wrong order ("10" before "2"). I see three ways of fixing this issue, one of which is the current one:

  • Change the StringMap so that comparison yields the correct ordering -- this is the one implemented in this PR;
  • Change every StringMap.fold usage which gives a list (there are at least two such places, and every subsequent edits must keep this caveat in mind, which I don't believe is the correct solution) -- this is the solution you are proposing;
  • Change the type of functions to have a list of typ instead of a typ usually containing a Record containing a Row as the argument.

I have also tried to look into implementing the third solution, but I believe there exists/existed some use case for types other than Record+Row as argument types, so I quickly abandoned the idea. I can still try to make it work if you believe this would be a better solution.

Note that this PR not only fixes #1210, but also the issue that the fn_params member list is not ordered properly in the backend IR (this is not an issue for now as only the types in that list are incorrectly ordered, an information currently unused in all backends, but I will need this fixed for the WasmFX backend).

rajdakin avatar Mar 25 '25 10:03 rajdakin

Thanks for the clarification. I see. I think we need to discuss which kind of design to adopt here. Because the change is pervasive.

dhil avatar Mar 25 '25 10:03 dhil