<div dir="ltr"><div><div><div dir="auto"><div dir="auto">Hi everyone,</div><div dir="auto"><br></div><div dir="auto">I have received some feedback during the last few days, mostly on IRC and Twitter regarding this proposal. Mostly a lot of it was positive, but not all of it.</div><div dir="auto"><br></div><div dir="auto">The most common criticism related to the choice to limit the construct to patterns of ok | {ok | error, term()}.</div><div dir="auto"><br></div><div dir="auto">I guess if we wanted to type the values it would be:</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:monospace,monospace">-type error(E) :: ok | {error, E}.</span></div><div dir="auto"><span style="font-family:monospace,monospace">-type error(T, E) :: {ok, T} | {error, E}.</span></div><div dir="auto"><br></div><div dir="auto">Anyway the criticism came from people who would prefer the construct to work with a full explicit pattern—to make the distinction clear, I’ll reuse the list comprehension <span style="font-family:monospace,monospace"><-</span> arrow instead of <span style="font-family:monospace,monospace"><~</span> when referring to that.</div><div dir="auto"><br></div><div dir="auto">So instead of</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:monospace,monospace">begin </span></div><div dir="auto"><span style="font-family:monospace,monospace"> {X,Y} <~ id({ok, {X,Y}})</span></div><div dir="auto"><span style="font-family:monospace,monospace"> ...</span></div><div dir="auto"><span style="font-family:monospace,monospace">end</span></div><div dir="auto"><br></div><div dir="auto">You would have to write:</div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">begin </span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, {X,Y}} <- id({ok, {X,Y}})</span></div><div dir="auto"><span style="font-family:monospace,monospace"> ...</span></div><div dir="auto"><span style="font-family:monospace,monospace">end</span></div><div dir="auto"><br></div><div dir="auto">This is a fine general construct, but I believe it is inadequate and even dangerous for errors; it is only good at skipping patterns, but can’t be safely used as a good error handling mechanism.</div><div dir="auto"><br></div><div dir="auto">One example of this could be taken from the current OTP pull request that adds new return value to packet reading based on inet options: <div><a href="https://github.com/erlang/otp/pull/1950" target="_blank">https://github.com/erlang/otp/<wbr>pull/1950</a></div></div></div><div dir="auto"><br></div><div dir="auto">This PR adds a possible value for packet reception to the current form:</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:monospace,monospace">{ok, {PeerIP, PeerPort, Data}}</span></div><div dir="auto"><br></div><div dir="auto">To ask make it possible to alternatively get:</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:monospace,monospace">{ok, {PeerIP, PeerPort, AncData, Data}}</span><br></div><div dir="auto"><br></div><div dir="auto">Based on socket options set earlier. So let’s put it in context for the current proposal:</div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">begin </span></div><div dir="auto"><span style="font-family:monospace,monospace"> {X,Y} <~ id({ok, {X,Y}}),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {PeerIP, PeerPort, Data} <~ gen_udp:recv(...),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> ...</span></div><div dir="auto"><span style="font-family:monospace,monospace">end</span></div><div dir="auto"><br></div><div dir="auto">If <span style="font-family:monospace,monospace">AncData</span> is received, an exception is raised: the value was not an error but didn’t have the shape or type expected for the successful pattern to match. Errors are still returned properly by exiting the <span style="font-family:monospace,monospace">begin ... end</span> block, and we ensure correctness in what we handle and return.</div><div dir="auto"><br></div><div dir="auto">However, had we used this form:</div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">begin </span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, {X,Y}} <- id({ok, {X,Y}}),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, {PeerIP, PeerPort, Data}} <- gen_udp:recv(...),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> ...</span></div><div dir="auto"><span style="font-family:monospace,monospace">end</span></div></div></div><div dir="auto"><br></div><div dir="auto">Since this would return early on any non-matching value (we can’t take a stance on what is an error or ok value aside from the given pattern), the whole expression, if the socket is configured unexpectedly, could return <span style="font-family:monospace,monospace">{ok, {PeerIP, PeerPort, AncData, Data}}</span> on a failure to match</div><div dir="auto"><br></div><div dir="auto">Basically, an unexpected but good result could be returned from a function using the <span style="font-family:monospace,monospace">begin ... end</span> construct, which would look like a success while it was actually a complete failure to match and handle the information given. This is made even more ambiguous when data has the right shape and type, but a set of bound variables ultimately define whether the match succeeds or fails.</div><div dir="auto"><br></div><div dir="auto">In worst cases, It could let raw unformatted data exit a conditional pipeline with no way to detect it after the fact, particularly if later functions in <span style="font-family:monospace,monospace">begin ... end</span> apply transformations to text, such as anonymizing or sanitizing data, for example. This could be pretty unsafe and near impossible to debug well.</div><div dir="auto"><br></div><div dir="auto">Think for example of:</div><div dir="auto"><br></div><div dir="auto"><span style="font-family:monospace,monospace">-spec fetch() -> iodata().</span></div><div dir="auto"><span style="font-family:monospace,monospace">fetch() -></span></div><div dir="auto"><span style="font-family:monospace,monospace"> begin</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, B = <<_/binary>>} <- f(),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> true <- validate(B),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, sanitize(B)}</span></div><div dir="auto"><span style="font-family:monospace,monospace"> end.</span></div><div dir="auto"><br></div><div dir="auto">If the value returned from <span style="font-family:monospace,monospace">f()</span> turns out to be a list (say it’s a misconfigured socket using <span style="font-family:monospace,monospace">list</span> instead of <span style="font-family:monospace,monospace">binary</span> as an option), the expression will return early, the <span style="font-family:monospace,monospace">fetch()</span> function will still return <span style="font-family:monospace,monospace">{ok, iodata()}</span> but you couldn’t know as a caller whether it is the transformed data or non-matching content. It would not be obvious to most developers either that this could represent a major security risk by allowing unexpected data to be seen as clean data.</div><div dir="auto"><br></div><div dir="auto">It is basically a risky pattern if you want your code to be strict or future-proof in the context of error handling. The current proposal, by comparison, would raise an exception on unexpected good values, therefore preventing ways to sneak such data into your control flow:</div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">-spec fetch() -> iodata().</span></div><div dir="auto"><span style="font-family:monospace,monospace">fetch() -></span></div><div dir="auto"><span style="font-family:monospace,monospace"> begin</span></div><div dir="auto"><span style="font-family:monospace,monospace"> B = <<_/binary>> <~ f(),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> _ <~ validate(B), % returns ok if valid</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, sanitize(B)}</span></div><div dir="auto"><span style="font-family:monospace,monospace"> end.</span></div></div><div dir="auto"><br></div><div dir="auto">Here misconfigured sockets won’t result in unchecked data passing trough your app.</div><div dir="auto"><br></div><div dir="auto">The general pattern mechanism may have a place, but I believe it could not guarantee the same amount of safety as the current proposal in any error-handling context, which was my concern in writing EEP 49.</div><div dir="auto"><br></div><div dir="auto">I can think of no way to make the general pattern approach safer than or even as safe as the currently suggested mechanism under its current form. You would have to necessarily add complexity to the construct with a kind of ‘<span style="font-family:monospace,monospace">else</span>’ clause where you must handle all non-matching cases explicitly if you want them returned (this is what Elixir allows), but unless the clause is mandatory (it is not in Elixir), you will have that risky ambiguity possibly hiding in all pattern matches:<br></div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">-spec fetch() -> iodata().</span></div><div dir="auto"><span style="font-family:monospace,monospace">fetch() -></span></div><div dir="auto"><span style="font-family:monospace,monospace"> begin</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, B = <<_/binary>>} <- f(),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> true <- validate(B),</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, sanitize(B)}</span></div><div dir="auto"><span style="font-family:monospace,monospace"> else</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {error, _} = E -> E;</span></div><div dir="auto"><span style="font-family:monospace,monospace"> false -> false</span></div><div dir="auto"><span style="font-family:monospace,monospace"> end.</span></div></div><div dir="auto"><br></div><div dir="auto">Putting it another way, to get the same amount of safety, you’d have to re-state all acceptable error forms just to ensure the unexpected cases don’t go through and instead appropriately raise exceptions, which would likely be a missing else clause exception. This exception would obscure the the original error site as well, something the current form does not do.</div><div dir="auto"><br></div><div dir="auto">The follow up question I ask is whether this would be a significant improvement over the current Erlang code, making it worth the new construct?<br></div><div dir="auto"><br></div><div dir="auto"><div dir="auto"><span style="font-family:monospace,monospace">-spec fetch() -> iodata().</span></div><div dir="auto"><span style="font-family:monospace,monospace">fetch() -></span></div><div dir="auto"><span style="font-family:monospace,monospace"> case f() of</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {ok, B = <<_/binary>>} -></span></div><div dir="auto"><span style="font-family:monospace,monospace"> case validate(B) of</span></div><div dir="auto"><span style="font-family:monospace,monospace"> true -> {ok, sanitize(B)};</span></div><div dir="auto"><span style="font-family:monospace,monospace"> false -> false</span></div></div></div></div></div><div><div><div dir="auto"><span style="font-family:monospace,monospace"> end;</span></div><div dir="auto"><span style="font-family:monospace,monospace"> {error, _} = E -> E</span></div><div dir="auto"><span style="font-family:monospace,monospace"> end.</span></div><div dir="auto"><br></div></div></div><div><div><div dir="auto">compared to the <span style="font-family:monospace,monospace"><-</span> form, it has roughly the same line count (a few more the more clauses are added), but nested <span style="font-family:monospace,monospace">case</span>s have the benefit of making it obvious which bad matches are acceptable in which context. Here, <span style="font-family:monospace,monospace">f()</span> won’t be able to validly return <span style="font-family:monospace,monospace">false</span> as a surprise value whereas the general pattern form would.<br></div><div dir="auto"><br></div><div dir="auto">This problem does not exist in the EEP 49 mechanism specifically because it mandates patterns that denote unambiguous success or failure conditions. You end up with code that is shorter and safer at the same time.</div><div dir="auto"><br></div><div dir="auto">I hope this helps validate the current design, but let me know if you disagree.<br></div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">Fred</div>
</div>
</div>
</div>