Skip to content

Conversation

@ChristopherRabotin
Copy link
Contributor

@ChristopherRabotin ChristopherRabotin commented Jul 17, 2025

This PR adds enum variants to simple types. The tests are separate from the enum function in serde.rs because these require a type annotation.

This is a zeroth implementation of #242 . A subsequent PR, if I can figure it out (likely with help), will enable recursive types.

This PR also runs clippy on the codebase, cleaning up some legacy implementation. This is a separate commit to make it easy to undo.

@Nadrieril
Copy link
Owner

Nadrieril commented Jul 19, 2025

Could you revert the Cargo.lock version update? dhall-rust supports rust 1.60 (I don't mind bumping if there's a good reason, but I don't see one right now). EDIT: did it myself

@Nadrieril
Copy link
Owner

Nadrieril commented Jul 19, 2025

Ah, it seems many of my dependencies have raised their MSRV (both Min and Max version it seems, even) :')

@ChristopherRabotin
Copy link
Contributor Author

Indeed, I'm seeing that the wasm crate requires a newer version of Rust: I'll check what's the MSRV I can get away with.

I'll check if StaticType supports the reflection, the manual building might be a relic of when I was trying the recursion.

Also remove unnecessary simple type definition
@ChristopherRabotin
Copy link
Contributor Author

WASM Bindgen MSRV is 1.76, which was released in February 2024, about fifteen months ago now.

@ChristopherRabotin
Copy link
Contributor Author

ChristopherRabotin commented Jul 20, 2025

I thought that the tests passed locally the other day, but now they don't anymore ... and the errors are not related to the changes I'm proposing here. (Edit: It's likely that I only tested the enum tests actually)

When running the tests are the root of the crate:

thread 'syntax::text::parser::test_grammar_files_in_sync' panicked at dhall/src/syntax/text/parser.rs:1056:9:
The local dhall.abnf file differs from the one from dhall-lang!

When running from the serde_dhall folder:

failures:

---- serde::test_file stdout ----

thread 'serde::test_file' panicked at serde_dhall/tests/serde.rs:290:9:
assertion `left == right` failed
  left: Err("No such file or directory (os error 2)")
 right: Ok(true)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- serde::test_import stdout ----

thread 'serde::test_import' panicked at serde_dhall/tests/serde.rs:12:9:
assertion `left == right` failed
  left: Err("Type error: error: error\n --> <current file>:1:1\n  |\n1 | ../dhall-lang/tests/parser/success/unit/BoolLitTrueA.dhall\n  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ No such file or directory (os error 2)\n  |")
 right: Ok(true)


failures:
    serde::test_file
    serde::test_import

Any idea how I could fix these? Or do these only work on Github for some reason?

I'll make another commit now that runs clippy on the other crates if it's OK. Since these changes are in their own commit, you should be able to trivially revert them if you prefer.

@Nadrieril
Copy link
Owner

Could you make a separate PR for the clippy fixes? Regarding the dhall.abnf error, did you maybe change the dhall-lang submodule to another commit than the one in version control?

@Nadrieril
Copy link
Owner

Btw a MSRV change is not a minor change, we should bump to 0.13. I've cherry-picked those commits to #245 so we can get things working again before we try to add any new feature.

@ChristopherRabotin
Copy link
Contributor Author

ChristopherRabotin commented Jul 23, 2025

Sure thing! I've updated the version to 0.13.0 in the three different files that require the change, and undid the clippy changes. However, in updating the version, there will necessarily be a change in the Cargo.lock file.

diff --git a/Cargo.lock b/Cargo.lock
index 7b6794d..c341d3c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1,6 +1,6 @@
 # This file is automatically @generated by Cargo.
 # It is not intended for manual editing.
-version = 3
+version = 4

 [[package]]
 name = "abnf"
@@ -1154,7 +1154,7 @@ dependencies = [

 [[package]]
 name = "serde_dhall"
-version = "0.12.2"
+version = "0.13.0"
 dependencies = [
  "dhall",
  "dhall_proc_macros",
lines 1-21/21 (END)

Concerning the submodule, I had not initialized it, so I don't think I had modified the commit it points to. I'm not entirely sure what is happening there.

Edit: When fetching for your repo (instead of my fork), git reports that the referenced commit on dhall-lang does not exist:

Fetching submodule dhall-lang
fatal: remote error: upload-pack: not our ref bf9783fc4298d5d54897af1631d677b05dd19db5
Errors during submodule fetch:
	dhall-lang

@ChristopherRabotin
Copy link
Contributor Author

I'll merge #245 into this branch to avoid conflicts with the objective of merging 245 before this PR.

@Nadrieril
Copy link
Owner

I did the changelog and version updates myself; could you rebase on top of master?

@Nadrieril
Copy link
Owner

Regarding dhall I'm on 204a9d9dd167d2c9038539148a09825ded62f1b8, how did you get that other commit?

@ChristopherRabotin
Copy link
Contributor Author

Sorry, I got pretty confused trying to undo the clippy changes, and with the various merges. So I cherry picked the commit that implements the enum struct variants onto its own branch here: #246 .

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants