-
Notifications
You must be signed in to change notification settings - Fork 213
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
[ADP-3226] Use even more IsRecentEra
.
#4439
base: master
Are you sure you want to change the base?
Conversation
IsRecentEra
.
38d5969
to
d07d079
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see all applications @era
that do not involve recentEra @era
removed, and replaced by a type annotation on the result.
The type application for recentEra @era
is shorthand for recentEra :: RecentEra era
, which I find acceptable.
@@ -2882,7 +2882,7 @@ constructTransaction api argGenChange knownPools poolStatus apiWalletId body = d | |||
Nothing -> [] | |||
|
|||
unbalancedTx <- liftHandler $ | |||
W.constructTransaction @n era | |||
W.constructTransaction @n @era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
W.constructTransaction @n @era | |
W.constructTransaction @n |
The correct solution is to give a type annotation to the return value, here (unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era))
.
In this case, annotating the type is necessary to avoid ambiguity — at some point, we have to specify the era in which the transaction is being created, specifically that this era is specific to the result obtained from the PParams
.
See also the comment involving certificateFromDelegationAction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeinrichApfelmus If I've interpreted your suggestion correctly, this would result in a patch similar to:
- unbalancedTx <-
- liftHandler $
- W.constructTransaction @n @era
+ unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era) <-
+ liftHandler $
+ W.constructTransaction @n
db transactionCtx3
PreSelection { outputs = outs <> mintingOuts }
The correct solution is to give a type annotation to the return value, here (unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era)).
My apologies, I don't follow your reasoning here.
Could you explain why you believe that this solution (of using a type annotation) is more "correct" than the current solution (of using a type application)?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why you believe that this solution (of using a type annotation) is more "correct" than the current solution (of using a type application)?
Two reasons:
- The current solution uses a language extension
TypeApplications
. No reasoning has been given on how this language extension improves our code (compared to plainHaskell2010
) — the current solution has no strong basis. If you question a new solution, you have to question the status quo as well. - A type annotation always adds documentation, making it more clear which value has which type — the only reason not to add a type annotation is brevity. As a side effect, this type annotation also disambiguates. In contrast, the type application only disambiguates, but provides no additional benefit in terms of documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeinrichApfelmus thanks for your comment!
I'll try to reply to your points:
The current solution uses a language extension
TypeApplications
.
No reasoning has been given on how this language extension improves our code (compared to plain Haskell2010)
The proposed solution uses ScopedTypeVariables
, which AFAIK is also not part of Haskell2010
. We've used both of these extensions within our code base for several years.
Nevertheless, if no reasoning has been given for how TypeApplications
improves our code, then on what basis can we advocate for recentEra @era
over recentEra :: RecentEra era
, as advocated for in an earlier comment:
The type application for
recentEra @era
is shorthand forrecentEra :: RecentEra era
, which I find acceptable.
I'm guessing there are some circumstances in which you find this extension acceptable, and other circumstances where you find it to be less acceptable?
Additionally, I'm still wondering about my original question, which is about correctness:
Could you explain why you believe that this solution (of using a type annotation) is more "correct" than the current solution (of using a type application)?
I asked the above question in response to the following claim (see original comment):
The correct solution is to give a type annotation to the return value, here (unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era)).
Apologies in advance if I've misunderstood, but I would interpret the use of a definite article here ("the correct solution") as implying that only the proposed solution is correct (i.e., that the original solution is incorrect in some way).
But I haven't seen any arguments (as yet) for why the original solution is incorrect.
I accept there is some debate to be had about the relative ability of type annotations versus type applications to provide documentation, but it seems to me that these are two points on a design spectrum that can be considered on a case-by-case basis. It's certainly not clear to me that one is strictly better than the other. (Or else we would presumably not prefer recentEra @era
over recentEra :: RecentEra era
.)
However... 😄
I'd like to make a proposal to you:
-
We move any discussion about the relative merits of
TypeApplications
versusScopedTypeVariables
to another forum. While I do agree with you that this is a worthwhile discussion, I suspect that moving it to a separate forum would be more productive (and perhaps enlightening). Perhaps it could be a future development meeting topic? -
If we agree that this PR has already met its goals, then I would advocate that we merge this PR without further changes.
What do you think?
For the two remaining usages of @era
(outside of recentEra
), I would prefer to keep them as is, for the following reasons:
-
The current solutions are much more concise than the proposed alternatives. In particular, for the second example (the call to
constructUnbalancedSharedTransaction
), attempting to add a type annotation to the result leads to a very awkward multi-line tuple, which I find rather unreadable. Of course, there are several ways to ameliorate this, but I suspect these are all rather verbose in some way. -
The functions in question both already use
TypeApplications
, and in particular the@n
type application: adding an@era
type application does not increase the idiomatic burden in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed solution uses ScopedTypeVariables, which AFAIK is also not part of Haskell2010.
The type application @era
uses both ScopedTypeVariables
and TypeApplications
. The claim that the proposed solution uses an orthogonal extension is not correct — instead, the proposed solution uses strictly less extensions.
We've used both of these extensions within our code base for several years.
Appealing to habit is not sufficient reasoning for the use of TypeApplications
. Other people have smoked for several years.
Nevertheless, if no reasoning has been given for how TypeApplications improves our code, then on what basis can we advocate for recentEra @era over recentEra :: RecentEra era, as advocated for in an #4439 (review):
I personally prefer recentEra :: RecentEra era
, but in this particular instance I would accept the shorthand recentEra @era
. I am not advocating the use of the type application, but in this case, I would have no objection to it — as recentEra
is a special case, it works as documentation of the result type.
But I haven't seen any arguments (as yet) for why the original solution is incorrect.
I have presented an argument based on the notion of "types as documentation" that favors the proposed solution over the original solution. To summarize my argument why the original solution is incorrect:
- It uses two extensions
TypeApplications
andScopedTypeVariables
that are outside ofHaskell2010
. Of those two,ScopedTypeVariables
is more in line with the spirit ofHaskell2010
. - The "spirit of Haskell2010" is that types are light-weight and compiler-checked documentation. This suggests the the "correct" fix is to add a type annotation to
unsealedTx
.
- The current solutions are much more concise than the proposed alternatives.
… but it pays a price in terms of documentation. I believe that the type of unsealedTx
should be documented in the code.
But removing @era
type applications does reduce the idiomatic burden. This pull request adds a type application @era
where I put a value application era
previously.
If we agree that this PR has already met its goals,
I'm afraid, I do believe that the merit of this PR depends on the merits of TypeApplications
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of type- application vs annotation, my vote in this particular case (and similar ones) is to keep the value level RecentEra era
like on master
, as it makes the requirement to specify the era clear and transparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to discuss different points in the design space with you.
However, I do object to your labelling of my original solution as "incorrect". After reading your responses, it seems that this designation stems from your personal opinion rather than objective fact. While I really do empathise with your desire to find the best solution, I believe it's also important to be clear about what is objective and what is subjective.
Let's examine what you have written:
To summarize my argument why the original solution is incorrect:
- It uses two extensions
TypeApplications
andScopedTypeVariables
that are outside of Haskell2010. Of those two,ScopedTypeVariables
is more in line with the spirit of Haskell2010.- The "spirit of Haskell2010" is that types are light-weight and compiler-checked documentation.
Forgive me if I've misunderstood, but you seem to be implying that Haskell2010
(and the set of extensions it includes) represents some kind of ideal, one that people writing Haskell should pursue by default, and that justification should be given before using extensions not found in Haskell2010
. Is that a fair appraisal of your position?
I am willing to grant you that some Haskellers do prefer to restrict their code to Haskell2010
. (I have also encountered those that would prefer Haskell98
, for various reasons, such as compatibility with compilers other than GHC.) However, it's also true that this opinion is not universally held within the Haskell community at large. Evidence:
- In the most recent State of Haskell Survey, the question "Which language extensions would you like to be enabled by default?",
TypeApplications
was the one of the most highly-voted-for extensions (very few people voted against). - The
GHC2021
specification (admittedly, a GHC specification rather than a Haskell language standard per se), includes this extension by default, along with the qualifying statement:GHC blesses a number of extensions, beyond
Haskell2010
, to be suitable to turned on by default. These extensions are considered to be stable and conservative.)
Note the emphasis on conservative.
Of course, we could debate the merits of the above survey, and we could question whether the GHC development team made the right choice to include TypeApplications
in their GHC2021
specification. However, this would not change the fact that "Haskell2010
should be preferred":
- is a subjective opinion.
- is not an objective fact.
(As a side point, while I do acknowledge your argument about documentation, I disagree somewhat with the idea that type applications do not have documentation benefits. But let's agree to disagree on that point.)
My primary concern lies with the approach this review seems to take, one that seems to characterise subjective opinion as objective truth, as exemplified by the assertion that my original solution is "incorrect." I believe this approach is unproductive, and even harmful.
I believe it's unproductive, because the non-threaded format of GitHub comments makes it rather impractical to keep track of the sort of long and nuanced discussion that a decision on the utility of TypeApplications
would probably require. While we could have such a discussion, it was certainly not my goal in creating this PR.
As for the harmfulness of this approach: have you considered the difference between:
- "The correct solution is ... / the original solution is incorrect"
- "Thanks for making this PR! I see you have used solution X. Have you considered solution Y, for reasons Z?"
While the former approach may be suitable for objectively incorrect implementations, such as an implementation that violates a specification, I feel that in this context, where multiple design options exist with different trade-offs, the approach of labelling someone's solution as "incorrect" seems unnecessarily condescending, and actually hinders collaboration.
I feel that starting with the latter approach might lead to a more positive and fruitful review process, and possibly expedite the process of achieving consensus. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me if I've misunderstood, but you seem to be implying that
Haskell2010
(and the set of extensions it includes) represents some kind of ideal, one that people writing Haskell should pursue by default, and that justification should be given before using extensions not found inHaskell2010
. Is that a fair appraisal of your position?
In essence, yes, with the qualification that I use Haskell2010
as an umbrella term for the point in the design space that I would label "spirit of Haskell2010" and which I would summarize as: "The spirit of Haskell2010 is that types are light-weight and compiler-checked documentation for terms."
My primary concern lies with the approach this review seems to take, one that seems to characterise subjective opinion as objective truth, as exemplified by the assertion that my original solution is "incorrect." I believe this approach is unproductive, and even harmful.
My objection to TypeApplications
is on the grounds that it deviates too much from the point in the design space I tried to summarize above. Importantly, this is an objective criterion. Perhaps "incorrect" is too strong a word, but the deviation from the "spirit of Haskell2010" as defined above is factual, not opinion — it's "incorrect" in the sense of being outside of the "spirit of Haskell2010". (It's tautologically true that TypeApplications
is not part of Haskell2010
, the question is whether it's still within the point of the design space that I like to call "the spirit of Haskell2010" or outside.)
I contrast, I find that your arguments in favor of TypeApplications
err on the side of opinion. The result of a survey is pure opinion. The inclusion in a set of stable extensions in more objective, but only as far as the criterion of stability is concerned, not the effect of expressiveness on code.
The important questions for any extensions are:
- Which new forms of expression does the extension enable that significantly simplify code?
- Is this simplification worth the cognitive overhead?
On both counts, TypeApplications
does not favor well. Did you consider these two questions before using the extension? What were your conclusions?
@@ -3325,7 +3325,7 @@ constructSharedTransaction | |||
Just (ApiPaymentAddresses content) -> | |||
F.toList (addressAmountToTxOut <$> content) | |||
(unbalancedTx, scriptLookup) <- liftHandler $ | |||
W.constructUnbalancedSharedTransaction @n era | |||
W.constructUnbalancedSharedTransaction @n @era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
W.constructUnbalancedSharedTransaction @n @era | |
W.constructUnbalancedSharedTransaction @n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeinrichApfelmus If I've interpreted your comment correctly, you're proposing to replace this:
(unbalancedTx, scriptLookup) <- liftHandler $
W.constructUnbalancedSharedTransaction @n @era
db txCtx PreSelection {outputs = outs}
with (something like):
(unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era)
, scriptLookup
) <- liftHandler $
W.constructUnbalancedSharedTransaction @n
db txCtx PreSelection {outputs = outs}
resulting in a patch similar to:
- (unbalancedTx, scriptLookup) <-
+ (unbalancedTx :: Cardano.TxBody (Write.CardanoApiEra era)
+ , scriptLookup) <-
liftHandler $
- W.constructUnbalancedSharedTransaction @n @era
+ W.constructUnbalancedSharedTransaction @n
db txCtx PreSelection {outputs = outs}
Of course, there are probably nicer ways to format this (so that we don't need a multi-line tuple).
But I'm not sure what we gain by replacing the type application (on the function) with a type annotation (on the result) here. If anything, the result is certainly less concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same argument as for the previous comment.
The type annotation for unbalancedTx
adds documentation — in this case, telling us in which era
this unbalanced transaction is contructed (namely the one in scope). This information is important, as it also disambiguates the era — even the compiler complains about that.
In contrast, the type application does not add documentation, as one still has to inspect the type of constructUnbalancedSharedTransaction @n @era
in order to compute the type of unbalancedTx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast, the type application does not add documentation, as one still has to inspect the type of constructUnbalancedSharedTransaction @n @era in order to compute the type of unbalancedTx.
Thanks.
Though I disagree with the notion that the @era
type application adds no documentation.
When I read this code, I see the @era
type application as documentation of two facts:
- That the construction of a transaction is era-dependent.
- That we're constructing an (unbalanced) transaction for the
era
type that is currently in scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read this code, I see the @era type application as documentation of two facts:
- That the construction of a transaction is era-dependent.
- That we're constructing an (unbalanced) transaction for the era type that is currently in scope.
Both of these items depend on knowledge of the type of constructUnbalancedSharedTransaction
. In principle, the implementation of this function could choose to ignore the type argument.
In contrast, the type signature on unbalancedTx
provides compiler-checked documentation that the result of constructUnbalancedSharedTransaction
indeed satisfies these two facts. The facts being documentation are the same, but the type signature is superior in that
- it pertains to the result of
constructUnbalancedSharedTransaction
, not to the arguments
The point of the type signature on unbalancedTx
is that these two facts are both documented and compiler-checked, without the reader needing to consult the implementation or even the type of constructUnbalancedSharedTransaction
.
d07d079
to
642df6b
Compare
642df6b
to
55ac254
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for singletons and I'm not happy with the necessity of signing to disambiguate on the era. So I'm mostly out of the discussion 🤷
Issues
ADP-3226 (Follow-on from #4422)
Description
This PR adjusts even more functions to use an
IsRecentEra era
constraint instead of aRecentEra era
parameter.