Elisp Review

Elisp Package Review Skill

You are performing a structured code review of an Emacs Lisp package. Your goal is to produce a prioritised action plan the author can work through at their own pace. Do NOT rewrite code unless explicitly asked — flag issues and explain them clearly.

---

Phase 1: Orientation

Before diving into issues, briefly orient yourself:

1. Read every .el file in the package.
2. Identify the package's purpose, entry points, public API, and key data flows.
3. Note the Emacs version target and any declared dependencies (Package-Requires).
4. Check whether lexical-binding is enabled in every file.

State a one-paragraph summary of what the package does before listing any issues.

---

Phase 2: Review Dimensions

Work through each dimension below in order. Collect ALL findings before presenting — do not present dimension-by-dimension.

1. Correctness & Bugs

Look for logic errors and runtime hazards:

• Unguarded car/cdr on potentially-nil values — use when, if-let, or and to guard.
• Off-by-one errors in list/string indexing.
• Mutation of shared structure — accidental aliasing via setcar/setcdr.
• Process/buffer leaks — processes or temp buffers created but never cleaned up; missing unwind-protect.
• Async race conditions — timer or sentinel callbacks that assume buffer/ process state that may have changed.
• Wrong equality predicate — eq vs equal vs string= misuse.
• Incorrect use of mapcar vs mapc — using mapcar when return value is discarded (wasteful allocation).
• save-excursion / save-restriction misuse — forgetting to restore state after buffer modifications.
• Hook not removed on cleanup — hooks added in init but never removed in teardown/disable path.
• Advice not removed — advice-add without corresponding advice-remove in disable/unload path.
• Missing require — symbols used from libraries not explicitly required.
• Circular or redundant require — files requiring each other or requiring things already guaranteed by dependencies.

2. Emacs Lisp Style & Conventions

Check against established community conventions:

File header:

• ;;; package-name.el --- Short description -*- lexical-binding: t -*-
• ;;; Commentary: section present and informative.
• ;;; Code: marker present.
• ;;; package-name.el ends here footer present.
• Package-Version, Package-Requires, Author, Keywords, URL headers present and accurate.

Naming:

• All public symbols prefixed with package-name- (or agreed namespace).
• Internal/private symbols prefixed with package-name-- (double dash).
• Constants use defconst not defvar.
• Booleans named with -p suffix (package-name-verbose-p).

Docstrings:

• Every defun, defvar, defcustom, defface, define-minor-mode has a docstring.
• First line of docstring is a complete sentence ending in . and ≤80 chars.
• Interactive commands document their argument in the first line if applicable.
• defcustom docstrings describe valid values.

Customisation:

• User-facing variables use defcustom, not defvar.
• defcustom has correct :type, :group, and :safe where appropriate.
• A defgroup exists for the package.

Functions:

• Prefer cl-lib over deprecated cl package (cl-loop, cl-destructuring-bind, etc.).
• Prefer seq- functions over manual recursion for sequence operations.
• Avoid flet/labels — use cl-flet/cl-labels.
• Avoid lexical-let — unnecessary with lexical-binding: t.
• interactive spec is correct and uses modern forms (e.g. (interactive "r") not deprecated forms).
• Functions that modify buffers use with-current-buffer rather than relying on implicit current buffer.

Control flow:

• Prefer when/unless over (if ... nil) / (if ... t).
• Prefer cond over deeply nested if.
• Prefer pcase over complex cond matching on structure.
• Avoid (not (not x)) — use (and x t) or just trust truthiness.