Pattern matching using [first | rest] in with clause seems to fail?

[vegabook]

:digraph.add_edge gives unpredictable results when adding an edge, one only knows that the first element in the list will be :"$e"

I am trying to create an atomic :digraph.add_vertex+:digraph.add_edge+:dets.insert function. If any clauses in the with statement below fail, I need to roll back the previous ones. So if the last clause (:dets.insert) fails, I need to delete the vertex and the edge I just created. In order to do that, I need to destructure the return of the :digraph.add_edge function in the with clause, so that I can refer back to the created edge in the else part…

defp atomic_vedge({detspath, graph}, v, parent, meta, ts) do                                                        
    # atomic add of a vertex and its edge to parent and save to dets                                                  
    # if any stage fails preceding successes are rolled back                                                          
    with {:addvertex, v} <- {:addvertex, :digraph.add_vertex(graph, v, meta)},                                        
         {:addedge, [:"$e" | rest]} <- {:addedge, :digraph.add_edge(graph, parent, v)},                               
         {:insert, :ok} <-                                                                                            
           {:insert, :dets.insert(detspath, {v, parent, ts, :vertedge, meta})} do                                     
      {:ok, {detspath, graph}}                                                                                        
    else                                                                                                              
      {:addvertex, {:error, reason}} ->                                                                               
        {:error, reason}                                                                                              
                                                                                                                      
      {:addedge, {:error, reason}} ->                                                                                 
        :digraph.del_vertex(graph, v)                                                                                 
        {:error, reason}                                                                                              
                                                                                                                      
      {:insert, false} ->                                                                                             
        :digraph.del_vertex(graph, v)                                                                                 
        :digraph.del_edge(graph, [:"$e" | rest])                                                                      
        {:error, "dets vertex insert failed"}                                                                         
    end                                                                                                               
  end              

However this is giving me an error when compiling:

Does this mean I cannot destructure into [first | rest] in a with statement? So I cannot track what the :digraph.add_edge returned in order to roll it back? It seems to be a problem with the [head | rest] part because the previous {:addvertex, v} doesn’t seem to cause a problem.

(the problem here btw is that :digraph is side-effecty. It doesn’t return a new graph each time because it uses ETS tables and that’s why I’m having to jump through these hoops)

[sabiwara]

This is a scope problem: rest is not available in the else clauses since it only gets defined when the pattern-matching succeeds.

Since you only need it in the insert case, you could pass it in the tuple maybe? {:insert, rest, :dets.insert(...)}

Regarding v, it is available in the parent scope, which is why you’re not getting the error (but it might not be the value of v you expect). Here is a minimal example:

iex(1)> with [x] <- 0, do: x, else: (_ -> x)
error: undefined variable "x"
  iex:1
** (CompileError) cannot compile code (errors have been logged)
iex(1)> x = 1
1
iex(2)> with [x] <- 0, do: x, else: (_ -> x)
1

PS: Complex else clauses in with can be considered an anti-pattern, maybe this code would actually be clearer with simply nesting case? This way, the necessary variables from intermediate matches would be in scope.

[Eiji]

Elixir works as expected. The bug is in your code. Firstly let’s take a look at the warning. As it says the rest variable is not used inside do … else block. Secondly the value is assigned to variable only on success, so it cannot be used outside of do … else block which contains instructions for success scenario. Having above in mind the error message is also correct as there is no rest variable assigned for else … end block which contains an instructions when with matches fails.

You can still access this data, but this is a bit tricky … See a simplified example below:

# a simple list of atoms
list = ~w[a b c]a
# simplified with
with [a | bc] <- list,
  # a tricky part i.e. we need to reassign the list for every next result
  {true, _reassigned_list} <- {is_nil(a), list}
  # possibly more clauses here with list reassignment
  do
  {"a is nil", a, bc}
else
  # here a list is available not from success matching, but as a failed match result
  {false, [a | bc]} -> {"a is not nil", a, bc}
end

The above code obviously returns:

{"a is not nil", :a, [:b, :c]}

[vegabook]

yeah I grokked the basics of with statements and liked them so much I wanted to use them extensively. Maybe too extensively! Now I grok them properly. Thank you. Back to good ol’ nested case in this (ahem) case.

[Eiji]

@sabiwara

PS: Complex else clauses in with can be considered an anti-pattern, maybe this code would actually be clearer with simply nesting case? This way, the necessary variables from intermediate matches would be in scope.

Nesting case is also anti-pattern. Since Elixir version 1.12.0 (see Kernel.then/2) and especially after Livebook version 0.7 (see Interactive user interface to visualize and edit Elixir pipelines) my personal guilty pleasure is to use pipes often, really really often.

~w[a b c]a
|> then(fn
  [nil | bc] -> {:ok, {"a is nil", a, bc}}
  [a | bc] -> {:ok, {"a is not nil", a, bc}}
  data -> {:error, {"data does not match", data}}
end)

