search

quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.92k stars 322 forks source link

test failure with latest Pandoc #9296

Closed gordonwoodhull closed 7 months ago

gordonwoodhull commented 7 months ago

Bug description

Posting as an issue because this is a little too involved for Slack. I have only scratched the surface, don't know much about citeproc, and have not diagnosed whether this is a Quarto or Pandoc bug.

Hoping that someone else recognizes this problem.

Running against latest Pandoc 3.1.12.3 we have one failure in docs/smoke-all/typst/typst-no-citeproc.qmd

https://github.com/quarto-dev/quarto-cli/actions/runs/8591298602/job/23539917424#step:26:862

    [typst]: Compiling typst-no-citeproc.typ to typst-no-citeproc.pdf...
    error: array is empty
        ┌─ docs/smoke-all/typst/typst-no-citeproc.typ:102:15
        │
    102 │   let target = query(it.target, loc).first()
        │           
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This query() returns no results:

#show ref: it => locate(loc => {
  let target = query(it.target, loc).first()
  let suppl = it.at("supplement", default: none)
  if suppl == none or suppl == auto {
    it
    return
  }

The markdown that triggers the bug is 

Hello [@Cronbach_1951; @Cronbach_1952]

I.e. this double citation crashes but the single citation above it is okay.

Steps to reproduce

Set pandoc version to 3.1.12.3 and run the tests

Expected behavior

Test should pass and line should produce two linked citations in parentheses.

Actual behavior

Test failure quoted above.

Your environment

Quarto check output

Quarto 99.9.9

[✓] Checking versions of quarto binary dependencies...
      Pandoc version 3.1.12: OK
      Dart Sass version 1.70.0: OK
      Deno version 1.41.0: OK
      Typst version 0.11.0: OK

[✓] Checking versions of quarto dependencies......OK

[✓] Checking Quarto installation......OK
      Version: 99.9.9
      commit: 492f6574b9555caaf4849e33f45aec1d2b234be7
      Path: /Users/gordon/src/quarto-cli/package/dist/bin


[✓] Checking tools....................OK
      TinyTeX: v2024.03.13
      Chromium: (not installed)


[✓] Checking LaTeX....................OK
      Using: TinyTex
      Path: /Users/gordon/Library/TinyTeX/bin/universal-darwin
      Version: 2024

(|) Checking basic markdown render....
(/) Checking basic markdown render....
(-) Checking basic markdown render....
(\) Checking basic markdown render....
(|) Checking basic markdown render....
[✓] Checking basic markdown render....OK


[✓] Checking Python 3 installation....OK
      Version: 3.9.6
      Path: /Library/Developer/CommandLineTools/usr/bin/python3
      Jupyter: 5.7.1
      Kernels: python3

(|) Checking Jupyter engine render....
(/) Checking Jupyter engine render..../Users/gordon/Library/Python/3.9/lib/python/site-packages/urllib3/__init__.py:35: NotOpenSSLWarning: urllib3 v2 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'LibreSSL 2.8.3'. See: https://github.com/urllib3/urllib3/issues/3020
  warnings.warn(

(-) Checking Jupyter engine render....
(\) Checking Jupyter engine render....
(|) Checking Jupyter engine render....
(/) Checking Jupyter engine render....
(-) Checking Jupyter engine render....
(\) Checking Jupyter engine render....
(|) Checking Jupyter engine render....
(/) Checking Jupyter engine render....
(-) Checking Jupyter engine render....
(\) Checking Jupyter engine render....
(|) Checking Jupyter engine render....
(/) Checking Jupyter engine render....
(-) Checking Jupyter engine render....
(\) Checking Jupyter engine render....
(|) Checking Jupyter engine render....
(/) Checking Jupyter engine render....
(-) Checking Jupyter engine render....
[✓] Checking Jupyter engine render....OK

(|) Checking R installation...........
(/) Checking R installation...........
(-) Checking R installation...........
(\) Checking R installation...........
(|) Checking R installation...........
[✓] Checking R installation...........OK
      Version: 4.3.2
      Path: /Library/Frameworks/R.framework/Resources
      LibPaths:
        - /Users/gordon/src/quarto-cli/tests/renv/library/R-4.3/aarch64-apple-darwin20
        - /Users/gordon/Library/Caches/org.R-project.R/R/renv/sandbox/R-4.3/aarch64-apple-darwin20/ac5c2659
      knitr: 1.45.13
      rmarkdown: 2.26.1

(|) Checking Knitr engine render......
(/) Checking Knitr engine render......
(-) Checking Knitr engine render......
(\) Checking Knitr engine render......
(|) Checking Knitr engine render......
(/) Checking Knitr engine render......
(-) Checking Knitr engine render......
(\) Checking Knitr engine render......
(|) Checking Knitr engine render......
(/) Checking Knitr engine render......
[✓] Checking Knitr engine render......OK
cderv commented 7 months ago

I'll have a look at this.

cderv commented 7 months ago

This is a Pandoc 3.1.12 change - the intermediates .md gives 

Hello #cite(<Cronbach_1951>, form: "prose")

Hello @Cronbach_1951@Cronbach_1952

where we see the second line treating the double citations is not correct.

With 3.1.11.1 we get 

Hello #cite(<Cronbach_1951>)

Hello #cite(<Cronbach_1951>);#cite(<Cronbach_1952>)

I'll see if this is a bug in Pandoc or if support has changed for this syntax.

cderv commented 7 months ago

I think this is a pandoc issue, so I'll report to them.

The extensions +citations should apply and typst writer should correct handle the multiple item case. https://pandoc.org/MANUAL.html#extension-citations

cderv commented 7 months ago

Oh I was wrong... It changes for good reason because Typst supports @ syntax. Sorry I missed that the code you shared was ours... 🤦‍♂️ https://github.com/quarto-dev/quarto-cli/blob/b24caa36965240404d0bffcaca3b2235ebae45a3/src/resources/formats/typst/pandoc/quarto/definitions.typ#L102-L107

So we need to adapt to our template. I took this by the wrong side. sorry... 😓

gordonwoodhull commented 7 months ago

Thanks @cderv! Good to know!

It's not a new feature, right? Pandoc is just emitting more terse/idiomatic Typst for this case?

gordonwoodhull commented 7 months ago

Oh, I think there is a simple, obvious solution here. Will file PR.

cderv commented 7 months ago

The easy fix here is to move the target definition. 

diff --git a/tests/docs/smoke-all/2023/12/04/7784/subdir/index.typ b/tests/docs/smoke-all/2023/12/04/7784/subdir/index.typ
index 21eed240f..64e239c47 100644
--- a/tests/docs/smoke-all/2023/12/04/7784/subdir/index.typ       
+++ b/tests/docs/smoke-all/2023/12/04/7784/subdir/index.typ       
@@ -97,7 +97,6 @@
 }

 #show ref: it => locate(loc => {
-  let target = query(it.target, loc).first()
   if it.at("supplement", default: none) == none {
     it
     return
@@ -105,6 +104,7 @@

   let sup = it.supplement.text.matches(regex("^45127368-afa1-446a-820f-fc64c546b2c5%(.*)")).at(0, default: none)
   if sup != none {
+    let target = query(it.target, loc).first()
     let parent_id = sup.captures.first()
     let parent_figure = query(label(parent_id), loc).first()     
     let parent_location = parent_figure.location()

This is useful only for our own custom ref. this will solve this failure, but we should test the rest.

What I noticed is that locate() behavior is changing in Typst 0.11 and I think we need to adapt as it won't take a closure anymore in the future, following the introduction of Context

Also related to our usage of query which assumes Context

gordonwoodhull commented 7 months ago

Even simpler solution than the one I came up with. Will file PR.

Let's look into ramifications of these changes soon.