Add property declarations to several functions#175
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds (declare ...) annotations across many helper functions, refreshes file Time-stamp headers, extends pel-elcode to detect non-local variable reads and allocating operators, and adds a CLI linter plus Makefile validate-declarations target. ChangesDeclare annotations, pel-elcode analysis, and lint tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai The command |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find and display pel-elcode.el
fd -e el -i 'pel-elcode' --exec cat {}Length of output: 63568 🏁 Script executed: #!/bin/bash
# Look for make-hash-table property declarations in Emacs byte-opt / subr files
# in case Emacs ships with a copy we can inspect
find /usr/share/emacs -name "byte-opt.el" 2>/dev/null | head -5
find /usr/share/emacs -name "subr.el" 2>/dev/null | head -5
# Check whether make-hash-table is declared side-effect-free in Emacs byte-opt
find /usr/share/emacs -name "byte-opt.el" 2>/dev/null -exec grep -n "make-hash-table" {} +
find /usr/share/emacs -name "byte-opt.el" 2>/dev/null -exec grep -n "side-effect-free" {} + | head -40
# Also check the Emacs version present
emacs --version 2>/dev/null | head -1Length of output: 859 🌐 Web query:
Length of output: 17431
Root Cause:
|
| Function | Allocates |
|---|---|
make-hash-table |
new hash table |
make-vector |
new vector |
make-list / list |
new list cells |
make-string |
new string |
make-symbol / gensym |
new uninterned symbol |
make-bool-vector |
new bool-vector |
make-char-table |
new char-table |
make-record / record |
new record |
make-keymap / make-sparse-keymap |
new keymap |
make-byte-code |
new compiled function |
copy-hash-table / copy-alist / copy-sequence / copy-tree |
new copy |
cons / vector / append |
new structure |
The Fix
Add a new constant listing the allocating functions that pel-elcode should treat as not side-effect-free, and check it in pel-elcode-properties-of-sexp before calling function-get:
(defconst pel-elcode-allocating-operators
'(;; Hash tables
make-hash-table copy-hash-table
;; Sequences / vectors / lists
make-vector make-list make-bool-vector make-char-table
cons list append reverse copy-sequence copy-tree copy-alist
vector vconcat
;; Strings
make-string concat format format-message propertize
;; Symbols
make-symbol gensym
;; Records / byte-code objects
make-record record make-byte-code
;; Keymaps
make-keymap make-sparse-keymap)
"Functions that allocate new objects.
Despite Emacs declaring these as `side-effect-free' (meaning their
result may be safely discarded), a function whose body calls any of
these cannot be declared `side-effect-free' by `pel-elcode' because
each call produces a fresh, non-`eq' heap object.")Then, in pel-elcode-properties-of-sexp, add a pre-check before the pcase:
(dolist (op operators)
(unless (function-get op 'pure)
(setq defun-props (delq 'pure defun-props)))
;; Allocation functions are declared side-effect-free by Emacs for
;; byte-compiler purposes, but pel-elcode treats them as impure
;; because they produce a new heap object on every call.
(if (memq op pel-elcode-allocating-operators)
(setq defun-props (pel-delqs '(side-effect-free error-free) defun-props))
(pcase (function-get op 'side-effect-free)
('error-free)
('t (setq defun-props (delq 'error-free defun-props)))
(_ (setq defun-props (pel-delqs '(side-effect-free error-free)
defun-props)))))
(unless defun-props
(throw 'pel-elcode-break nil)))This ensures that any operator in pel-elcode-allocating-operators immediately strips side-effect-free and error-free, overriding whatever Emacs's function-get would have returned.
Would you like me to open a GitHub issue to track this, or would you prefer to plan the full code change now?
🧠 Learnings used
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-05-15T13:12:54.900Z
Learning: In pel-package.el (pierre-rouleau/pel), the `pel--show-pkgs-in-excess-for` function always returns 0 because the local variable `n` is initialized but never incremented. The actual list insertion is delegated to `pel--insert-pkgs-list`. This is a bug: callers expecting an excess package count receive 0 always.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-05-18T14:02:09.141Z
Learning: In pel-ibuffer.el (pierre-rouleau/pel), the function `pel--get-all-buffer-directories` (around line 64-72) has an unsafe `(string= (car lst) e)` comparison. On the first dolist iteration `lst` is nil, so `(car nil)` returns nil and `(string= nil e)` signals `(wrong-type-argument stringp nil)`. The fix is `(equal (car lst) e)` which handles nil cleanly.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T19:01:21.066Z
Learning: In PEL (pierre-rouleau/pel), avoid proposing `scan-sexps` for compatibility-sensitive code because PEL supports Emacs 26.3 and `scan-sexps` is not available there per project guidance. Use older-compatible alternatives such as `(save-excursion (goto-char start) (forward-sexp 1) (point))` wrapped in `condition-case` when computing the end position of a form.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-05-13T16:21:52.484Z
Learning: In the PEL Emacs package (pierre-rouleau/pel), `pel-*-mode` dispatcher functions (e.g. `pel-dart-mode`, `pel-rust-mode`) intentionally have no fallback/`t` branch for when `pel-use-LANG` is `nil`. When `pel-use-LANG` is `nil`, PEL never registers the dispatcher in `auto-mode-alist` (the step-2 association in `pel_keys.el` is guarded by the customization variable), so the dispatcher is never called. Do not flag the absence of a nil/fallback branch in these dispatchers as a bug — it is correct by design.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T19:43:35.828Z
Learning: In PEL (pierre-rouleau/pel), `scan-sexps` may be used in compatibility-sensitive Emacs Lisp code despite some documentation suggesting it is unavailable in Emacs 26.3; the maintainer verified directly inside Emacs 26.3 that `scan-sexps` is available. Do not flag `scan-sexps` usage as incompatible with PEL's Emacs 26.3 support solely on that documentation claim.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-05-19T19:29:22.508Z
Learning: In PEL (pierre-rouleau/pel), `pel_keys.el` intentionally uses `(declare-function <fn-name> "pel_keys")` within the same file for functions whose `defun` is placed inside a conditional form (e.g., `(when pel-use-<feature> ...)`). This is required — not redundant — because some Emacs versions' byte-compiler cannot detect the `defun` in all code paths when it is not at top-level scope, and will emit "unknown function" warnings at call sites. Multiple such `declare-function` calls appear throughout `pel_keys.el` for exactly this reason. Never flag these as redundant in code review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pel-skels.el (1)
379-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
(pure t)frompel-lisp-family-major-mode-p
pel-lisp-family-major-mode-pdepends onmajor-mode(buffer-local mutable state), so it cannot satisfy Emacs Lisp’s(declare (pure t))referential-transparency contract; keep only(side-effect-free t).Proposed fix
(defun pel-lisp-family-major-mode-p () "Return t if the current major mode is for a Lisp-family language. Return nil otherwise." - (declare (pure t) (side-effect-free t)) + (declare (side-effect-free t)) (memq major-mode '(lisp-mode emacs-lisp-mode lfe-mode clojure-mode hy-mode scheme-mode racket-mode)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pel-skels.el` around lines 379 - 386, The function pel-lisp-family-major-mode-p declares (pure t) but reads the buffer-local variable major-mode, so remove the (pure t) clause and leave only (side-effect-free t) in the (declare ...) form inside pel-lisp-family-major-mode-p; update the (declare (pure t) (side-effect-free t)) occurrence to just (declare (side-effect-free t)) to reflect the correct contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pel-elisp-eval.el`:
- Line 80: The function pel--eval-buffer-binding-type is declared as (declare
(pure t) (side-effect-free error-free)) but its return depends on the current
buffer’s lexical-binding, so remove the pure annotation while keeping
side-effect-free and error-free; update the declare form inside
pel--eval-buffer-binding-type to drop (pure t) and leave (side-effect-free
error-free) so the function still signals no side effects or errors but is not
marked pure.
In `@pel-erlang.el`:
- Line 1155: The function pel-erlang-stop-when-arg-used-p wrongly declares (pure
t) even though it reads current-prefix-arg; remove the pure declaration from the
(declare ...) form in pel-erlang-stop-when-arg-used-p and keep the remaining
(side-effect-free error-free) declarations unchanged so the function no longer
claims purity.
In `@pel-skels-erlang.el`:
- Around line 340-356: Remove the incorrect (pure t) declaration from the
predicate functions that read user-customizable variables:
pel-erlang-skel-edoc-in-function-p, pel-erlang-skel-prompt-for-file-purpose-p,
and pel-erlang-skel-prompt-for-function-purpose-p; leave the (side-effect-free
t) declaration (and any other non-pure qualifiers) intact so these functions are
no longer marked pure while still documenting they have no side effects, and
ensure the functions still reference pel-erlang-skel-with-edoc and
pel-erlang-skel-prompt-for-purpose as before.
---
Outside diff comments:
In `@pel-skels.el`:
- Around line 379-386: The function pel-lisp-family-major-mode-p declares (pure
t) but reads the buffer-local variable major-mode, so remove the (pure t) clause
and leave only (side-effect-free t) in the (declare ...) form inside
pel-lisp-family-major-mode-p; update the (declare (pure t) (side-effect-free t))
occurrence to just (declare (side-effect-free t)) to reflect the correct
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10b61cf5-38ae-417c-b687-9a192eeee26f
📒 Files selected for processing (21)
pel--keys-macros.elpel-autocomplete.elpel-c-utils.elpel-completion.elpel-elisp-eval.elpel-erlang.elpel-ert.elpel-face-ut.elpel-filedir.elpel-hash.elpel-search.elpel-setup.elpel-skels-c.elpel-skels-cpp.elpel-skels-erlang.elpel-skels.elpel-spell.elpel-syntax.elpel-time.elpel-xref.elpel-yang.el
Owner
Author
|
@coderabbitai The latest code fails to build on Ubuntu-latest with Emacs 27.1 and fails with the following error: The build was passing before. It passes currently on my system with Emacs 30.2. I suspect the change with the declaration added to Please analyze and report the reason why the code fails and if possible propose a code change to prevent the byte compilation error. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== pel-elisp-eval.el lines 70-120 ==="
fd -e el "pel-elisp-eval" --exec sed -n '70,120p' {}
echo ""
echo "=== Full pel-elisp-eval.el ==="
fd -e el "pel-elisp-eval" --exec cat {}Length of output: 28272 ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
Root Cause:
|
Owner
Author
|
@coderabbitai After the last change the following compilation error occurs on Emacs 26.1, proabably for a similar reason: Please analyze and identify the root cause for this error. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== pel--available-completion-mode-mask definition and declare ==="
rg -n "pel--available-completion-mode-mask" pel-completion.el -C5
echo ""
echo "=== Lines 700-730 of pel-completion.el ==="
sed -n '700,730p' pel-completion.el
echo ""
echo "=== All declare forms in pel-completion.el ==="
rg -n "declare" pel-completion.el -C2Length of output: 4619 ✏️ Learnings added
🧠 Learnings used |
Owner
Author
|
@coderabbitai With the proposed changes to pel-elcode.el in the analysis of functions, the following funciton is no longer found to be Why is that? |
|
Tip For best results, initiate chat on the files or code changes.
(if (memq op pel-elcode-allocating-operators)
(setq defun-props (pel-delqs '(side-effect-free error-free) defun-props)) ; ← too aggressiveThis is too aggressive. The Emacs The distinction:
For
The proposed allocating-operators branch should only strip (if (memq op pel-elcode-allocating-operators)
- (setq defun-props (pel-delqs '(side-effect-free error-free) defun-props))
+ (setq defun-props (delq 'pure defun-props)) ; allocation prevents pure, not side-effect-free
(pcase (function-get op 'side-effect-free)
...))With this correction:
🧠 Learnings used |
Owner
Author
|
@coderabbitai The Please propose code modification to handle this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate pel-elcode.el ==="
fd -e el -i '^pel-elcode\.el$'
echo
echo "=== pel-elcode-properties-of-sexp and surrounding code ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "defun pel-elcode-properties-of-sexp|defun pel-elcode-operators-in|defconst pel-elcode|defvar pel-elcode|defcustom pel-elcode" "$file" -C 8
' sh {}
echo
echo "=== All property/operator helper definitions in pel-elcode.el ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "^(\\(defun|defconst|defvar|defcustom) pel-elcode" "$file"
' sh {}
echo
echo "=== Full relevant middle section (approx lines 1-260) ==="
fd -e el -i '^pel-elcode\.el$' --exec sed -n '1,260p' {}
echo
echo "=== Later relevant section (approx lines 260-520) ==="
fd -e el -i '^pel-elcode\.el$' --exec sed -n '260,520p' {}Length of output: 21938 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate pel-elcode.el ==="
fd -e el -i '^pel-elcode\.el$'
echo
echo "=== pel-elcode-properties-of-sexp and surrounding code ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "defun pel-elcode-properties-of-sexp|defun pel-elcode-operators-in|defconst pel-elcode|defvar pel-elcode|defcustom pel-elcode" "$file" -C 8
' sh {}
echo
echo "=== All property/operator helper definitions in pel-elcode.el ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "^(\\(defun|defconst|defvar|defcustom) pel-elcode" "$file"
' sh {}
echo
echo "=== Full relevant middle section (approx lines 1-260) ==="
fd -e el -i '^pel-elcode\.el$' --exec sed -n '1,260p' {}
echo
echo "=== Later relevant section (approx lines 260-520) ==="
fd -e el -i '^pel-elcode\.el$' --exec sed -n '260,520p' {}Length of output: 21938
(defun foo ()
(if pel-use-ido "ido" "other"))Even if I would not special-case only user-options. A user-option is one example of a non-local variable read. Any unquoted free/global variable read should remove Proposed modificationAdd a second analysis pass that collects non-local variables read by the sexp, then make diff --git a/pel-elcode.el b/pel-elcode.el
index 0000000..0000000 100644
--- a/pel-elcode.el
+++ b/pel-elcode.el
@@ -108,6 +108,151 @@ Take NEW-LOCAL-VARS local variables into account."
`(dolist (item ,body)
(pel-elcode--add-ops-from-list item ,var ,new-local-vars)))
+(defun pel-elcode--variable-read-symbol-p (symbol local-vars)
+ "Return non-nil if SYMBOL denotes a non-local variable read.
+
+Ignore locally bound variables, nil, t and keywords."
+ (and (symbolp symbol)
+ (not (memq symbol local-vars))
+ (not (memq symbol '(nil t)))
+ (not (keywordp symbol))))
+
+(defmacro pel-elcode--add-vars-from-list (items var new-local-vars)
+ "Add variable reads present in ITEMS list to VAR.
+Take NEW-LOCAL-VARS local variables into account."
+ `(setq ,var
+ (append (reverse (pel-elcode-variables-read-in
+ ,items ,new-local-vars))
+ ,var)))
+
+(defmacro pel-elcode--add-vars-from-lists (body var new-local-vars)
+ "Add variable reads present in BODY list to VAR.
+Take NEW-LOCAL-VARS local variables into account."
+ `(dolist (item ,body)
+ (pel-elcode--add-vars-from-list item ,var ,new-local-vars)))
+
+(defun pel-elcode-variables-read-in (exp &optional local-vars)
+ "Return non-local variable symbols read by EXP.
+
+LOCAL-VARS is a list of symbols locally bound in the current lexical
+scope. Variable symbols found in quoted forms, function forms,
+lambda/defun arglists, binding positions and `setq' target positions
+are ignored.
+
+The returned list preserves first-read order and contains no duplicates."
+ (let ((symbols ()))
+ (cond
+
+ ;; A plain symbol in value position is a variable read unless it is local
+ ;; or self-evaluating.
+ ((pel-elcode--variable-read-symbol-p exp local-vars)
+ (push exp symbols))
+
+ ((and (consp exp) (symbolp (car exp)))
+ (let ((head (car exp))
+ (body (cdr exp)))
+
+ ;; Keep macro handling consistent with `pel-elcode-operators-in'.
+ (when (and (macrop head)
+ (not (memq head pel-elcode-structural-forms))
+ (not (memq head pel-elcode-non-impacting-operators)))
+ (let ((expanded (macroexpand-1 exp)))
+ (unless (equal expanded exp)
+ (setq exp expanded
+ head (car-safe exp)
+ body (cdr-safe exp)))))
+
+ (cond
+
+ ;; (defun NAME (ARGS) [DOCSTRING] BODY...)
+ ;; Skip NAME and ARGS. BODY sees ARGS as locals.
+ ((memq head '(defun defsubst))
+ (let ((locvars (append (pel-elcode--args-in (cadr body))
+ local-vars)))
+ (pel-elcode--add-vars-from-lists (cddr body) symbols locvars)))
+
+ ;; (lambda (ARGS) BODY...)
+ ;; Skip ARGS. BODY sees ARGS as locals.
+ ((eq head 'lambda)
+ (let ((locvars (append (pel-elcode--args-in (car body))
+ local-vars)))
+ (pel-elcode--add-vars-from-lists (cdr body) symbols locvars)))
+
+ ;; (let ((VAR VAL) ...) BODY...)
+ ;; VAL forms see the outer scope. BODY sees all VARs as locals.
+ ((eq head 'let)
+ (let* ((bindings (car body))
+ (vars (delq nil
+ (mapcar (lambda (b)
+ (cond
+ ((symbolp b) b)
+ ((symbolp (car-safe b)) (car b))))
+ bindings)))
+ (vals (delq nil
+ (mapcar (lambda (b)
+ (when (consp b) (cadr b)))
+ bindings)))
+ (locals (append vars local-vars)))
+ (pel-elcode--add-vars-from-lists vals symbols local-vars)
+ (pel-elcode--add-vars-from-lists (cdr body) symbols locals)))
+
+ ;; (let* ((VAR VAL) ...) BODY...)
+ ;; Each VAL sees the variables bound by previous bindings.
+ ((eq head 'let*)
+ (let ((running-locals local-vars))
+ (dolist (binding (car body))
+ (if (consp binding)
+ (progn
+ (setq symbols
+ (append (reverse (pel-elcode-variables-read-in
+ (cadr binding) running-locals))
+ symbols))
+ (when (symbolp (car binding))
+ (push (car binding) running-locals)))
+ (when (symbolp binding)
+ (push binding running-locals))))
+ (pel-elcode--add-vars-from-lists (cdr body)
+ symbols
+ running-locals)))
+
+ ;; (dolist (VAR LIST [RESULT]) BODY...)
+ ;; LIST sees the outer scope. RESULT and BODY see VAR as local.
+ ((eq head 'dolist)
+ (let* ((var-spec (car body))
+ (var (car var-spec))
+ (list-form (cadr var-spec))
+ (result-form (cddr var-spec))
+ (body-forms (cdr body))
+ (locals (if (symbolp var)
+ (cons var local-vars)
+ local-vars)))
+ (pel-elcode--add-vars-from-list list-form symbols local-vars)
+ (pel-elcode--add-vars-from-lists result-form symbols locals)
+ (pel-elcode--add-vars-from-lists body-forms symbols locals)))
+
+ ;; (dotimes (VAR COUNT [RESULT]) BODY...)
+ ;; COUNT sees the outer scope. RESULT and BODY see VAR as local.
+ ((eq head 'dotimes)
+ (let* ((var-spec (car body))
+ (var (car var-spec))
+ (count-form (cadr var-spec))
+ (result-form (cddr var-spec))
+ (body-forms (cdr body))
+ (locals (if (symbolp var)
+ (cons var local-vars)
+ local-vars)))
+ (pel-elcode--add-vars-from-list count-form symbols local-vars)
+ (pel-elcode--add-vars-from-lists result-form symbols locals)
+ (pel-elcode--add-vars-from-lists body-forms symbols locals)))
+
+ ;; (setq VAR1 VAL1 VAR2 VAL2 ...)
+ ;; Target variables are writes, not reads. Only VAL forms are reads.
+ ((eq head 'setq)
+ (let ((val-forms (mapcar #'cadr (seq-partition body 2))))
+ (pel-elcode--add-vars-from-lists val-forms symbols local-vars)))
+
+ ;; (cond (TEST BODY...) ...)
+ ;; Each clause is a list of expressions, not a function call.
+ ((eq head 'cond)
+ (dolist (clause body)
+ (pel-elcode--add-vars-from-lists clause symbols local-vars)))
+
+ ;; (quote X) / (function X) / (declare ...) contain no variable reads
+ ;; for the purpose of this analysis.
+ ((memq head '(quote function declare)))
+
+ ;; Standard function/macro call: only arguments are value positions.
+ (t
+ (pel-elcode--add-vars-from-lists body symbols local-vars)))))
+
+ ;; If the head is not a symbol, every element is in value position.
+ ((consp exp)
+ (pel-elcode--add-vars-from-lists exp symbols local-vars)))
+
+ (reverse
+ (seq-filter #'identity
+ (delete-dups symbols)))))
+
(defun pel-elcode-operators-in (exp &optional local-vars)
"Recursively extract operator symbols from EXP, ignoring variable names.Then modify @@ -287,8 +432,10 @@
(defun pel-elcode-properties-of-sexp (sexp)
"Return a property declare form for specified SEXP.
The declare form identifies whether the sexp is pure, side-effect-free and/or
error-free."
- (let ((operators (pel-elcode-operators-in sexp)))
- (when operators
+ (let ((operators (pel-elcode-operators-in sexp))
+ (vars-read (pel-elcode-variables-read-in sexp)))
+ (when (or operators vars-read)
;; Some flow control/iteration special form/functions have
;; no impact on whether the defun is pure or side-effect-free,
;; so remove them from the inspected list of operators.
@@ -304,6 +451,17 @@ error-free."
;; have that property: so remove it from the defun-props.
(let ((defun-props (list 'pure 'side-effect-free 'error-free)))
+
+ ;; Reading a non-local variable makes the result depend on external
+ ;; state. That includes user-options (`defcustom' variables), globals
+ ;; and dynamically scoped variables. Such reads do not necessarily
+ ;; mutate state, so they do not by themselves remove `side-effect-free',
+ ;; but they must remove `pure'.
+ (when vars-read
+ (setq defun-props (delq 'pure defun-props))
+ ;; A read of an unbound non-local variable can signal void-variable.
+ (unless (seq-every-p #'boundp vars-read)
+ (setq defun-props (delq 'error-free defun-props))))
+
(catch 'pel-elcode-break
(dolist (op operators)
(unless (function-get op 'pure)Resulting behaviorGiven: (defun pel-completion-state-name ()
(if pel-use-ido "ido" "other"))
(pel-use-ido)So But it can still keep (declare (side-effect-free error-free))assuming The key design point is: operator analysis and variable-read analysis must be separate. Operators determine whether calls are pure/side-effect-free/error-free; free variable reads independently invalidate purity. 🐇💭 ✏️ Learnings added
🧠 Learnings used |
Owner
Author
|
@coderabbitai Please propose the code for a new PEL linter application that can be called from the command line. That application would process all Emacs Lisp files in the current directory and would identify any defun sexp that could have a |
* pel-elcode-properties-of-sexp now recognizes that sexp that allocate cannot be pure.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate relevant files ==="
fd -e el -i '^(pel-elcode|pel-lint)\.el$|^pel-lint\.el$|^pel.*lint.*\.el$'
echo
echo "=== bin files ==="
fd . bin -t f 2>/dev/null || true
echo
echo "=== pel-elcode.el definitions relevant to property inference ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "^(\\(defun|defconst|defvar|defcustom|defmacro) pel-elcode|pel-elcode-properties-of-sexp|pel-elcode-variables-read-in|pel-elcode-operators-in|declare" "$file" -C 4
' sh {}
echo
echo "=== Existing bin/pel-lint.el, if any ==="
if [ -f bin/pel-lint.el ]; then
sed -n "1,260p" bin/pel-lint.el
else
echo "bin/pel-lint.el not found"
fiLength of output: 21191 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate relevant files ==="
fd -e el -i '^(pel-elcode|pel-lint)\.el$|^pel-lint\.el$|^pel.*lint.*\.el$'
echo
echo "=== bin files ==="
fd . bin -t f 2>/dev/null || true
echo
echo "=== pel-elcode.el definitions relevant to property inference ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "^(\\(defun|defconst|defvar|defcustom|defmacro) pel-elcode|pel-elcode-properties-of-sexp|pel-elcode-variables-read-in|pel-elcode-operators-in|declare" "$file" -C 4
' sh {}
echo
echo "=== Existing bin/pel-lint.el, if any ==="
if [ -f bin/pel-lint.el ]; then
sed -n "1,260p" bin/pel-lint.el
else
echo "bin/pel-lint.el not found"
fiLength of output: 21191
I’m also using the existing PEL linter guidance/learning here: parse Emacs Lisp with Proposed new file:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pel-elcode.el`:
- Around line 512-521: In pel-elcode-properties-of-sexp, when op is in
pel-elcode-allocating-operators you currently only remove 'pure from
defun-props, leaving 'error-free/'side-effect-free and allowing declarations for
allocators; change the allocator branch to remove the same flags as the
non-allocator path (remove 'pure and also delete 'side-effect-free and
'error-free from defun-props—e.g. use pel-delqs on '(side-effect-free
error-free) like the other branch) so allocator wrappers no longer infer
side-effect-free/error-free.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6dbeb54-d52b-4aeb-a160-1707bfe275c7
📒 Files selected for processing (2)
pel-elcode.elpel-skels.el
✅ Files skipped from review due to trivial changes (1)
- pel-skels.el
Comment on lines
+512
to
+521
| ;; Allocation functions are declared side-effect-free by Emacs for | ||
| ;; byte-compiler purposes, but pel-elcode treats them as impure | ||
| ;; because they produce a new heap object on every call. | ||
| (if (memq op pel-elcode-allocating-operators) | ||
| (setq defun-props (delq 'pure defun-props)) | ||
| (pcase (function-get op 'side-effect-free) | ||
| ('error-free) | ||
| ('t (setq defun-props (delq 'error-free defun-props))) | ||
| (_ (setq defun-props (pel-delqs '(side-effect-free error-free) | ||
| defun-props))))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant code around the snippet lines
rg -n "pel-elcode-allocating-operators|pel-elcode-properties-of-sexp|defun-props|vars-read-in" -S pel-elcode.el
# Show the exact allocator branch block (around the lines mentioned in the review comment)
nl -ba pel-elcode.el | sed -n '480,560p'
# Show where pel-elcode-allocating-operators is defined and its docstring/rationale
nl -ba pel-elcode.el | sed -n '60,120p'
# Inspect the vars-read walker integration (to ensure it doesn't also affect the allocator branch)
nl -ba pel-elcode.el | sed -n '140,340p'
# Search for pel-delqs usage and any later normalization that might strip side-effect-free/error-free
rg -n "pel-delqs|delq 'side-effect-free|delq 'error-free|declare \\(side-effect-free|declare \\(error-free" pel-elcode.elRepository: pierre-rouleau/pel
Length of output: 1504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show allocator operators definition + surrounding rationale
sed -n '60,110p' pel-elcode.el | cat -n
echo "----"
# Show pel-elcode-properties-of-sexp around the allocator branch
sed -n '450,540p' pel-elcode.el | cat -nRepository: pierre-rouleau/pel
Length of output: 6746
Allocator operator wrappers still infer side-effect-free / error-free.
In pel-elcode-properties-of-sexp, the pel-elcode-allocating-operators branch removes only pure; it leaves error-free, so the final formatting can still emit (declare (side-effect-free error-free)) for functions that allocate fresh heap objects (e.g. wrapping make-hash-table), contradicting the allocator rule/docstring.
Suggested fix
- (if (memq op pel-elcode-allocating-operators)
- (setq defun-props (delq 'pure defun-props))
+ (if (memq op pel-elcode-allocating-operators)
+ (setq defun-props
+ (pel-delqs '(pure side-effect-free error-free)
+ defun-props))
(pcase (function-get op 'side-effect-free)
('error-free)
('t (setq defun-props (delq 'error-free defun-props)))
(_ (setq defun-props (pel-delqs '(side-effect-free error-free)
defun-props)))))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pel-elcode.el` around lines 512 - 521, In pel-elcode-properties-of-sexp, when
op is in pel-elcode-allocating-operators you currently only remove 'pure from
defun-props, leaving 'error-free/'side-effect-free and allowing declarations for
allocators; change the allocator branch to remove the same flags as the
non-allocator path (remove 'pure and also delete 'side-effect-free and
'error-free from defun-props—e.g. use pel-delqs on '(side-effect-free
error-free) like the other branch) so allocator wrappers no longer infer
side-effect-free/error-free.
Owner
Author
|
@coderabbitai I would liek the new pel-lint-declarations.el program to be able to detect invalid declarations too. If a defun has a declare form and that form is not appropriate, I would like the program to flag this as an error and stop. IS this the case in your proposed code? If not please propose modifications to its code. |
|
Tip For best results, initiate chat on the files or code changes.
For example, if inference says only: (declare (side-effect-free t))but the function already has: (declare (pure t) (side-effect-free t))the previous code would report no missing property, but it would fail to flag that Here are the modifications I would make. 1. Add helpers for invalid / unexpected declared properties(defun pel-lint-declarations--unexpected-props (present allowed)
"Return properties from PRESENT that are not in ALLOWED, preserving order."
(let ((unexpected '()))
(dolist (prop present (nreverse unexpected))
(unless (memq prop allowed)
(push prop unexpected)))))
(defun pel-lint-declarations--invalid-property-clauses-in-declare-form
(declare-form)
"Return invalid PEL property clauses from DECLARE-FORM.
Only clauses related to `pure' and `side-effect-free' are checked.
Other declaration clauses, such as `indent', `debug' or `obsolete', are
ignored by this linter."
(let ((invalid '()))
(dolist (clause (cdr-safe declare-form))
(pcase clause
(`(pure t)
nil)
(`(pure ,_)
(push clause invalid))
(`(side-effect-free t)
nil)
(`(side-effect-free error-free)
nil)
(`(side-effect-free ,_)
(push clause invalid))
(_
nil)))
(nreverse invalid)))
(defun pel-lint-declarations--invalid-property-clauses-in-defun-declare
(defun-form)
"Return invalid PEL property clauses in DEFUN-FORM's leading declarations."
(let ((body (cdddr defun-form))
(invalid '()))
;; Skip optional docstring.
(when (stringp (car-safe body))
(setq body (cdr body)))
;; Inspect one or more leading declare forms.
(while (and (consp body)
(consp (car body))
(eq (caar body) 'declare))
(setq invalid
(append invalid
(pel-lint-declarations--invalid-property-clauses-in-declare-form
(car body))))
(setq body (cdr body)))
invalid))2. Replace
|
Owner
Author
|
@coderabbitai I have changed my mind about stopping on the first error. Please modify the code to list all detected errors. |
|
Tip For best results, initiate chat on the files or code changes.
I’m using the earlier linter design context here: it already detects both missing and invalid/over-strong declaration properties; this change only alters diagnostic collection/reporting behavior. Task: make
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/pel-lint-declarations.el`:
- Around line 259-320: pel-lint currently stops at the first diagnostic because
pel-lint-declarations--first-diagnostic-in-file,
pel-lint-declarations-first-diagnostic, and pel-lint-declarations-main use
throw/catch to bail early; change them to accumulate diagnostics instead: modify
pel-lint-declarations--first-diagnostic-in-file to return a list of diagnostics
found in a file (e.g., collect diagnostics in a local list while reading forms
rather than throwing), update pel-lint-declarations-first-diagnostic to iterate
over filenames and concat each file's diagnostics into a master list (remove the
catch/throw), and update pel-lint-declarations-main to print each diagnostic in
that accumulated list (using princ/external-debugging-output) and call
(kill-emacs 1) if the list is non-empty or (kill-emacs 0) otherwise. Ensure you
keep existing helper calls like pel-lint-declarations--diagnostic-for-defun and
pel-lint-declarations--defun-form-p unchanged while switching flow from
early-return to accumulation.
In `@Makefile`:
- Line 894: The Makefile has a misspelled prerequisite: the
validate-declarations flow should depend on bin/pel-lint-declarations.elc but
the implicit prerequisite was attached to bin/pel-lint-delclarations.elc (typo).
Fix the wiring by renaming the erroneous target reference from
bin/pel-lint-delclarations.elc to bin/pel-lint-declarations.elc (or attach the
pel-elcode.elc prerequisite to the correctly spelled target), so that
validate-declarations -> bin/pel-lint-declarations.elc -> pel-elcode.elc is the
actual dependency chain and pel-elcode.elc is rebuilt before the linter runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64a66f5d-bfe5-472b-8ee4-04e72c6bea30
📒 Files selected for processing (2)
Makefilebin/pel-lint-declarations.el
Owner
Author
|
@coderabbitai I added a make support for the new
I ran the command by issuing the make command in a shell and I got the following:
The code is: Please analyze and report why the linter results are invalid. Please propose code modifications to the linter code. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate files ==="
fd -e el -i '^(pel-elcode|pel--base)\.el$|^pel-lint-declarations\.el$'
fd -t f -i '^Makefile$'
echo
echo "=== bin/pel-lint-declarations.el full ==="
fd -e el -i '^pel-lint-declarations\.el$' --exec sed -n '1,420p' {}
echo
echo "=== pel-elcode.el property inference definitions ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
rg -n "^(\\(defun|defmacro|defconst|defvar|defcustom) pel-elcode|pel-elcode-properties-of-sexp|pel-elcode-operators-in|pel-elcode-variables-read-in" "$file" -C 5
' sh {}
echo
echo "=== pel-elcode.el lines around pel-elcode-properties-of-sexp ==="
fd -e el -i '^pel-elcode\.el$' --exec sh -c '
file="$1"
line=$(rg -n "defun pel-elcode-properties-of-sexp" "$file" | cut -d: -f1 | head -1)
if [ -n "$line" ]; then
start=$((line-40)); [ "$start" -lt 1 ] && start=1
end=$((line+120))
sed -n "${start},${end}p" "$file"
fi
' sh {}
echo
echo "=== pel--base.el around pel-expression-p ==="
fd -e el -i '^pel--base\.el$' --exec sh -c '
file="$1"
rg -n "defun pel-expression-p" "$file" -C 12
nl -ba "$file" | sed -n "500,535p"
' sh {}
echo
echo "=== Makefile validate-declarations support ==="
sed -n '1240,1290p' Makefile 2>/dev/null || true
rg -n "pel-lint-declarations|validate-declarations|BIN_EL_FILES|BIN_ELC_FILES" Makefile -C 3 || trueLength of output: 32983
Why the linter result is invalid1. It stops after one diagnostic because the old first-diagnostic code is still presentThe file still has: pel-lint-declarations--first-diagnostic-in-file
pel-lint-declarations-first-diagnosticand both use (throw 'diagnostic diagnostic)So the “list all errors” change has not actually been applied yet. 2. The line number is wrong because
|
Owner
Author
|
@coderabbitai As for "The false positive is most likely caused by loading stale pel-elcode.elc" please note that the make file has the following dependency rule on line 894: So the the linter depending on old/stale pel-elcode.elc is not the problem. |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Owner
Author
|
@coderabbitai. There is a problem with the Makefile. A dependency is missing to build the elc file of the pel-int-declaration.el file. See my attempt from a shell when the current directory is the root of the repo: Please analyze and propose a change to the Makefile to ensure that the bin/pel-linter-declaration.el is compiled. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Makefile references to pel-lint-declarations / validate-declarations / pel-elcode ==="
rg -n "pel-lint-decl|pel-lint-delcl|validate-declarations|BIN_EL_FILES|BIN_ELC_FILES|pel-elcode\.elc" Makefile -C 4 || true
echo
echo "=== Makefile around reported generic compile rule and validation target ==="
sed -n '1140,1185p' Makefile 2>/dev/null || true
sed -n '1260,1285p' Makefile 2>/dev/null || true
sed -n '880,905p' Makefile 2>/dev/null || true
echo
echo "=== bin/pel-lint-declarations.el require and call sites ==="
if [ -f bin/pel-lint-declarations.el ]; then
rg -n "\(require 'pel-elcode\)|pel-elcode-properties-of-sexp|eval-and-compile|eval-when-compile|declare-function" bin/pel-lint-declarations.el -C 4
echo
sed -n '1,80p' bin/pel-lint-declarations.el
echo
sed -n '185,215p' bin/pel-lint-declarations.el
else
echo "bin/pel-lint-declarations.el not found"
fiLength of output: 14946
Root cause1. The explicit Makefile dependency target is misspelledCurrent Makefile Line 894: bin/pel-lint-delclarations.elc: pel-elcode.elcBut the actual generated target is: bin/pel-lint-declarations.elcSo the explicit dependency rule never applies. bin/%.elc: bin/%.el2.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Makefile occurrences ==="
rg -n "pel-lint-declarations|pel-lint-delclarations|validate-declarations|BIN_EL_FILES|BIN_ELC_FILES|pel-elcode\.elc" Makefile -C 3 || true
echo
echo "=== Makefile around line 860-910 ==="
sed -n '860,910p' Makefile || true
echo
echo "=== Makefile around line 1140-1180 ==="
sed -n '1140,1180p' Makefile || true
echo
echo "=== Makefile around line 1260-1285 ==="
sed -n '1260,1285p' Makefile || true
echo
echo "=== bin/pel-lint-declarations.el relevant lines ==="
if [ -f bin/pel-lint-declarations.el ]; then
rg -n "require|eval-and-compile|declare-function|pel-elcode-properties-of-sexp|provide" bin/pel-lint-declarations.el -C 4 || true
echo
nl -ba bin/pel-lint-declarations.el | sed -n '1,240p'
else
echo "bin/pel-lint-declarations.el not found"
fiLength of output: 12759 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Makefile occurrences ==="
rg -n "pel-lint-declarations|pel-lint-delclarations|validate-declarations|BIN_EL_FILES|BIN_ELC_FILES|pel-elcode\.elc" Makefile -C 3 || true
echo
echo "=== Makefile around line 860-910 ==="
sed -n '860,910p' Makefile || true
echo
echo "=== Makefile around line 1140-1180 ==="
sed -n '1140,1180p' Makefile || true
echo
echo "=== Makefile around line 1260-1285 ==="
sed -n '1260,1285p' Makefile || true
echo
echo "=== bin/pel-lint-declarations.el relevant lines ==="
if [ -f bin/pel-lint-declarations.el ]; then
rg -n "require|eval-and-compile|declare-function|pel-elcode-properties-of-sexp|provide" bin/pel-lint-declarations.el -C 4 || true
echo
nl -ba bin/pel-lint-declarations.el | sed -n '1,240p'
else
echo "bin/pel-lint-declarations.el not found"
fiLength of output: 12759
Root causeThe explicit dependency rule is misspelled: bin/pel-lint-delclarations.elc: pel-elcode.elcbut the actual target is: bin/pel-lint-declarations.elcSo bin/%.elc: bin/%.el
$(EMACS) ... -f batch-byte-compile $<That means the linter is compiled without a correct target-specific rule and without loading There is also a second issue: in (let ((load-prefer-newer t))
(require 'pel-elcode))Because it is wrapped in a normal Makefile fixReplace the misspelled dependency rule with a correctly spelled, explicit rule for If native compilation is enabledbin/pel-lint-declarations.elc: bin/pel-lint-declarations.el pel-elcode.elc
$(EMACS) -Q --batch -L . -L bin \
--eval '(setq byte-compile-error-on-warn t)' \
--eval '(require (quote pel-elcode))' \
-f batch-byte-compile $<
ifeq ($(EMACS_NATIVE_COMP_AVAILABLE), yes)
$(EMACS) -Q --batch -L . -L bin \
--eval '(setq byte-compile-error-on-warn t)' \
--eval '(require (quote pel-elcode))' \
-f batch-native-compile $<
endifThen remove or correct this bad line: -bin/pel-lint-delclarations.elc: pel-elcode.elcA cleaner patch in context would be: bin/pel-lint.elc: pel--base.elc
-bin/pel-lint-delclarations.elc: pel-elcode.elc
+bin/pel-lint-declarations.elc: bin/pel-lint-declarations.el pel-elcode.elc
+ $(EMACS) -Q --batch -L . -L bin \
+ --eval '(setq byte-compile-error-on-warn t)' \
+ --eval '(require (quote pel-elcode))' \
+ -f batch-byte-compile $<
+ifeq ($(EMACS_NATIVE_COMP_AVAILABLE), yes)
+ $(EMACS) -Q --batch -L . -L bin \
+ --eval '(setq byte-compile-error-on-warn t)' \
+ --eval '(require (quote pel-elcode))' \
+ -f batch-native-compile $<
+endif
pel--base.elc: pel-comp.elcThis ensures:
Optional source-side improvementI would also make the linter self-contained for direct byte compilation outside Make: (eval-and-compile
(let ((load-prefer-newer t))
(require 'pel-elcode)))instead of: (let ((load-prefer-newer t))
(require 'pel-elcode))That way both direct commands and Makefile builds get compile-time visibility. But the Makefile typo still needs to be fixed regardless. 🧠 Learnings used |
Owner
Author
|
@coderabbitai With the changes to the Makefile:
Here is the complete log: Please analyze and identify why the linter running from the command line gives different results from what I get from within Emacs. Is it possible the issue has to do with the major mode? Inside emacs the major more is |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
1 similar comment
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Owner
Author
|
@coderabbitai With the changes to the Makefile:
Here is the complete log: Please analyze and identify why the linter running from the command line gives different results from what I get from within Emacs. Is it possible the issue has to do with the major mode? Inside emacs the major more is |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Owner
Author
|
@coderabbitai With the changes to the Makefile:
Here is a partial log: Please analyze and identify why the linter running from the command line gives different results from what I get from within Emacs. Is it possible the issue has to do with the major mode? Inside emacs the major more is |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit