expez / edn.el

Read and write EDN from Elisp
MIT License
47 stars 4 forks source link

Doesn't work with peg 1.0 (?) #7

Closed FreekPaans closed 4 years ago

FreekPaans commented 5 years ago

Hi,

I recently had an issue with clj-refactor, I think due to an issue with edn.el. See my writeup here: https://github.com/clojure-emacs/clj-refactor.el/issues/441.

The gist of it was is that somehow my Emacs upgraded peg to 1.0, which seems to break edn.el. Invoking edn-read on valid edn somehow throws an exception:

(wrong-type-argument number-or-marker-p nil)
  peg-record-failure((set nil (9 32 10 44) nil))
  #f(compiled-function () #<bytecode 0x46c7c345>)()
  #f(compiled-function () #<bytecode 0x46c7c299>)()
  #f(compiled-function () #<bytecode 0x46c65a41>)()
  edn--read()
  edn--read-from-string(

I don't know how dependencies work in Emacs - I see that edn.el depends on peg 0.6, but I don't know what the rules are for using newer versions. But even if it's a mistake on my part, maybe it's useful for other people as well to have this documented.

expez commented 5 years ago

Hi, and thanks for the report!

peg.el is a very old library and the last version was released back in 2009. It works as it should and I wasn't really expecting another update. In fact, I can't find a version 1.0 now that I'm looking for it either.

If you do M-x find-library peg what sort of file do you see?

;;; peg.el --- Parsing Expression Grammars in Emacs Lisp
;;
;; Copyright 2008  Helmut Eller <eller.helmut@gmail.com>.
;;
;; Version: 0.6 (2009-Nov-04)
;; Package-Version: 20150708.641

This is the package header I see.

I don't know what the rules are for using newer versions

The rule is: give me at least this version :/ At least that was the rule previously. Maybe they've added some better support for explicit versions.

FreekPaans commented 5 years ago

Hi, this is what I get:

;;; peg.el --- Parsing Expression Grammars in Emacs Lisp  -*- lexical-binding:t -*-

;; Copyright (C) 2008-2019  Free Software Foundation, Inc.
;;
;; Author: Helmut Eller <eller.helmut@gmail.com>
;; Maintainer: Stefan Monnier <monnier@iro.umontreal.ca>
;; Package-Requires: ((emacs "25"))
;; Version: 1.0
;;
;; This program is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or

When I list-packages and then select the package:

peg is an installed package.

     Status: Installed in ‘peg-1.0/’.
    Version: 1.0
    Summary: Parsing Expression Grammars in Emacs Lisp
   Requires: emacs-25
Required by: edn-1.1.2
   Homepage: https://elpa.gnu.org/packages/peg.html
Other versions: 0.6 (installed), 1.0 (gnu), 0.6 (melpa-stable).

Parsing Expression Grammars (PEG) are a formalism in the spirit of

Seems it's on GNU: https://elpa.gnu.org/packages/peg.html

expez commented 5 years ago

Thanks @FreekPaans, I see that the new maintainer has made quite a few changes to the library.

@bbatsov Is there some way to peg to a specific package version in emacs now?

edn.el has been working flawlessly for years. Feels like a huge waste of time to re-write it because peg.el has been refactored after going untouched for 10 years :/

FreekPaans commented 5 years ago

"Is there some way to peg to a specific package version" 😂

Thanks for looking into this.

bbatsov commented 5 years ago

@bbatsov Is there some way to peg to a specific package version in emacs now?

@expez Unfortunately not. But users can specify to peg a package to a specific package archive and I see that PEG in MELPA hasn't been updated in 5 years, so that's one workaround.

There's also the newer https://github.com/clojure-emacs/parseedn that doesn't have any third-party parser deps, so I guess one alternative is to just use instead of edn.el. You might remember I was afraid to use edn.el in CIDER back in the day, because peg.el seemed like abandonware at the time . This eventually lead to the creation of parseedn.

bbatsov commented 5 years ago

Here's how pinning packages works in practice http://www.lonecpluspluscoder.com/2014/11/25/using-elpa-pinned-packages-gnu-emacs-24-4/

expez commented 5 years ago

You might remember I was afraid to use edn.el in CIDER back in the day, because peg.el seemed like abandonware at the time .

Funny how the opposite turned out to be the cause of problems. If nobody had decided to resurrect it everything would've kept on working fine.

Is parseedn in a state where we can just use that in clj-refactor instead? I noticed the build is currently failing.

If that's going to be maintained going forward, then I might as well retire edn.el.

bbatsov commented 5 years ago

Funny how the opposite turned out to be the cause of problems. If nobody had decided to resurrect it everything would've kept on working fine.

Yeah, there's definitely some irony in that. :D

Is parseedn in a state where we can just use that in clj-refactor instead? I noticed the build is currently failing.

Should be. I'm using to a limited extent in CIDER and it does the job. I'm juggling too many projects and I didn't have much time for it, but I definitely hope it's going to be our standard Elisp parser for EDN going forward (and as bonus it can also parse Clojure as well, so it can power some refactorings directly).

jsrodrigues commented 5 years ago

Hello,

For what it's worth, I was having a similar problem with the new peg-1.0 library. It was manifesting itself when I was attempting to refactor a symbol in my Clojure code using clj-refactor, I would get an error saying

void symbol peg-stack

From the following function in edn.el

;;; edn.el --- Support for reading and writing the edn data format from elisp

;; Author: Lars Andersen <expez@expez.com>
;; URL: https://www.github.com/expez/edn.el
;; Package-Version: 1.1.2
;; Keywords: edn clojure
;; Version: 1.1.2
;; Package-Requires: ((cl-lib "0.3") (emacs "24.1") (dash "2.10.0") (peg "0.6") (s "1.8.0"))

(defun edn--maybe-add-to-list ()
  (if (not discarded)
      (let ((v (pop peg-stack)))
        (push (cons v (pop peg--stack)) peg-stack))
    (setq discarded nil)
    ::dummy))

My peg version is:

;;; peg.el --- Parsing Expression Grammars in Emacs Lisp  -*- lexical-binding:t -*-

;; Copyright (C) 2008-2019  Free Software Foundation, Inc.
;;
;; Author: Helmut Eller <eller.helmut@gmail.com>
;; Maintainer: Stefan Monnier <monnier@iro.umontreal.ca>
;; Package-Requires: ((emacs "25"))
;; Version: 1.0
;;

The issue is that

peg-stack has been renamed to peg--stack in peg.el
(defvar peg--stack nil)

So, my workaround was to edit edn.el and rename peg-stack to peg--stack

(defun edn--maybe-add-to-list ()
  (if (not discarded)
      (let ((v (pop peg--stack)))
        (push (cons v (pop peg--stack)) peg--stack))
    (setq discarded nil)
    ::dummy))

This seems to have fixed the issue for me. Just wanted to report it here, so that others can get some direction if they face the same issue. Also, do we want to update edn.el to refer to the newly named variable in the peg package?

Thanks, John

expez commented 4 years ago

I think the way forward is to use parseedn in clj-refactor.

I don't want to spend my time tracking whatever changes show up in peg.el and I don't think bundling the old version of peg.el with this library is a good idea either.

JAremko commented 4 years ago
(require 'edn)

(define-obsolete-variable-alias 'peg-stack 'peg--stack "1.0")

works for me.

bbatsov commented 4 years ago

FYI - We've already migrated clj-refactor.el to parseedn. CIDER also uses parseedn internally.