arjan / decorator

Function decorators for Elixir
MIT License
385 stars 20 forks source link

[Question] What to do about used/unused variable warnings? #31

Open coladarci opened 5 years ago

coladarci commented 5 years ago

Forgive me for using this as a venue to get a recommendation, but I'm having a really hard time even forming my question in other venues, so I thought I'd try it out here since this library is what spurred this question.

My goal here is to explore adding permission checks to my context functions.

@decorate permit(:create_thing)
def create_thing(_user, params), do:
   %Thing{} |> Thing.changset(params) |> Repo.insert()

I have setup permit to use the first argument provided to the decorated function (the user):

def permit(action, body, %{args: [user, _params]}) do
   with :ok -> Permissions.can(user, action) do
     unquote(body)
   end
end

Note that in the above use-case, the user is actually only used in the decorated code, NOT the function being decorated. This confuses the compiler and gives warnings (if it's underscored, I'm told that it's being used even though it's underscored. If it's NOT underscored, I get warnings saying it's unused).

I have to note that this entire thing smells bad because my best case scenario here is that a seemingly unused arg is actually being used (or vice-versa). This makes me seriously question my API choices, but I think I'm onto a very powerful use of this library.

SO, my question is: 1) Is what I'm running into something that feels like a bug somewhere? Is it a common issue? 2) Seeing what I'm attempting to accomplish, am I missing anything obvious that might be a better API approach?

Thanks for a great library - appreciate all the work that went into it!

geonnave commented 5 years ago

+1, I am also having this issue. To complement, if I remove the _ from the beginning of the variable name, the compiler complains that the variable is unused; but if I add the _, it will say that it is used (probably because of the macro using it) and should not have a _.

Weird behaviours! 🙃

coladarci commented 5 years ago

Yeah - basically, I realized that I was in a lose-lose here... the idea that you write code that uses things that look like they aren't used is terrible :) ha.. was still curious if anyone else solved a similar use-case, but this is not the right venue for that, I suspect.

arjan commented 5 years ago

I will have a look.. has been some time since I wrote this, I'll see if I can make the generated code play nicer with the compiler.

arjan commented 5 years ago

This is more complicated than I expected. Tried detecting and renaming the variables on compile time but that does not seem to help much.

BenMorganIO commented 5 years ago

I'm experiencing this issue as well....

arjan commented 5 years ago

I have not continued with this yet, like I said, renaming the variables on compiletime does not work, it seem we are in a catch-22 here.

A solution would be to not include this unused argument in the function declaration, but instead let the decorator automatically add this "context" argument as the first (or last?) argument.

This would require a different annotation though, for instance called @decorate_with_context:

@decorate_with_context permit(:create_thing)
def create_thing(params), do:
   %Thing{} |> Thing.changset(params) |> Repo.insert()

This would NOT define create_thing/1, but create_thing/2 with the first argument passed through into the decorator.

def permit(action, body, %{args: [user, _params]}) do
  quote do
    with :ok -> Permissions.can(user, action) do
      unquote(body)
    end
  end
end
robacourt commented 3 years ago

I have this issue too. The workaround I've used may be useful for others if you know the type of your unused variable.

Instead of:

@decorate permit(:create_thing)
def create_thing(_user, params), do: ...

If you know that user is a map you can do:

@decorate permit(:create_thing)
def create_thing(%{}, params), do:

Not great, but if you have warnings as errors as we do, this workaround helped.

Arp-G commented 1 year ago

Using Kernel.binding/1 I was able to get rid of the un-used variable warnings, It's not the best way and seems like a hack, but it's all I would find for now.

Thanks to this PR for the idea

Example usage:

@decorate with_fallback()
def handle_event("accept_event", _, %{assigns: assigns} = socket) do

Then

def with_fallback(opts, body, _context) do
  quote do
    try do
      unquote(body)
    catch
      %DataSource.Error{} = error ->
      # handle error
      socket = Keyword.get(binding(), :socket)
      {:noreply, socket}
  end
end