Skip to content

Dead code to investigate #14733

Description

@cderv

Found this with 🤖 and saving for later so i investigate this more closely


Two guards in src/ have been permanently disabled since February 2021 because
an async predicate is called without await. !somePromise is always false,
so in both cases the guarded branch is unreachable.

Found while fixing the same class of bug in package/src/common/dependencies/
(#14731). These two are in src/ and, unlike the ones in that PR, each has real
behavioral consequences — so filing rather than folding them into that PR.

1. src/command/render/latexmk/pdf.ts:394 — Windows bibtex .aux fixup

if (isWindows) {
  if (bibCommand !== "biber" && !hasTexLive()) {
    // See https://github.com/rstudio/tinytex/blob/b2d1bae772f3f979e77fca9fb5efda05855b39d2/R/latex.R#L284
    // Strips the '.bib' from any match ...

hasTexLive is async (src/command/render/latexmk/texlive.ts:52), so
!hasTexLive() is always false. The \bibdata{...}.bib stripping has never
run. Affects Windows + natbib (i.e. bibCommand === "bibtex") renders where
tlmgr is not on PATH. Introduced 2021-02-16 (24e6757).

The enclosing makeBibliographyIntermediates is async, so await compiles —
but I don't think a bare await is the right fix here. The comment says
"If we're on windows and auto-install isn't enabled", while the code tests
hasTexLive(), not pkgMgr.autoInstall. Comment and condition have disagreed
for five years, and it isn't clear which reflects the intent. Awaiting as-is
would switch on .aux rewriting for every affected render after five years of
those users rendering without it — worth deciding deliberately, and worth
checking whether the upstream MiKTeX/bibtex behavior the tinytex workaround
targets is still current.

2. src/tools/impl/tinytex.ts:446uninstall() TinyTeX check

async function uninstall(context: InstallContext) {
  if (!isTinyTex()) {
    context.error("Current LateX installation does not appear to be TinyTex");
    return Promise.reject();
  }

isTinyTex is async (same file, line 636), so the guard never fires.
Introduced 2021-02-05 (fc1e03a). Note line 91 in the same file calls it
correctly (return await isTinyTex();), so this is an inconsistency within one
file.

Scope of the impact is narrower than it first looks: the removal step targets
tinyTexInstallDir(), a fixed path (~/.TinyTeX, ~/Library/TinyTeX,
%APPDATA%\TinyTeX), so a system TeX Live is never deleted. The real exposure is
the --update-path branch, which calls removePath(await texLiveContext(true))
tlmgr path remove against the detected installation. With a system TeX Live
present and no Quarto TinyTeX, that can strip the system install's symlinks
before failing with "Couldn't find install directory".

Again, a bare await may not be safe. It would activate a predicate that has
never executed, and isTinyTex() looks fragile for the default layout:

  • The directory-name check is
    root.match(/[/\\][Tt]iny[Tt]e[Xx][/\\]?/), which requires / or \
    immediately before TinyTeX. The Linux install dir is ~/.TinyTeX — a .
    intervenes, so this fails and the function depends entirely on the
    texmf-dist/web2c/fmtutil.cnf fallback.
  • texLiveRoot() derives the root with three .. on Windows but four
    elsewhere. That only reconciles if tlmgr resolves through a symlink to
    tlmgr.pl on non-Windows; I could not verify that without a real install.

If either misfires, awaiting turns a currently-working quarto uninstall tinytex
into one that refuses to run. The guard and its predicate probably want fixing
together, with testing on real installs on all three platforms.

Prevention

The four instances in #14731 compared the promise to undefined, which is
TS error 2367 — deno check over package/src would have caught them, and
nothing in CI runs it today.

These two are different: !promise is perfectly valid TypeScript, so no
type-check pass catches them. They need a lint rule — typescript-eslint's
no-misused-promises is the canonical one; I'm not sure deno lint has an
equivalent, which is worth checking before assuming CI can prevent recurrence.

Completeness

I scanned src/ and package/src/ for async functions called without await
in a boolean position, in two passes: direct use in a condition, and assignment
to a variable that is then tested. Six instances total — the four in #14731 plus
these two. Everything else flagged was a false positive (a sync function sharing
a name with an async one elsewhere).

Not built or test-run; the analysis above is from reading the code.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions