Skip to content

Conversation

LilyRose2798
Copy link

@LilyRose2798 LilyRose2798 commented Aug 24, 2025

This fixes the issue where the following code snippets with discarded variables in utf8 bit array patterns are allowed to compile:

let assert <<_:utf8>> = <<>>
case <<>> {
  <<_:utf8>> -> todo
  _ -> todo

When the variable is not discarded, the compiler already correctly disallows the pattern, as variable length string matches are not allowed for bit arrays:

let assert <<x:utf8>> = <<>> // correctly gives a compiler error

The fix is just this single line change:

- Pattern::Variable { .. } if segment_type.is_string() => {
+ Pattern::Variable { .. } | Pattern::Discard { .. } if segment_type.is_string() => {

Credit to Julian (@julian.nz) in the Gleam Discord server for finding this issue.

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! Could you update the changelog bug fix section please 🙏

@LilyRose2798 LilyRose2798 force-pushed the main branch 2 times, most recently from 7203abc to 8c4a13b Compare August 26, 2025 07:34
@LilyRose2798
Copy link
Author

LilyRose2798 commented Aug 26, 2025

I've updated the changelog as well as rebasing onto the latest main commit (apologies for all the messy force pushes, I'm pretty new to doing pull requests and not sure of all the best practices for this).

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!

Force pushing is good by me 👍

@lpil lpil enabled auto-merge (rebase) August 26, 2025 09:51
@lpil
Copy link
Member

lpil commented Aug 26, 2025

Looks like there's a snapshot that needs to be accepted!

auto-merge was automatically disabled August 26, 2025 10:05

Head branch was pushed to by a user without write access

@LilyRose2798
Copy link
Author

Hopefully it should work now? I ran cargo insta review and accepted the snapshot.

@LilyRose2798
Copy link
Author

Well there are more checks passing now, but still some failing. Looking at the big block of code in the output for them, it seems like the code contains references to _:utf8 which may be the reason they're now failing. Not sure why cargo test runs successfully on my machine (linux) but not on those checks.

@GearsDatapacks
Copy link
Member

Looks like it's failing from this line of the stdlib: https://github.com/gleam-lang/stdlib/blob/main/src/gleam/bit_array.gleam#L70
On Erlang, _:utf8 is the same as _:utf8_codepoint, so I guess that's why the issue wasn't caught until now.

@LilyRose2798
Copy link
Author

LilyRose2798 commented Aug 26, 2025

Ah, thanks for the input. So I'm assuming the stdlib may need to be updated to use utf8_codepoint instead of utf8 in that location then before this PR can be merged?

@GearsDatapacks
Copy link
Member

Seems like it yes. In that case would this be considered a breaking change? Or will we treat it like a bug which was never intended?

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.

3 participants