emacs-citar / citar

Emacs package to quickly find and act on bibliographic references, and edit org, markdown, and latex academic documents.
GNU General Public License v3.0
515 stars 55 forks source link

bibtex-actions-read returns (nil nil) when selecting item #126

Closed publicimageltd closed 3 years ago

publicimageltd commented 3 years ago

I have encountered a strange bug: Certain bibliography items seem to cause bibtex-actions-read to hickup. It returns (nil nil). Here's the .bib file:

@book{jorke_2003_Demokratie,
  title = {Demokratie als Erfahrung: John Dewey und die politische Philosophie der Gegenwart},
  shorttitle = {Demokratie als Erfahrung},
  author = {Jörke, Dirk},
  date = {2003},
  publisher = {{Westdt. Verl}},
  location = {{Wiesbaden}},
  url = {http://bvbr.bib-bvb.de:8991/F?func=service&doc_library=BVB01&doc_number=010344150&line_number=0001&func_code=DB_RECORDS&service_type=MEDIA},
  urldate = {2012-08-31},
  file = {/home/jv/Zotero/storage/3LUESP9S/Jörke - 2003 - Demokratie als Erfahrung John Dewey und die polit.pdf},
  langid = {german}
}

@article{joerissenAnthropologischeHinsichtenPragmatische2002,
  title = {Anthropologische Hinsichten, pragmatische Absichten},
  author = {Jörissen, Benjamin},
  date = {2002},
  pages = {24},
  file = {/home/jv/Zotero/storage/X7X3JUAD/Jörissen - 2002 - Anthropologische Hinsichten, pragmatische Absichte.pdf},
  langid = {german}
}

Selecting the first item yields the key (in a list); selecting the second item yields (nil nil).

Trying to debug that behavior, I encountered another weird error. I wanted to instrument the function with edebug. Edebug, however, said the defun was malformed. So I added a fake variable to the argument list:

(cl-defun bibtex-actions-read (&optional _fake_argument &key initial rebuild-cache)
  "Read bibtex-completion entries. ....")

Now debugging worked -- but the error was gone!

So it turned out that the error occurs only as long as I use the byte compiled version of the package. If I evaluate the function in the buffer directly, everything works fine. I get the error again if I explicitly force a reloading of the package with (load "bibtex-actions").

So my guess is that the problem has to do with compilation.

I use GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.27, cairo version 1.17.4) of 2021-03-26, and the latest bibtex-actions commit 149f9ae.

bdarcus commented 3 years ago

So my guess is that the problem has to do with compilation.

But no errors upon such compilation; right?

In the past, I sometimes have run into errors like that which comes down to the way completing-read-multiple parses candidates (it gets confused by trailing whitespace, and punctuation can throw it off, which is why the ampersand is the delimiter).

publicimageltd commented 3 years ago

But no errors upon such compilation; right?

No, the other way around: Error only with the compiled version, no error if I evaluate the defun directly.

bdarcus commented 3 years ago

Sorry; I meant ... there are no compile errors, or even warnings?

bdarcus commented 3 years ago

... but adding the dummy parameter fixes it anyway?

EDIT: and "certain bibliography items" trigger it?

What happens if you remove the url from the first one you posted? I don't think it will make any difference, but ...

publicimageltd commented 3 years ago

Deleting the URL doesn't change anything.

Adding the dummy parameter "fixes" it, simply because I then use the eval'd version instead of the byte compiled one.

Byte compiling yields no warning or whatever.

Maybe it's something with cl-defun? It looks like an Emacs bug to me. The hard way to check would be to rewrite the package without cl-defun and test it. I have no time for this the next days, however, and I am not suggesting you should do it.

But you should be able to reproduce the error:

  1. open emacs
  2. set the .bib file above as bibliography file for bibtex-completion
  3. bibtex-actions-refresh
  4. select "jörissen"
  5. nil nil
  6. go to the source
  7. re-eval bibtex-actions-read
  8. select "jörissen" again
  9. it works
  10. reload the compiled version (load-library "bibtex-actions")
  11. and back to 2.

I will try to reproduce the error with a clean version (-q) and report the results. Might take a while, though.

bdarcus commented 3 years ago

It looks like an Emacs bug to me.

Sure sounds like it.

I will try to reproduce the error with a clean version (-q) and report the results. Might take a while, though.

Thank you!

publicimageltd commented 3 years ago

It's the comma!

I was able to reproduce the error with a clean setup. I finally found the error: It's the "," in the title field. It makes completion-read-multiple think that I have entered multiple items, whereas in reality, I have not. Stupid little Emacs. I have no idea why the error goes away once I eval the defun directly (this behavior too was reproducible).

Here's what I did for debugging:

