Skip to content

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Aug 23, 2025

Another PoC, this time using the expand-time approach.

Working at expand time is a bit awkward because we're manipulating map AST, but unlike the escape approach from #14720, it doesn't break the assumption that escaping emits a literal.

The first commit does it in a regex-aware way.
The second commit is a proof of concept for what a no-cheating version could look like.

The idea:

  • the current escape code checking for refs in maps will, instead of raising, replace the value by :__expand_compile__ (name TBD), if it's a struct defining StructMod.__expand_compile__(struct, key)
  • when expanding the map AST, if the arg kv-list contains an :__expand_compile__ value (relatively cheap to check), we confirm it's a literal (more expensive) struct defining the fun, and we call StructMod.__expand_compile__(struct, key) to get the new value AST
  • alternatively, we could probably add an option to do_quote to let refs through and add a meta to the map to let elixir_expand know to swap it - but I'm not sure there's value in allowing refs in AST if we don't need them anyway?
  • nice-to-have side effect (just an idea): in match contexts, we could replace :__expand_compile__ with a wildcard (_), since we can't call functions like :re.import anyway and refs would otherwise never match.

defp translate_options(<<>>, acc), do: acc
defp translate_options(t, _acc), do: {:error, t}

if Code.ensure_loaded?(:re) and function_exported?(:re, :import, 1) do
Copy link
Member

Choose a reason for hiding this comment

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

We would need to check this at runtime, because Elixir compiled for OTP 27 may run on OTP 28.

@josevalim
Copy link
Member

Nice! 😍 I like using the approach of a custom callback in the struct module. However, I don't understand why we need to change elixir_map. In theory, we only need to escape compile times structs, when we are expanding/injecting them. So changing the escaping code should be enough?

@sabiwara
Copy link
Contributor Author

sabiwara commented Aug 23, 2025

However, I don't understand why we need to change elixir_map

This is what :elixir_expand ends up calling: https://github.com/elixir-lang/elixir/blob/main/lib/elixir/src/elixir_expand.erl#L27-L28.

We could indeed do it directly in escape (#14720), but the issue is that it makes escape return an AST that is not a literal anymore, which is why I figured you suggested :elixir_expand?
I could try to use a callback for #14720 as well in this case, it should be even simpler than the expand version.

@josevalim
Copy link
Member

#14720 is definitely the way to go. We indeed just need the additional callback if we want to make it generic.

@sabiwara
Copy link
Contributor Author

OK! 🚀
Will close this one and continue on #14720, thanks for the confirmation!

@sabiwara sabiwara closed this Aug 23, 2025
@sabiwara sabiwara deleted the import-re-expand branch August 30, 2025 02:35
@sabiwara sabiwara restored the import-re-expand branch August 30, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants