Skip to content
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

fix pattern aliases for strings when targeting erlang #2782

Merged
merged 8 commits into from
Mar 28, 2024

Conversation

wgancar
Copy link
Contributor

@wgancar wgancar commented Mar 19, 2024

Hello, this is my attempt to fix the issue described here

#2733

Currently making aliases for string patterns doesn't work when complied to erlang. The problem is that erlang doesn't allow aliases for constant binary patterns.

My workaround is to match the number of bytes first and then write additional guard to check if matched bytes matches the provided pattern

so that this

case x {
  "Hello, " as greeting <> name  -> greeting
}

becomes this

<<Greeting:7/binary, Name/binary>> when Greeting =:= <<"Hello, ">> ->
            Greeting;

instead of invalid code that we have today

<<"Hello, "/utf8 = Greeting, Name/binary>> ->
            Greeting;

If this is acceptable approach I still have one issue to fix, If there are some user defined guards on the clause level I have to replace when with andalso to make sure that both guards are supported

@lpil
Copy link
Member

lpil commented Mar 20, 2024

The approach looks great to me! We do something similar to this on JavaScript. Let me know when you would like a code review, thank you.

@lpil lpil marked this pull request as draft March 20, 2024 20:37
@wgancar wgancar marked this pull request as ready for review March 21, 2024 11:32
@wgancar
Copy link
Contributor Author

wgancar commented Mar 21, 2024

Ok, I think im ready for review. Ive made sure that user defined guards are working properly and I've added test to verify that

@wgancar wgancar force-pushed the fix-pattern-aliases-for-strings-erlang branch from 7b67fa2 to ac34ef2 Compare March 21, 2024 11:55
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 but we cannot introspect the algebra to learn things about the code. The code is source of information here.

Could you also add comments like the ones you removed please 🙏

Vec(docs) => docs.iter().any(|d| d.contains_string(pattern)),
_ => false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove this please, it's not appropriate to introspect the printing alegbra, and this isn't a correct implementation as it could be across multiple variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right, sorry

@wgancar
Copy link
Contributor Author

wgancar commented Mar 21, 2024

Thank you but we cannot introspect the algebra to learn things about the code. The code is source of information here.

Could you also add comments like the ones you removed please 🙏

Thank you for the review I appreciate it a lot 💜 Im still not sure where this guard check should happen. Right now I made simple function in patterns module but Im not sure if its feels right. Any suggestions?

@wgancar wgancar force-pushed the fix-pattern-aliases-for-strings-erlang branch from fa2d3f1 to 66d99d4 Compare March 21, 2024 15:29
@wgancar wgancar requested a review from lpil March 25, 2024 07:04
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.

Beautiful work! Thank you very much! 💜

@lpil lpil force-pushed the fix-pattern-aliases-for-strings-erlang branch from 66d99d4 to 8589ffa Compare March 28, 2024 15:16
@lpil lpil enabled auto-merge (rebase) March 28, 2024 15:17
@lpil lpil merged commit 9655c5b into gleam-lang:main Mar 28, 2024
11 checks passed
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.

None yet

2 participants