;; start with emacs -q -l 'filenameofthatsnippet.el'
(require 'package)
(package-initialize)
(require 'use-package)
(require 'quelpa-use-package)
(setq visible-bell nil
      ring-bell-function 'ignore)

(use-package bibtex-completion
  :config
  (setq bibtex-completion-bibliography   '("~/bibfile_causing_nil.bib"))
  (setq bibtex-completion-notes-path "~/Dokumente/Hefte/zettelkasten"))

(use-package bibtex-actions
  :bind
  (("C-x l" . bibtex-actions-open-notes)))

And I added some debug vars to bibtex-actions-read:

  (when-let ((crm-separator "\\s-*&\\s-*")
          (candidates (setq bibtex-actions--debug-candidates (bibtex-actions--get-candidates rebuild-cache)))
              (chosen
               (setq bibtex-actions--debug-chosen
                     (completing-read-multiple ......

Using this setup, I could provoke the error and look at the variables. If I choose the first title with the comma, bibtex-actions--debug-chosen is a list of two strings. This causes the cl-loop clause to produce (nil nil). If I choose the second title, it is a list with only one string -- the expected behavior.

bdarcus commented 3 years ago

Yeah, why I mentioned the CRM issue above and asked about the URL field, which has an &.

When you realize the CRM selections are one string, you can see how it can be brittle in certain circumstances.

But I don't see why the comma is a problem, given the crm-separator is the ampersand?

Indeed, @oantolin suggested the ampersand precisely to avoid that problem with these data (because you have to escape it in tex).

Is there something subtle wrong with that regexp, or with how I'm setting the local variable? E.g. is & not in fact being used as the crm-separator??

publicimageltd commented 3 years ago

The default value of crm-separator is "[ ]*,[ ]*". So it looks like the when-let doesn't override it. But I just caught the value of crm-separator in a global debug variable, like with the other let-bound values above, and it returned the modfied string with the ampersand. So it is overriden in this scope.

I can't explain it, but I have the feeling that somewhere in the depths of completions-read-multiple, something relies on dynamic scoping, or for some other reasons accesses the default instead of the overriden value. Which is why it works when I eval it manually in the file.

publicimageltd commented 3 years ago

Well, at least the source file for completing-read-multiple has no lexical scoping declaration in its top.

bdarcus commented 3 years ago

@oantolin @minad - do you have any insight into what @publicimageltd is hypothesizing just above here? Is this a crm bug? Something wrong with this code??

publicimageltd commented 3 years ago

Debugging in real time: If I set the variable crm-separator explicitly to the ampersand regexp usingdefvar, it works. So the problem is indeed that completing-read-multiple accesses the dynamically bound value for crm-separator, whereas bibtex-actions-read uses lexical-binding: t.

Problem being, of course, that using defvar, the global default is changed. But there must be a way to set a variably dynamically within a file with lexical binding enabled... or? And I vaguely remember having read that in Emacs 28, all files have be changed to use lexical binding.

minad commented 3 years ago

@bdarcus @publicimageltd Yes this can be. Are you requiring 'crm? Then you should get the defvar which makes the variable dynamically bound. Otherwise you can write a defvar declaration yourself, (defvar crm-separator). My recommendation is to use package-lint in combination with flycheck or flymake to catch many of such issues early on. In this case it would probably warn that crm-separator is lexically bound but unused.

bdarcus commented 3 years ago

I originally did that, but didn't understand why.

But when it went through the review at MELPA, they recommended removing it.

I'll add it back then.

So am I adding that specific crm-separator as a defvar at the top, and removing the let-bound one on the function, or am I only setting the defvar to nil (this is what I originally did).

bdarcus commented 3 years ago

Any chance you want to submit a PR @publicimageltd?

bdarcus commented 3 years ago

On this, @minad @publicimageltd:

My recommendation is to use package-lint in combination with flycheck or flymake to catch many of such issues early on. In this case it would probably warn that crm-separator is lexically bound but unused.

  1. I use doom emacs for elisp coding, which has all this setup, and does flag these sorts of warnings and errors for me. No such warnings or errors ATM.
  2. I have the elisp-check github action setup, which does the same checks, and flags unused lexical variables on PRs. I have a policy, for myself as well, of never merging a PR with even a warning.
  3. I haven't seen this bug, and @publicimageltd only sees is when he uses the byte-compiled package.

These three facts don't add up for me. Do they for you two?

bdarcus commented 3 years ago

@publicimageltd - #23 is the original PR I merged that I thought fixed the problem, and later removed.

publicimageltd commented 3 years ago

Declaring the variable fixes the issue. And it makes sense, since it is an issue with byte compilation, and that's the only thing changed by that declaration. I made a PR.

publicimageltd commented 3 years ago
3. I haven't seen this bug, and @publicimageltd only sees is when he uses the byte-compiled package.

Which Emacs version do you use? If it's a greater (how do you say? bigger?) version than 27.2, have a look at the source file for crm, and if it has a lexical binding declaration on the first line. As I said, I have read somewhere on emacs devel that all files have been converted to lexical binding lately for some more recent version of Emacs.

minad commented 3 years ago

@bdarcus Indeed, there is no warning in this case. The problem is that you used when-let here, which is not a good idea. when-let automatically uses the lexically bound variables since it expands to let+when. I suggest you only use when-let in case it is necessary to check the value.

bdarcus commented 3 years ago

Which Emacs version do you use?

I've mostly been using 28, but reverted to 27.2 recently because of an apparent bug that was bothering me.