Skip to content

Improve generated code for let assert on the Erlang target #4502

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

Merged
merged 9 commits into from
Apr 27, 2025

Conversation

GearsDatapacks
Copy link
Member

Closes #4497 and fixes #4145
This PR improves the generated code for let assert in a couple of ways:

  • Instead of pattern matching on the value twice, we match once, extract the variables, then return a tuple from the case expression.
  • If a pattern always matches (e.g. a plain variable or tuple pattern) we omit the check entirely and just assign like a simple let assignment.
    This simplification fixes the other bug by default, as we no longer ignore variables in the first pattern match.

@giacomocavalieri
Copy link
Member

This entire portion of code is being worked on by the decision tree PR, which already gets rid of needless checks using the decision tree the compiler produces; and merging this would likely cause loads of conflicts with it. I'd love to avoid that! 😁

@GearsDatapacks
Copy link
Member Author

Ah I see. I thought that PR was only for the javascript target. My bad!

@giacomocavalieri
Copy link
Member

Ah Gears I'm so sorry I totally messed up! You're right, it is only for the js target 🙇‍♂️
This would not overlap all that much with the decision tree branch. If you want to, though you could still use the decision tree on the Erlang target as well to see if the let assert could be simplified, I'm not sure what would be easier but that's what I'm doing on the JS target if you want to have a look at the implementation

Sorry again for the confusion!

@GearsDatapacks
Copy link
Member Author

All good. I think this implementation is going to be fine for now. I imagine Erlang performs some optimisations using a decision tree also. Perhaps once the plumbing is in place for the decision tree we can improve upon it further

@lpil lpil force-pushed the improve-erlang-let-assert branch from 6af0c8b to a167361 Compare April 27, 2025 09:10
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!!

@lpil lpil enabled auto-merge (rebase) April 27, 2025 09:12
@lpil lpil merged commit f151619 into gleam-lang:main Apr 27, 2025
12 checks passed
@GearsDatapacks GearsDatapacks deleted the improve-erlang-let-assert branch April 27, 2025 09:29
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.

Simplifiy generated Erlang code for let assert Referencing result of earlier segment when pattern matching generates invalid Erlang
3 participants