-
Notifications
You must be signed in to change notification settings - Fork 202
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
[DAML-LF] specification for generic comparison. #4942
Conversation
ad49df1
to
0db7157
Compare
The specification for contract id will be done in a separate PR. |
83c9468
to
35d266d
Compare
daml-lf/spec/daml-lf-1.rst
Outdated
@@ -1887,140 +1887,123 @@ will always be used to compare values of same types:: | |||
.. note:: the equality of generic map is not sensitive to the order of | |||
its entries. See rules ``'GenEqNonEmptyGenMap'``. | |||
|
|||
Value order | |||
Type order |
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.
You’re calling this section “Type order” and then you start by talking about numeric values. That doesn’t make sense to me.
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.
You are right, this is confusing.
This section is about only type ordering.
The main difference between type ordering and value ordering is that we need to specify the latter operationally to explain how it fails when encounter non comparable values. We do not need an operation specification for the former because the type checker will ensure that comparison of is always defined (the rules for typing 'to_any'
and 'type_rep'
ensure the type used do not contains variable, quanitifier not type synonyms).
Value ordering is define in the section Generic comparison functions
.
daml-lf/spec/daml-lf-1.rst
Outdated
``<ᵥ``. This relation is transitive, and antisymmetric with respect to ``~ᵥ``. | ||
It is a total order when comparing serialized values of the same type. | ||
Formally, ``<ₜ`` is defined as the least transitive relation | ||
on types that verifies the following rules:: |
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.
on types that verifies the following rules:: | |
on types that satisfies the following constraints:: |
daml-lf/spec/daml-lf-1.rst
Outdated
——————————————————————————————————————————————————— | ||
(PkgId:ModName₁):T₁ <ₜ (PkgId:ModName₂):T₂ | ||
|
||
T₁ < T₂ |
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.
Is this supposed to be lexicographic ordering on type constructor names? It’s not very clear to me.
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 replace by "comes lexicographically before"
.. note: In the above rules, map entries for TextMap and GenMap are ordered | ||
by key. The rules make this assumption explicit. | ||
——————————————————————————————————————————————————— | ||
⟨ f₁ : τ₁, …, fₘ : τₘ ⟩ <ₜ |
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 don’t understand this rule. Are you trying to say that adding extra fields makes it bigger? If so, I don’t understand why f_n
is at the end in both cases and what the point of f_m
is.
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.
It is better now ?
——————————————————————————————————————————————————— | ||
⟨ f₁ : τ₁, …, fₘ : τₘ ⟩ <ₜ τ σ | ||
|
||
τ₁ <ₜ τ₂ |
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.
Maybe I’m missing something but which rules allows me to deduce that Int64 < Numeric 10
?
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.
By transitivity (stated in the text) you have Int64 <ₜ τ σ
for all τ
and σ
.
I replace the text by an explicit rule.
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 don't see it @remyhaemmerle-da . There's no rule that lets you infer, for example, that τ₁ <ₜ τ₂ σ
whenever τ₁
is not a type application.
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.
Yeah I also cannot find it in the text.
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.
@associahedron
TypeOrderStructTyApp
allows you to infer trivially ⟨ ⟩ <ₜ τ₂ σ
@moritzkiefer-da Here is how I infer Int64 < Numeric 10
- Using
TypeOrderTransitivity
,TypeOrderInt64Date
, andTypeOrderDateTimestamp
you can infer that 'Int64' <ₜ 'Timestamp'. - Combining with
TypeOrderTransitivity
, andTypeOrderTimestampText
you can infer that'Int64' <ₜ 'Text'
... - Combining with
TypeOrderTransitivity
, andTypeOrderNatStruct
you can infer that'Int64' <ₜ ⟨ ⟩
- Combining with
TypeOrderTransitivity
andTypeOrderStructTyApp
you can infer that'Int64' <ₜ Numeric 10
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 do not like to define the rank explicitly (mapping types to integers)
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 think it would make the spec match the code more closely, and be easier to understand, which are both better for maintenance, but if you are sure about this way then I'm not opposed.
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 want to be able to introduce a new type between two already existing.
This will oblige me to have different ranks between two specs.
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.
That's fine. The type rank function is entirely formal, and would only exist in the spec, not be exposed in DAML/DAML-LF. Alternatively, you can define a formal enumeration of type ranks (instead of an type rank integer) and use the order from the enumeration.
Anyway, it is up to you.
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 did not include the rank.
+ update generic equality CHANGELOG_BEGIN CHANGELOG_END
35d266d
to
d0fc2a7
Compare
daml-lf/spec/daml-lf-1.rst
Outdated
⟨ f₀= v₀', f₁= v₁', …, fₘ= vₘ' ⟩) = Err t | ||
|
||
𝕆('LESS_EQ' @τ₁ v₀ v₀') = Ok 'True' | ||
𝕆('LESS_EQ' @τ₁ v₀' v₀) = Ok 'False' |
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.
This is fine for the spec, but if we were to actually implement LESS_EQ
this way it becomes an O(n^2) algorithm. The actual implementation should calculate the relative order of two items (less than, equal, or greater than) in a single step, rather than needing two boolean steps to make the comparison.
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.
This is indeed a spec. The implementation is based on compare
that return -1
, 0
or +1
.
This was a bitter cumbersome to use formally (use the cases 'less than', 'equal to', or 'greater than').
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.
Sounds good 👍
daml-lf/spec/daml-lf-1.rst
Outdated
⟨ f₀= v₀', f₁= v₁', …, fₘ= vₘ' ⟩) = Ok 'False' | ||
|
||
𝕆('LESS_EQ' @τ₁ v₀ v₀') = Ok 'True' | ||
𝕆('LESS_EQ' @τ₁ v₀' v₀) = Err t |
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.
Do you anticipate this situation being possible? I.e. v0 <= v0' is fine but v0' <= v0 leads to error?
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 don't. I though it was simpler to specify explicitly this error case, than to prove this never happen. This would indeed go away with the compare
function way. I will try once last time to go this way (I abandon twice already).
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.
IMO it's fine if you want to keep the boolean presentation, but change this rule into a note that says that
𝕆('LESS_EQ' @τ₁ v₀ v₀') = Ok 'True'
𝕆('LESS_EQ' @τ₁ v₀' v₀) = Err t
cannot happen, and that if it happens, that's a bug.
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 simply drop the rule. I gave up a third time trying to specify with the compare approach.
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.
Great job! I added some small remarks.
In this PR, we formalize the specification of the generic ordering implemented in #4808
This PR advances the state of #2256.
CHANGELOG_BEGIN
CHANGELOG_END
Pull Request Checklist
CHANGELOG_BEGIN
andCHANGELOG_END
tagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines run
totrigger the build.