It’s much more clear than above with code since we see much better:

1. On what data we are working (value in pipe)
2. Every variable scope, so we understand the code much easier just looking at it

[kokolegorille]

@Eiji

~w[a b c]a
|> then(fn ->
  [nil | bc] -> {:ok, {"a is nil", a, bc}}
  [a | bc] -> {:ok, {"a is not nil", a, bc}}
  data -> {:error, {"data does not match", data}}
end)

You might have an arrow too much… it should be fn instead of fn →

Where I could use multiple heads anonymous function… I would prefer normal Module function with multiple heads.

But it’s just a question of taste

[adamu]

@Eiji

see Kernel.then/2

Did you just tell the guy who wrote it to look it up?

[Eiji]

@kokolegorille

You might have an arrow too much… it should be fn instead of fn →

Thanks, yes I have written it from memory.

@kokolegorille

Where I could use multiple heads anonymous function… I would prefer normal Module function with multiple heads.

Personally I have 3 levels here:

1. If clauses are simple I’m rather not changing them
2. If applicable I use similar to tap/then macros like for example the ones from ok hex package
3. If 2 rules above are not enough to keep pipe simple I use private functions as you have suggested

@adamu

Did you just tell the guy who wrote it to look it up?

Umm … maybe?

1. Obviously I do not remember who worked on every Elixir feature
2. At least I’m trying to write my posts as generic as possible, so I do not target only one person
3. To be strict I replied not to him, but to his message about nesting conditionals i.e. my message would be same no matter who wrote it
4. And … well … thanks for introducing such an amazing feature!

[vegabook]

defp atomic_vedge({detspath, graph}, v, parent, meta, ts) do                                                        
    # atomic add of a vertex and its edge to parent and save to dets                                                  
    # if any stage fails preceding successes are rolled back                                                          
    # yes nested case anti pattern yada but this is fine with only 3 levels                                           
    # https://elixirforum.com/t/pattern-matching-using-first-rest-in-with-clause-seems-to-fail/60541                  
    case :digraph.add_vertex(graph, v, meta) do                                                                       
      v ->                                                                                                            
        case :digraph.add_edge(graph, parent, v) do                                                                   
          [:"$e" | rest] ->                                                                                           
                                                                                                                      
            dets_result =                                                                                             
            try do                                                                                                    
              :dets.insert(detspath, {v, parent, ts, :vertedge, meta})                                                
            rescue                                                                                                    
              e in ArgumentError -> {:error, e}                                                                       
            end                                                                                                       
                                                                                                                      
            case dets_result do                                                                                       
              :ok -> {:ok, {detspath, graph}} # success function returns                                              
              # dets fail so rollback edge and vertex inserts                                                         
              {:error, e} ->                                                                                          
                :digraph.del_edge(graph, [:"$e" | rest])                                                              
                :digraph.del_vertex(graph, v)                                                                         
                {:error, e}                                                                                           
            end                                                                                                       
          # edge insert fail so rollback vertex insert                                                                
          {:error, reason} ->                                                                                         
            :digraph.del_vertex(graph, v)                                                                             
            {:error, reason}                                                                                          
        end                                                                                                           
      # vertex insert fail so return error                                                                            
      some_error ->                                                                                                   
        {:error, some_error}                                                                                          
    end                                                                                                               
  end                                 

Looks okay by me the nested case. Thanks for help. Actually I don’t see how this is really an anti-pattern as @Eiji was saying, for modest nesting depths, and Racket people might actually love it

The only slightly messy thing here is trapping the :dets error, and I know “let it fail” and all that but in this case I do need to handle it because otherwise I’ll have dangling edges and vertices in my graph. Any thoughts on this erlang lib error trapping would be appreciated but this thing for now passes a bunch of tests. Again thank you for help to all.

[Eiji]

@vegabook

Looks okay by me the nested case. Thanks for help. Actually I don’t see how this is really an anti-pattern as @Eiji was saying, for modest nesting depths, and Racket people might actually love it

Never heard about “Racket people”. Google also translates racket as same as rocket. I guess such people prefer to not use chaining calls in Ruby or pipes in Bash and of course they have their own secret arguments, I never heard of, for that.

@vegabook

and I know “let it fail” and all that

Sorry, but it sounds like an Elixir developer saying about other language:

and I know “object-oriented programming” and all that

Well … no matter what we talk about like pipes, let it fail principle or literally anything else … They are all parts of language, no? Depending on the case you would sooner or later use them more or less and I really don’t get a point when people coming from other languages expects … I’m not even sure what exactly …

As always I’m speaking generally. One time I saw someone doing … something? Just imagine that whole logic is in only one function (from parsing custom absinthe type, via middleware, resolver and context logic up to custom ecto queries). 300+ LOC for just one function, over 10 levels of indention, even rewritten functions of Elixir core and mandatorily with all credo rules disabled (he added credo btw.). All of that because:

