<div dir="ltr"><div class="gmail_default" style="font-family:tahoma,sans-serif">Vlad</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif">Thank you for such a thorough job!  You are an example to us all.</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif">Simon<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 29 Nov 2022 at 10:56, Vladislav Zavialov <<a href="mailto:notifications@github.com">notifications@github.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><p></p>
<p dir="auto">Hi <a class="gmail-notranslate" href="https://github.com/bgamari" target="_blank">@bgamari</a>! I have finished another round of review and identified a few potential improvements.</p>
<ol>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Proposal structure.</strong> At the moment, the "Proposed Change Specification" section contains a lot of information in addition to the specification itself. It's mixed with exposition and implementation-specific code snippets. Instead, I expect to see a concise list (bulleted or numbered) of proposed changes, as seen from the end-user perspective. Undoubtedly, the rest of the prose is also valuable, so I ask you not to remove it but to distribute it across other sections of the proposal text.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Make <code class="gmail-notranslate">ExceptionContext</code> opaque.</strong> The proposal define <code class="gmail-notranslate">ExceptionContext</code> as a wrapper around <code class="gmail-notranslate">[SomeExceptionAnnotation]</code>, but should we expose this fact to our users? What if we decide to store additional information there? I believe it would be best not to export the data constructor to allow for future changes.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Remove the <code class="gmail-notranslate">Semigroup</code> instance for <code class="gmail-notranslate">ExceptionContext</code></strong>. The annotations are stored in a list, so <code class="gmail-notranslate">(<>)</code> is O(n), and that's not good. Furthermore, I can't think of a use case for appending annotations from different exceptions, so there's a simple way to address this: do not define <code class="gmail-notranslate">Semigroup</code> or <code class="gmail-notranslate">Monoid</code> instances.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Semantics of <code class="gmail-notranslate">ExceptionContext</code>.</strong> The list of <code class="gmail-notranslate">SomeExceptionAnnotation</code> is ordered, so we ought to define how users can interpret this order. What does it mean for one annotation to come before another? Here's an idea: introduce an operation similar to <a href="https://hackage.haskell.org/package/annotated-exception-0.2.0.4/docs/Control-Exception-Annotated.html#v:checkpoint" rel="nofollow" target="_blank"><code class="gmail-notranslate">checkpoint</code></a> from <code class="gmail-notranslate">annotated-exceptions</code>:</p>
<div dir="auto"><pre><span>annotateIO</span> <span>::</span> <span>ExceptionAnnotation</span> <span>a</span> <span>=></span> <span>a</span> <span>-></span> <span><span>IO</span></span> <span>r</span> <span>-></span> <span><span>IO</span></span> <span>r</span></pre></div>
<p dir="auto">The order of <code class="gmail-notranslate">SomeExceptionAnnotation</code> would reflect the nesting of calls to <code class="gmail-notranslate">annotateIO</code>, reflecting the call stack in its own way (separately from the collected backtraces).</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Group backtraces in a record.</strong> If we say that a single element in <code class="gmail-notranslate">SomeExceptionAnnotation</code> corresponds to a call to <code class="gmail-notranslate">annotateIO</code>, it is somewhat strange to add multiple annotations for a single event of collecting backtraces. After all, what would it mean for the <code class="gmail-notranslate">HasCallStack</code> backtraces to come before the <code class="gmail-notranslate">CostCentre</code> backtrace or vice versa? So introduce a record to store them together:</p>
<div dir="auto"><pre>  <span>data</span> <span>Backtraces</span> <span>=</span>
    <span>Backtraces</span> <span>{</span>
      <span>costCentreBacktrace</span> <span>::</span> <span><span>Maybe</span></span> (<span>Ptr</span> <span>CostCentreStack</span>),
      <span>hasCallStackBacktrace</span> <span>::</span> <span><span>Maybe</span></span> <span><span>GHC.Stack.</span>CallStack</span>,
      <span>executionBacktrace</span> <span>::</span> <span><span>Maybe</span></span> [<span><span>GHC.ExecutionStack.</span>Location</span>],
      <span>ipeBacktrace</span> <span>::</span> <span><span>Maybe</span></span> [<span>StackEntry</span>]
   <span>}</span></pre></div>
<p dir="auto">(I refine this idea in the next suggestion)</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Encode the <code class="gmail-notranslate">Backtraces</code> record with GADTs</strong>. The record type <code class="gmail-notranslate">Backtraces</code> that I suggest in the previous point introduces a slightly annoying form of code duplication. The proposal already has an enumeration of backtrace mechanisms:</p>
<div dir="auto"><pre><span>data</span> <span>BacktraceMechanism</span>
   <span>=</span> <span>CostCenterBacktraceMech</span>
   | <span>ExecutionStackBacktraceMech</span>
   | <span>IPEBacktraceMech</span>
   | <span>HasCallStackBacktraceMech</span></pre></div>
