search

naomijub / edn-rs

[DEPRECATED]: Crate to parse and emit EDN
https://crates.io/crates/edn-rs
MIT License
81 stars 14 forks source link

EdnError rework #123

Closed Grinkers closed 7 months ago

Grinkers commented 9 months ago

Continuation of #120 with a rebase (should be a lot less noise). Currently a draft for #119

Grinkers commented 9 months ago

Oops, missed a couple tests that weren't run on my local setup. Fixed.

Please look/think about the new Error type and new usage. As it is right now, only Display and Debug are public. Eq and PartialEq are not. I'll probably make the error code pub too. I'll have to see how the walker impl ends up.

Obviously the TODOs are to do.

Grinkers commented 9 months ago

btw I started fuzzing with https://github.com/Grinkers/edn-rs_fuzzer/tree/main finding lots of things. Errors are also pretty annoying because I'll sometimes just end up with "ParseEdn" (not sure why on this one) or something at "index 2" but it's actually 500 characters deep.

Grinkers commented 9 months ago

Any other thoughts before doing the implementation?

I'd like to see #122 and #125 finished before progressing, as there's already conflicts.

Grinkers commented 9 months ago

While looking at clojure's read-string limitations for symbols I ran into https://github.com/naomijub/edn-rs/blob/bd558559f2b36c7c1c2ba3d24b38c15aaf0fcab5/tests/deserialize.rs#L803

(clojure.edn/read-string "5011227E71367421e12")
java.lang.NumberFormatException: Invalid number: 5011227E71367421e12

I'll be expanding my fuzzer for things that are not valid edn. Here's a couple patterns that it has found that we consider valid EDN but is not.

"#'clojure.generated.ns1/var1"
"#object[clojure.lang.Namespace 0x6dcc40f5 \\\"clojure.generated.ns1\\\"]"

I don't think these issues should block #125, as that's fixing valid edn issues. I think I'll rebase after 125, implement a walker with the most minimal version of EdnError as drafted here (while making sure there's no regressions), then start fuzzing non-valid edn too.

For the new ErrorCode enum, I think I'll loosely try to match the names Clojure's reader gives. https://github.com/clojure/clojure/blob/c6756a8bab137128c8119add29a25b0a88509900/src/jvm/clojure/lang/EdnReader.java