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

Support Elixir expressions in Playground #564

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xxdavid
Copy link
Contributor

@xxdavid xxdavid commented Jan 17, 2025

Implements #520.

This is a partially working implementation. There are probably unnoticed bugs and edge cases when it does work, but it works quite well in a lot of cases.

All attribute types are now editable. When Elixir expressions are expected, there is an Elixir-like icon visualizing that the input must be Elixir code. The icon goes red when the code is not valid and its title says what the error is. Textareas are used for these inputs, so that user can make them bigger.

The Elixir evaluation uses Dune under the hood. Therefore, the evaluation is relatively safe. However, there are two issues I noticed. The first one is small – structs cannot be constructed as their syntactic form is forbidden (don't know why). This can be fixed by implementing a custom Allowlist. The second issue is more important. To prevent atom exhaustion, atoms that don't already exist are translated to atoms like : a__Dune_atom_1__. Although not fatal, this considerably lowers the usability. Maybe we can say that the expression evaluation in playground should be enabled only on local machines without remote access and then just use Code.eval_string/1

TODO:

  • Polish code
  • Fix bugs
  • Tackle the Dune drawbacks
  • Make the expression evaluation configurable (off by default)
  • Write tests

@cblavier
Copy link
Contributor

Looks amazing!
Do you think you can mix dune code evaluation with Code.eval_string/1 to circumvent atom limitations?

@xxdavid
Copy link
Contributor Author

xxdavid commented Jan 20, 2025

Yep, I think we could do something like

  defp eval_expr(string) do
    case Dune.eval_string(string) do
      %Dune.Success{} ->
        {evaluated, _binding} = Code.eval_string(string)
        %Expr{value: evaluated, string: string}

      %Dune.Failure{message: error} ->
        %Expr{string: string, error: error}
    end
  end

This would still prevent doing things like File.rm_rf!!("/") or File.read!("secret_file") or System.get_env(), but would allow shutting down the BEAM instance by atom exhaustion (and also evaluates the expression twice). We need to choose between these tradeoffs. What solution out of

  1. Dune only
  2. Dune + Code.eval_string/1
  3. Code.eval_string/1 only
    sounds best to you (neither of them is ideal)?

@cblavier
Copy link
Contributor

I'm not super fond of Dune-based solutions which are either half-functional (no atoms) or half-secure (can crash the VM)
I would rather use Code.eval_string/1 without Dune, but only for the dev environment.
WDYT?

@xxdavid
Copy link
Contributor Author

xxdavid commented Jan 20, 2025

Running just Code.eval_string/1 seems okay to me if we warn users enough not to use this in production. Would you also create the new option, or just always enable it in the dev environment and that's it?

@cblavier
Copy link
Contributor

It can default to enabled in dev, disabled otherwise.

We already have this kind of default behavior with compilation_mode defaulting to :lazy in dev and :eager in other environments.

defp compilation_mode(opts) do

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.

2 participants