Skip to content

Sorting record members first on the length of the key #1211

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rajdakin
Copy link
Contributor

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.

Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

I am not sure we want this. I'd think it should be sufficient to localise this change to type printing only.

@rajdakin
Copy link
Contributor Author

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).

@dhil
Copy link
Member

dhil commented Mar 25, 2025

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

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

Successfully merging this pull request may close these issues.

Incorrect arguments printing in Roundtrip printer with at least 10 arguments
2 participants