1. Everything is in one place, so no need to change file == easier to read
2. Pipes are completely new “magic” introduced by Elixir and therefore they are not clear for new people
3. Elixir core functions does not work as a language he came from
4. credo and I following it “complain”
5. don’t ask about style guide and just write code like I do
   and some other arguments like that

After experiencing a hell of unclear rules coming from “some language” by default I just can’t look positively on a code which do not follow any Elixir style guide. Of course I don’t mean to force someone to use my favourite one. Also there is no rule which covers all cases, but especially on forum when sharing opinions and code samples for possibly thousands of readers I try to keep consistent style which is well explained in some style guide.

Of course Elixir language gives a lot of freedom, for example we could even use non-ASCII characters in variable names, so in theory I could write a code with Polish naming. I can of course write tons of delegated functions and make all core functions be callable using Polish naming and just for sure for me it would look good. First of all I’m native Polish and secondly it would be my own code, so I can consider it as a clear.

However with all that freedom there comes responsibility. Such code is not clear for everyone and therefore it’s definitely not good at least when sharing in international community. Same comes for other things like many indention levels. There is no point in thinking if for you or someone else it’s clear. All I know is that it’s terribly hard to read in my personal opinion and from my perspective such code should be refactored.

As always I’m never trying to be guru. After all that years I’m sure about few things and one of them is to follow in most cases a good practices, style guides and other resources like that. Instead of forcing other to do something in specific way it’s better to show alternatives. I believe that my simplified version speaks for itself well enough, so that’s all from me. Have a good night.

[sabiwara]

I agree that avoiding over-nesting and splitting in small functions is good advice in general.

Regarding what the exact maximum nesting level should be, 1 feels quite opinionated. For example, it is quite easy to find examples that are nesting 2 or 3 case expressions within the Elixir codebase.
But like anything readability related, this is a very controversial topic, so I don’t think that there’s such a thing as a correct answer.

For the record, here is an opposite take embracing nesting over splitting functions (not necessarily agreeing but interesting to hear both sides).

[Eiji]

For me max indention level should be 4 in most cases, because:

1. I fully agree with credo rule linked above
2. My personal example is pretty simple

defmodule Example do
  # first is obvious for function/macro definition
  defmacro sample do
    # similarly this extra indention in macros is obvious
    quote do
      # max optional 1 extra level for `credo` rule goes here
    end
  end
end

Honestly I’m already tired seeing 4x end at the end, but quote is a special case and I’m fine with some workarounds like for example I usually put macros with quote above public functions.

[vegabook]

https://racket-lang.org/

[adamu]

@sabiwara

examples

I believe this:

  defp adjust_position(reversed_pre, post) do
    case move_spaces(post, reversed_pre) do
      # If we are between spaces and a dot, move past the dot
      {[?. | post], reversed_pre} when hd(post) != ?. and hd(reversed_pre) != ?. ->
        {post, reversed_pre} = move_spaces(post, [?. | reversed_pre])
        {reversed_pre, post}

      _ ->
        case strip_spaces(reversed_pre, 0) do
          # If there is a dot to our left, make sure to move to the first character
          {[?. | rest], _} when rest == [] or hd(rest) not in ~c".:" ->
            {post, reversed_pre} = move_spaces(post, reversed_pre)
            {reversed_pre, post}

          # If there is a % to our left, make sure to move to the first character
          {[?% | _], _} ->
            case move_spaces(post, reversed_pre) do
              {[h | _] = post, reversed_pre} when h in ?A..?Z ->
                {reversed_pre, post}

              _ ->
                {reversed_pre, post}
            end

          _ ->
            {reversed_pre, post}
        end
    end
  end

Can be simplified down quite a bit. It looks like everything below the first case is a no-op. But I’m not sure if I understood it. At least the tests pass.

  defp adjust_position(reversed_pre, post) do
    case move_spaces(post, reversed_pre) do
      # If we are between spaces and a dot, move past the dot
      {[?. | post], reversed_pre} when hd(post) != ?. and hd(reversed_pre) != ?. ->
        {post, reversed_pre} = move_spaces(post, [?. | reversed_pre])
        {reversed_pre, post}

      {post, reversed_pre} ->
        {reversed_pre, post}
    end
  end

[Eiji]

Oh, first time see this. Please next time use Rachet devs instead of people as it’s confusing.

Merry Christmas!

[vegabook]

yeah I’m gonna say 90% of Elixir devs know what Racket is.

BTW if I may say, this ~w[a b c]a is a complete code-golf gratuitous overcomplication which doesn’t even save a single keystroke. You should refrain from trying to look clever because it detracts from your otherwise decent points.

Merry Christmas to you too!