Skip to content
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

Surprising behaviour of StandardVersion #848

Closed
trskop opened this issue Mar 9, 2019 · 5 comments
Closed

Surprising behaviour of StandardVersion #848

trskop opened this issue Mar 9, 2019 · 5 comments

Comments

@trskop
Copy link

trskop commented Mar 9, 2019

Latest release redefined StandardVersion as:

data StandardVersion
    = NoVersion
    | V_5_0_0
    | V_4_0_0
    | V_3_0_0
    | V_2_0_0
    | V_1_0_0
    deriving (Enum, Bounded)

defaultStandardVersion :: StandardVersion
defaultStandardVersion = NoVersion

Unfortunately, with this definition it is not possible to get latest supported version, of the standard, in a nice way. The dhall command fails victim to this as well:

⊢ dhall version
Haskell package version: 1.21.0
Standard version: none

My first reaction after seeing that there is a Bounded instance available was to use maxBound :: StandardVersion. Unfortunately that doesn't work as expected due to ordering of data constructors:

>>> renderStandardVersion (maxBound :: StandardVersion)
"1.0.0"

These are possible solutions that I came up with:

  1. Reorder data constructors to get expected behaviour from Enum and Bounded instances.
  2. Define Enum and Bounded instances manually.
  3. Introduce latestSupportedStandardVersion :: StandardVersion that would, at the moment, point to V_5_0_0.

I'm happy to help with the implementation as well.

@sellout
Copy link
Collaborator

sellout commented Mar 9, 2019

And what does NoVersion mean? Like, before the standard was versioned? That might be better called PreVersioning or something. Or does it mean that something doesn’t care about the version? That might be better modeled as Maybe StandardVersion.

@Gabriella439
Copy link
Collaborator

@sellout: The standard versions are used exclusively for the binary protocol. For a while they were used to tag encoded expressions but once dhall-lang/dhall-lang#362 was merged then the version was not included in the encoded expression. That's what NoVersion means (don't include in the encoded expression)

The reason for those constructors being in that order is that the Haskell implementation tries to support encoded expressions generated by previous versions of the standard. Since new standard versions going forward don't require a version tag for encoded expressions there won't be new standard versions added to that list (i.e. there won't be a V_6_0_0 or V_7_0_0 constructor).

That implies two things:

  • We should hide the StandardVersion from both the API and command-line flags. The StandardVersion type is now just an internal implementation detail of supporting older versions and users are not expected to explicitly select which standard version to use.

  • We should also correct the dhall version output to correctly report the matching standard version. As I noted above, the true standard version to use won't be any of the StandardVersion constructors. It will just be a separately recorded string that we'll need to remember to bump for each new release.

@trskop: Does that answer your question?

@trskop
Copy link
Author

trskop commented Mar 11, 2019

@Gabriel439, thank you for the explanation. I'm really excited about this since it'll simplify migration. (I was one of those that moaned about it in the survey.)
I have few questions regarding implementation:

  • Would something like this be acceptable?
    latestSupportedStandardVersion :: Text
    latestSupportedStandardVersion = "5.0.0.0"
  • There is perfectly usable data type Version in base, wouldn't it be better?
  • Where should this definition reside? Dhall.Binary doesn't seem right to me any more. One obvious candidate is Dhall.Core, but I'm not very sure about it. Maybe Dhall.Version?

@Gabriella439
Copy link
Collaborator

@trskop: Yes, baking in the standard version as either Text or a Version from base is a good solution to this. It could go in a separate Dhall.Version module like you suggested

trskop added a commit to trskop/dhall-haskell that referenced this issue Mar 13, 2019
trskop added a commit to trskop/dhall-haskell that referenced this issue Mar 13, 2019
Data type `StandardVersion` was used to tag binary representation of
previous versions of the Dhall Standard.  We still need to keep this
data type around to support older versions. Starting with version 6.0.0
of the standard this tag is no longer required for encoded expressions.

Dhall API uses `StandardVersion` in various places.  Instead of removing
it entirely from the public API this commit opted to hide data
constructors and expose only `defaultStandardVersion`.

Dhall command line tool allowed `StandardVersion` to be specified on the
command line. After this commit it is no longer possible to do so.

issue dhall-lang#848
@Gabriella439
Copy link
Collaborator

Closing this as the StandardVersion type was removed from the API in #2190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants