Skip to content

Conversation

@sandydoo
Copy link
Contributor

@sandydoo sandydoo commented May 23, 2025

Updated to GHC 9.6.7 and also updated nixpkgs.

Tested shake and nix builds on: aarch64-darwin.

Fixes #819.

"expression": {
"tag": "FloatLiteral",
"value": 2
"value": 2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes the tests run by shake.

At a glance, this looks correct, but I'm not sure if something during the upgrade affected this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, my guess is that maybe this is something that changed in aeson 2.2 for default float deserializers? I'll have to review how the test suite works here... if this just changes the output, then I think this change is fine. If this json file is used as input, then I think ideally "2" should still be valid and be allowed to parse. Maybe the json serializer/deserializer now needs to be manually specified? Also I wonder if now aeson needs to be locked to >= 2.2 if using 2.1 vs 2.2 does actually change the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context. I'll try to narrow down what caused this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, if I cabal freeze aeson to 2.1.2.1 the tests still expect 2.0. Not really sure what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've figured it out. It has nothing to do with ghc, aeson, or elm-format.

The test-suite uses jq to pretty-print the json.

Running the following in a cabal repl on main:

import Data.Aeson
import qualified ElmFormat.AST.PublicAST as PublicAST
import qualified ElmFormat.Parse as Parse
import qualified Reporting.Result as Result
import ElmVersion

let Result.Result _ (Result.Ok mod)  = Parse.parse Elm_0_19 "float = [ 2.0 ]"
encode $ PublicAST.fromModule (PublicAST.Config { PublicAST.showSourceLocation = True }) mod

We get a FloatLiteral with a value of 2.0. This is what we expect to see from Aeson for a Double, but not what we're seeing in the test output.

"{\"moduleName\":\"Main\",\"imports\":{},\"body\":[{\"tag\":\"Definition\",\"name\":\"float\",\"parameters\":[],\"returnType\":null,\"expression\":{\"tag\":\"ListLiteral\",\"terms\":[{\"tag\":\"FloatLiteral\",\"value\":2.0,\"display\":{\"representation\":\"DecimalFloat\"},\"sourceLocation\":{\"start\":{\"line\":1,\"col\":11},\"end\":{\"line\":1,\"col\":14}}}],\"sourceLocation\":{\"start\":{\"line\":1,\"col\":9},\"end\":{\"line\":1,\"col\":16}}},\"sourceLocation\":{\"start\":{\"line\":1,\"col\":1},\"end\":{\"line\":1,\"col\":16}}}]}"

Running this through jq (v1.6 from main), we get:

{
  "moduleName": "Main",
  "imports": {},
  "body": [
    {
      "tag": "Definition",
      "name": "float",
      "parameters": [],
      "returnType": null,
      "expression": {
        "tag": "ListLiteral",
        "terms": [
          {
            "tag": "FloatLiteral",
            "value": 2,
            "display": {
              "representation": "DecimalFloat"
            },
            "sourceLocation": {
              "start": {
                "line": 1,
                "col": 11
              },
              "end": {
                "line": 1,
                "col": 14
              }
            }
          }
        ],
        "sourceLocation": {
          "start": {
            "line": 1,
            "col": 9
          },
          "end": {
            "line": 1,
            "col": 16
          }
        }
      },
      "sourceLocation": {
        "start": {
          "line": 1,
          "col": 1
        },
        "end": {
          "line": 1,
          "col": 16
        }
      }
    }
  ]
}

Would you look at that. A FloatLiteral with the value 2.

TLDR: jq's pretty-formatting stopped converting 2.0 to 2, so this just fixes the test suite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incredible detective work!!

@avh4
Copy link
Owner

avh4 commented May 28, 2025

Thanks for doing all this tedious work! Feel free to ping me again about this if I don't get a chance to merge it by this weekend.

@sandydoo
Copy link
Contributor Author

sandydoo commented Jun 4, 2025

@avh4 this is ready for merge.

The float serialization changes only affect the test suite. elm-format itself is not affected. See #829 (comment) for details.

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

Thanks! Looking good, just running through reviewing this now, pairing with @perkee and currently debugging the CI dependency deprecations.

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

Adding a couple follow-up commits via #830

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

FYI, didn't finish resolving CI issues today.

For the linux CI build, this commit is needed 8d34739

But there are issues with Windows CI build now due to haskell dependency mess :( https://github.com/avh4/elm-format/actions/runs/15452597789/job/43498079088?pr=830 It seems that the new version of process requires Win32-2.14, but CI is providing Win32-2.13 -- I don't really know where the Win32 comes from, I'm assuming probably it's packaged with ghc. @perkee and I took a look at locking process to a specific version (in the cabal.project file, but didn't have time to finish seeing that will solve the problem here.

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

The code changes in the PR look good though ✅ . And thanks for figuring out the jq formatting thing! Just gotta wrangle CI now. Feel free to help if you like, I will try to comment here when I am sitting down to actively continue work on that.

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

I think I figured it out. Waiting on https://github.com/avh4/elm-format/actions/runs/15454652951/job/43504407696?pr=831 and will likely merge via #831

@avh4 avh4 mentioned this pull request Jun 4, 2025
avh4 added a commit that referenced this pull request Jun 4, 2025
@avh4 avh4 merged commit 2b5f519 into avh4:main Jun 4, 2025
0 of 2 checks passed
@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

Yeah, there was an issue with Windows CI and newer versions of process, which maybe could have been solved by updating Win32 somehow, but I wasn't sure how or if that would be possible.

See 547175f I think @sandydoo must have run cabal freeze outside of nix-shell/direnv on this PR? If not, then I'm still a little confused about why I'm getting a different result from cabal freeze. I think I want cabal.project.freeze to reflect the dependency versions that are provided by the pinned nixpkgs -- sorry I didn't have that documented anywhere! Hopefully all the versions there (547175f) are good for your purposes.

@avh4
Copy link
Owner

avh4 commented Jun 4, 2025

Thanks for contributing this!

@sandydoo
Copy link
Contributor Author

sandydoo commented Jun 5, 2025

@avh4, I think @sandydoo must have run cabal freeze outside of nix-shell/direnv on this PR?

I used the nix shell, of course. I may have removed the local packages to get an initial shell working, as it was broken, and never added them back in. I generally avoid shellFor for this reason: it makes the development shell dependent on a working build of whatever you're working on, which isn't always possible.

I also think I bumped bytestring with a forced version constraint in 2b5f519. That was probably unnecessary.

That explains the version changes. Otherwise, running freeze with the local package deps in the shell removes the feature flags from the freeze for some reason.

Thanks for taking the time to fix CI!

@sandydoo sandydoo deleted the bump-ghc-967 branch June 5, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to ghc 9.6

3 participants