<p dir="auto">And in the record, we have a field per mechanism, each wrapped in a <code class="gmail-notranslate">Maybe</code>. Fortunately, there is an encoding that removes this duplication. Let us index <code class="gmail-notranslate">BacktraceMechanism</code> by the representation type of the backtrace:</p>
<div dir="auto"><pre><span>data</span> <span>BacktraceMechanism</span> <span>a</span> <span>where</span>
  <span>CostCentreBacktrace</span>   <span>::</span> <span>BacktraceMechanism</span> (<span>Ptr</span> <span>CostCentreStack</span>)
  <span>HasCallStackBacktrace</span> <span>::</span> <span>BacktraceMechanism</span> <span><span>GHC.Stack.</span>CallStack</span>
  <span>ExecutionBacktrace</span>    <span>::</span> <span>BacktraceMechanism</span> [<span><span>GHC.ExecutionStack.</span>Location</span>]
  <span>IPEBacktrace</span>          <span>::</span> <span>BacktraceMechanism</span> [<span>StackEntry</span>]</pre></div>
<p dir="auto">Now we can encode the set of enabled mechanisms as a function to <code class="gmail-notranslate">Bool</code> and the record of collected backtraces as a function to <code class="gmail-notranslate">Maybe a</code>. Something along the lines of:</p>
<div dir="auto"><pre><span>type</span> <span>EnabledBacktraceMechanisms</span> <span>=</span> <span>forall</span> <span>a</span><span>.</span> <span>BacktraceMechanism</span> <span>a</span> <span>-></span> <span><span>Bool</span></span>
<span>type</span> <span>Backtraces</span> <span>=</span> <span>forall</span> <span>a</span><span>.</span> <span>BacktraceMechanism</span> <span>a</span> <span>-></span> <span><span>Maybe</span></span> <span>a</span></pre></div>
<p dir="auto">This isn't as low-tech as a an enum and a record, and I realize that low-tech solutions are appealing in their own way, but in this particular case, I find that GADTs offer a more elegant encoding.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Hide the implicit parameter behind a synonym</strong>. We choose to pass around the exception context as an implicit parameter, but this should be hidden from the end user. This is the way it's done with <code class="gmail-notranslate">HasCallStack</code>, where documentation clearly states:</p>
<blockquote>
<p dir="auto">NOTE: The implicit parameter <code class="gmail-notranslate">?callStack :: CallStack</code> is an implementation<br>
detail and <strong>should not</strong> be considered part of the CallStack API, we may<br>
decide to change the implementation in the future.</p>
</blockquote>
<p dir="auto">Let's do the same and introduce a synonym for exception contexts:</p>
<div dir="auto"><pre><span>type</span> <span>HasExceptionContext</span> <span>=</span> (<span>?</span><span>exceptionContext</span> <span>::</span> <span>ExceptionContext</span>)</pre></div>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Preserve backtraces on rethrowing</strong>. The way the proposal is currently written, when an exception is caught and rethrown, its old backtrace is dropped and a new one is constructed. This is very bad, because rethrowing happens all the time (e.g. in <code class="gmail-notranslate">bracket</code>)! But it is not hard to fix: <code class="gmail-notranslate">catch</code> should provide the context to the handler as an implicit parameter, and <code class="gmail-notranslate">throw</code> should make use of it:</p>
<div dir="auto"><pre><span>catch</span> <span>::</span> <span>Exception</span> <span>e</span> <span>=></span> <span><span>IO</span></span> <span>a</span> <span>-></span> (<span>HasExceptionContext</span> <span>=></span> <span>e</span> <span>-></span> <span><span>IO</span></span> <span>a</span>) <span>-></span> <span><span>IO</span></span> <span>a</span>
<span>throwIO</span> <span>::</span> (<span>HasExceptionContext</span>, <span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span><span>IO</span></span> <span>a</span></pre></div>
<p dir="auto">What about uses of <code class="gmail-notranslate">throwIO</code> where <code class="gmail-notranslate">HasExceptionContext</code> has not been provided by <code class="gmail-notranslate">catch</code>? Easy: default to an empty context. It shouldn't be hard to add this special case to the solver, since <code class="gmail-notranslate">HasCallStack</code> has already set a precedent.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Get rid of <code class="gmail-notranslate">toExceptionWithContext</code></strong>. The proposal introduces a new method to <code class="gmail-notranslate">Exception</code>:</p>
<div dir="auto"><pre><span>toException</span> <span>::</span> <span>Exception</span> <span>e</span> <span>=></span> <span>e</span> <span>-></span> <span>SomeException</span>                                 <span><span>--</span> old</span>
<span>toExceptionWithContext</span> <span>::</span> <span>Exception</span> <span>e</span> <span>=></span> <span>e</span> <span>-></span> <span>ExceptionContext</span> <span>-></span> <span>SomeException</span>  <span><span>--</span> new</span></pre></div>
<p dir="auto">But if we are passing the context as a constraint, then we could simply add it to the original method:</p>
<div dir="auto"><pre><span>toException</span> <span>::</span> (<span>HasExceptionContext</span>, <span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span>SomeException</span></pre></div>
<p dir="auto">There is no need for two methods this way.</p>
</li>
<li>
<p dir="auto"><input type="checkbox" id="m_2168953519220726455" disabled> <strong>Improvements to <code class="gmail-notranslate">throwIONoBacktrace</code></strong>. Currently the proposal defines <code class="gmail-notranslate">NoBacktrace</code> variants of <code class="gmail-notranslate">throw</code> and <code class="gmail-notranslate">throwIO</code>:</p>
<div dir="auto"><pre><span>throwNoBacktrace</span>   <span>::</span> <span>forall</span> <span>e</span> <span>a</span><span>.</span> (<span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span>a</span>
<span>throwIONoBacktrace</span> <span>::</span> <span>forall</span> <span>e</span> <span>a</span><span>.</span> (<span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span>a</span></pre></div>
<p dir="auto">The idea is to allow users to opt out of backtraces for non-exceptional control flow. But the problem is that this choice is not recorded anywhere in the exception, so when the exception is caught and rethrown, it will have unwanted backtraces added to it.</p>
<p dir="auto">One solution is to add a <code class="gmail-notranslate">Bool</code> flag to <code class="gmail-notranslate">ExceptionContext</code> to record the choice of not collecting the backtraces, so that they are not collected when the exception is rethrown. In fact, we could avoid the duplication of <code class="gmail-notranslate">throw</code> and <code class="gmail-notranslate">throwIO</code> this way:</p>
<div dir="auto"><pre><span>backtracesEnabled</span> <span>::</span> <span>HasExceptionContext</span> <span>=></span> <span><span>Bool</span></span>
<span>enableBacktraces</span>, <span>disableBacktraces</span> <span>::</span> (<span>HasExceptionContext</span> <span>=></span> <span>r</span>) <span>-></span> (<span>HasExceptionContext</span> <span>=></span> <span>r</span>)

<span>throw</span> <span>::</span> (<span>HasExceptionContext</span>, <span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span><span>IO</span></span> <span>a</span>
<span>throwIO</span> <span>::</span> (<span>HasExceptionContext</span>, <span>Exception</span> <span>e</span>) <span>=></span> <span>e</span> <span>-></span> <span><span>IO</span></span> <span>a</span></pre></div>
<p dir="auto"><code class="gmail-notranslate">throw</code> and <code class="gmail-notranslate">throwIO</code> can check for <code class="gmail-notranslate">backtracesEnabled</code> before calling <code class="gmail-notranslate">collectBacktraces</code>. The users would write something along the lines of:</p>
<div dir="auto"><pre>disableBacktraces <span>$</span>
  throwIO <span>MyControlFlowException</span></pre></div>
<p dir="auto">And the choice to disable backtraces would be carried along the exception in its context.</p>
</li>
</ol>
<p dir="auto">Hopefully, I have not missed anything. In the process of writing this review, I took a stab at rewriting the "Proposed Change Specification" section in accordance with all of the suggestions above, mainly to convince myself that the combination of those suggestions forms a coherent design. You can find it here: <a href="https://gist.github.com/int-index/750c6c292eb8266729adc61a5812a581" target="_blank">https://gist.github.com/int-index/750c6c292eb8266729adc61a5812a581</a>. If you agree with my comments, feel free to incorporate the updated specification (or parts of it) into the proposal.</p>
<p dir="auto">Thank you!</p>

<p style="font-size:small;color:rgb(102,102,102)">—<br>Reply to this email directly, <a href="https://github.com/ghc-proposals/ghc-proposals/pull/330#issuecomment-1330446106" target="_blank">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAEOY6ZVMSEXYJ6E3PRAMG3WKXOPHANCNFSM4M3U4KSA" target="_blank">unsubscribe</a>.<br>You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAEOY67DUVIM2F32AMP74DLWKXOPHA5CNFSM4M3U4KSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJ5GP6GQ.gif" alt="" width="1" height="1"><span style="color:transparent;font-size:0px;display:none;overflow:hidden;opacity:0;width:0px;height:0px;max-width:0px;max-height:0px">Message ID: <span><ghc-proposals/ghc-proposals/pull/330/c1330446106</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
</blockquote></div>