Continue pel-elcode and linter investigation.#178
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (15)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds and updates Emacs Lisp ChangesDeclarations and timestamps
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 With the latest changes the problem with format is fixed. However a new problem appears:
Note that the check was done in the file pel-skels-cpp.e content before the chenge I submitted in this PR. These two do not match! What causes this problem with the latest code? Please analyze and report. Then please propose modifications to solve this problem. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look at pel-skels-c++-doc-section-titles definition around line 241
echo "=== pel-skels-cpp.el around line 241 ==="
grep -n "pel-skels-c++-doc-section-titles" pel-skels-cpp.el | head -20
echo ""
echo "=== Full function body ==="
# Find the defun and show ~30 lines
awk '/defun pel-skels-c\+\+-doc-section-titles/,/^\(def/' pel-skels-cpp.el | head -50
echo ""
echo "=== pel-elcode.el - properties-of-sexp function ==="
awk '/defun pel-elcode-properties-of-sexp/,/^\(def/' pel-elcode.el | head -120
echo ""
echo "=== pel-elcode.el - variable read walker if any ==="
grep -n "non-local\|free-var\|variable-read\|walk\|read-var\|local-var\|pel-elcode-read" pel-elcode.el | head -30
echo ""
echo "=== bin/pel-lint-declarations.el - require and key logic ==="
cat bin/pel-lint-declarations.el | head -100
echo ""
echo "=== Does pel-elcode.el require byte-opt? ==="
grep -n "require.*byte-opt\|byte-opt" pel-elcode.el | head -10
echo ""
echo "=== pel-elcode.el - how it handles unknown/user-defined functions ==="
grep -n "function-get\|defun-props\|nil\|unknown" pel-elcode.el | head -40Length of output: 9088 🏁 Script executed: #!/bin/bash
echo "=== pel-skels-c++-doc-section-titles full body ==="
# Use sed to extract from defun line to the closing paren of the defun
sed -n '241,290p' pel-skels-cpp.el
echo ""
echo "=== pel-elcode-properties-of-sexp full body ==="
# Get the full function (approximate range)
grep -n "defun pel-elcode" pel-elcode.elLength of output: 2697 🏁 Script executed: #!/bin/bash
echo "=== pel-elcode-properties-of-sexp full body (lines 495-570) ==="
sed -n '495,570p' pel-elcode.el
echo ""
echo "=== What is pel-append-to? ==="
grep -n "pel-append-to\|defmacro pel-append\|defun pel-append" pel-macros.el 2>/dev/null | head -20
grep -rn "defmacro pel-append-to\|defun pel-append-to" --include="*.el" | head -10
echo ""
echo "=== Does pel-append-to have a declare form? ==="
grep -A5 "defmacro pel-append-to\|defun pel-append-to" pel-macros.el 2>/dev/null | head -20Length of output: 4535
Root Cause
The Fix:
|
| Environment | pel-append-to loaded? |
inferred | result |
|---|---|---|---|
| Interactive | ✓ (expands to built-ins) | side-effect-free |
✓ |
| Batch | ✗ (absent property) | side-effect-free |
✓ no false-invalid |
The PR's removal of (declare (side-effect-free t)) from pel-skels-c++-doc-section-titles should be reverted — the annotation is correct and the fix above will make the batch linter agree.
✏️ Learnings added
Learnt from: pierre-rouleau
URL: https://github.com/pierre-rouleau/pel/pull/178
Timestamp: 2026-06-04T10:31:48.680Z
Learning: In PEL's `pel-elcode.el`, the `pcase` arm `(_ (pel-delqs '(side-effect-free error-free) defun-props))` in `pel-elcode-properties-of-sexp` (around line 545) is triggered both when a function has no `side-effect-free` property registered (absent — unknown/unloaded operator) and when it is explicitly nil (known side-effectful). In batch mode (`emacs -Q --batch`) user-defined macros such as `pel-append-to` (from `pel--macros.el`) are not loaded and cannot be macro-expanded, causing false "invalid declaration" reports. The fix is to use `(plist-member (symbol-plist op) 'side-effect-free)` inside the `_` arm: only remove both properties when the property is explicitly on the plist; when absent (unknown operator), remove only `error-free` (conservative) and keep `side-effect-free` (neutral).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T21:11:49.567Z
Learning: [scratchpad_start]
[task analyze_build_failure_emacs27_1/1]
[observations]
- `pel--eval-buffer-binding-type` is at lines ~78-81 in `pel-elisp-eval.el`
- Its declare form: `(declare (pure t) (side-effect-free error-free))`
- Its body: `(if lexical-binding "lexical" "dynamic")` — reads buffer-local var `lexical-binding`
- `pel-eval-info` calls `(pel--eval-buffer-binding-type)` at line ~86 with no arguments
- Error: "In toplevel form: pel-elisp-eval.el:86:4:Error: Symbol's function definition is void: pel--eval-buffer-binding-type"
- Emacs 27.1 fails; Emacs 30.2 succeeds
[analysis]
- Root cause: `(declare (pure t))` tells the Emacs 27 byte-compiler that this function is pure and its calls with constant arguments can be constant-folded at compile time
- `(pel--eval-buffer-binding-type)` takes no arguments → all arguments are trivially constant → Emacs 27's byte optimizer tries to call the function at compile time (during the optimization pass of `pel-eval-info`)
- In Emacs 27 batch mode (`emacs -Q --batch`), during byte-compilation, the function may not yet be established in the interpreter's runtime environment when the optimizer fires — producing "Symbol's function definition is void"
- Emacs 30's byte optimizer is smarter: it recognizes the function reads a buffer-local variable (`lexical-binding`) and does not attempt to fold it, OR the compile-time environment handles it differently
- **Semantic error too**: the function reads `lexical-binding`, a buffer-local variable whose value changes per-buffer. A function whose return value depends on buffer-local state is definitionally NOT `pure` (pure = same inputs always give same output, no dependence on external state). `(pure t)` was incorrect to begin with
- The fix is to remove `(pure t)` from the `declare` form; `(side-effect-free error-free)` can remain since the function only reads (doesn't write) state
[done]
Analysis complete.
[scratchpad_end]
pierre-rouleau: I've identified the root cause. Here is the full diagnosis.
---
## Root Cause: `(declare (pure t))` Is Both Incorrect and Emacs-27-Incompatible
### Why the Build Breaks on Emacs 27
In Emacs 27's byte-compiler, `(declare (pure t))` inside a `defun` registers the function with the `pure` property. The byte-optimizer then applies **compile-time constant folding**: when it encounters a call `(pel--eval-buffer-binding-type)` with no arguments, all arguments are trivially constant, so it tries to *call the function at compile time* to replace the call with a literal result.
In Emacs 27 batch mode (`emacs -Q --batch`), during the optimization pass of `pel-eval-info`, the function `pel--eval-buffer-binding-type` is not yet (or is no longer) established in the interpreter's live environment — producing:
```
Symbol's function definition is void: pel--eval-buffer-binding-type
```
Emacs 30 does not trigger this because its optimizer detects that the function body reads a buffer-local variable (`lexical-binding`) and does not attempt to fold it.
### Why `(pure t)` Is Also Semantically Wrong
The `pure` property means: *given the same arguments, the function always returns the same value, with no dependence on external state*. But `pel--eval-buffer-binding-type` reads `lexical-binding`, which is a **buffer-local variable** — its value differs per buffer. The function is therefore not pure by definition. The annotation was incorrect from the start.
---
## The Fix
Remove only `(pure t)` from the `declare` form. The `(side-effect-free error-free)` annotation can remain since the function only reads state without modifying it:
```diff
(defun pel--eval-buffer-binding-type ()
"Return a string describing the binding type of current buffer code."
- (declare (pure t) (side-effect-free error-free))
+ (declare (side-effect-free error-free))
(if lexical-binding "lexical" "dynamic"))
```
This single-character change resolves the Emacs 27 build failure and corrects the invalid semantic annotation.
<!--
In PEL's `pel-elisp-eval.el`, `(declare (pure t))` must NOT be applied to `pel--eval-buffer-binding-type`. The function reads the buffer-local variable `lexical-binding`, making it semantically impure (its return value depends on per-buffer state). Additionally, in Emacs 27's batch byte-compiler, `(declare (pure t))` causes the optimizer to attempt compile-time constant-folding of any zero-argument call to the function; when the function is not yet present in the interpreter's live environment during the optimization pass, Emacs 27 signals "Symbol's function definition is void". The correct declare form is `(declare (side-effect-free error-free))` only.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T21:22:08.946Z
Learning: [scratchpad_start]
[task analyze_emacs26_build_failure_pel_completion_1/1]
[observations]
- `pel--available-completion-mode-mask` is defined at line 574 with `(declare (pure t) (side-effect-free t))` at line 582
- It is a zero-argument function
- It is called at line 745 inside `pel-set-completion-mode`:
`(allowed-mask (logand requested-mask (pel--available-completion-mode-mask)))`
- The error is at line 714:53 — this is inside the `let*` form of `pel-set-completion-mode`
- The function body reads `pel-use-ido`, `pel-use-ivy`, `pel-use-counsel`, `pel-use-helm` — all external global user-option (defcustom) variables
[analysis]
- Root cause is identical to the `pel--eval-buffer-binding-type` case:
1. `(declare (pure t))` marks the function as pure → Emacs 26/27 byte-optimizer tries to constant-fold the zero-argument call at compile time
2. In batch compilation (`emacs -Q --batch`), the function is not yet live in the interpreter → "Symbol's function definition is void"
3. The annotation is also semantically wrong: the function reads mutable global variables (`pel-use-ido`, etc.) so its return value depends on external state → it is NOT pure
- `(side-effect-free t)` can remain: the function only reads state without modifying it → no observable side effects
- Fix: remove `(pure t)` only; keep `(side-effect-free t)`
[done]
Analysis complete.
[scratchpad_end]
pierre-rouleau: The root cause is identical to the `pel--eval-buffer-binding-type` case.
---
## Root Cause: `(declare (pure t))` on `pel--available-completion-mode-mask`
Line 582 of `pel-completion.el`:
```elisp
(defun pel--available-completion-mode-mask ()
"Return bit mask corresponding to the encoding of completion modes available."
(declare (pure t) (side-effect-free t)) ; ← (pure t) is the culprit
(let ((mask 0))
(when pel-use-ido ...)
(when pel-use-ivy ...)
(when pel-use-counsel ...)
(when pel-use-helm ...)
mask))
```
The function is then called at line 745 with **no arguments**:
```elisp
(allowed-mask (logand requested-mask
(pel--available-completion-mode-mask)))
```
Because `(declare (pure t))` is present, Emacs 26's (and 27's) byte-optimizer sees a zero-argument call to a pure function — all arguments are trivially constant — and attempts to **constant-fold it at compile time** by calling the function during the optimization pass. In `emacs -Q --batch` mode, the function is not yet live in the interpreter at that point, so:
```
Symbol's function definition is void: pel--available-completion-mode-mask
```
### Why `(pure t)` is also semantically wrong
The function body reads `pel-use-ido`, `pel-use-ivy`, `pel-use-counsel`, and `pel-use-helm` — all mutable global user-option (`defcustom`) variables. A **pure** function must return the same value given the same arguments with no dependence on external state. Since this function's return value changes when those customization variables change, it is definitionally not pure.
`(side-effect-free t)` is correct and can stay: the function only reads variables without modifying any state.
---
## The Fix
Remove only `(pure t)`:
```diff
(defun pel--available-completion-mode-mask ()
"Return bit mask corresponding to the encoding of completion modes available."
- (declare (pure t) (side-effect-free t))
+ (declare (side-effect-free t))
(let ((mask 0))
```
---
## Pattern to Watch For Going Forward
Any **zero-argument function** annotated with `(declare (pure t))` is at risk of this failure on Emacs 26/27, regardless of whether the function actually reads buffer-local or global variables. The rule of thumb for this PR's property declarations:
- `(pure t)` is only valid for functions whose return value depends **solely on their explicit arguments** (no global variables, no buffer-local variables, no dynamic state). A zero-argument function can only be `pure` if it always returns the same constant value (e.g., a function that just returns a literal).
- `(side-effect-free t)` is safe for functions that read but don't write any state.
<!--
In PEL's `pel-completion.el`, `(declare (pure t))` must NOT be applied to `pel--available-completion-mode-mask`. The function reads mutable global user-option variables (`pel-use-ido`, `pel-use-ivy`, `pel-use-counsel`, `pel-use-helm`), making it semantically impure. Additionally, in Emacs 26/27's batch byte-compiler, `(declare (pure t))` on any zero-argument function causes the optimizer to attempt compile-time constant-folding; when the function is not yet live in the interpreter, Emacs signals "Symbol's function definition is void". The correct declare form is `(declare (side-effect-free t))` only — `side-effect-free` is correct since the function only reads state without modifying it.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-04T09:50:05.865Z
Learning: In PEL's `pel-elcode.el`, `format` and `format-message` must not preserve inferred `error-free` declarations. They are side-effect-free, but can signal when supplied arguments do not match the format directives, and `pel-elcode` does not perform type inference. Functions such as `pel-time.el` `pel--time-fmt` and `pel-yang.el` `pel-yang-regexp-for` should therefore infer `(declare (side-effect-free t))`, not `(declare (side-effect-free error-free))`, when their only potentially problematic operator is `format`.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T22:11:15.883Z
Learning: In PEL's proposed `bin/pel-lint-declarations.el`, the linter should detect both missing and invalid declarations. It should compare inferred properties from `pel-elcode-properties-of-sexp` against existing defun `declare` properties in both directions: report missing properties when inferred props are absent, and report invalid/over-strong properties when present props are not inferred. Invalid declarations should be reported before missing declarations, and malformed known property clauses such as `(pure nil)` or `(side-effect-free nil)` should be flagged while unrelated declare clauses like `indent` or `debug` are ignored.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-04T03:41:10.632Z
Learning: In PEL's `pel-elcode.el`, `pel-elcode-properties-of-sexp` calls `(function-get op 'pure)` and `(function-get op 'side-effect-free)` for each operator in the analyzed sexp. These symbol properties for built-in functions like `not`, `eq`, `symbolp`, `consp` are registered by `byte-opt.el` (part of the Emacs byte compiler). In an interactive Emacs session `byte-opt.el` is loaded, so inference works correctly. In `emacs -Q --batch` without invoking the byte-compiler, `byte-opt.el` is NOT loaded, causing `function-get` to return nil for all operators, which drains `defun-props` to nil and makes `pel-elcode-properties-of-sexp` return nil — producing false "invalid declaration" positives in the `pel-lint-declarations` linter. The fix is to add `(require 'byte-opt)` to `pel-elcode.el` so the built-in function properties are always available regardless of environment.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T21:16:02.451Z
Learning: In PEL's `pel-elisp-eval.el`, `(declare (pure t))` must NOT be applied to `pel--eval-buffer-binding-type`. The function reads the buffer-local variable `lexical-binding`, making it semantically impure (its return value depends on per-buffer state). Additionally, in Emacs 27's batch byte-compiler, `(declare (pure t))` causes the optimizer to attempt compile-time constant-folding of any zero-argument call to the function; when the function is not yet present in the interpreter's live environment during the optimization pass, Emacs 27 signals "Symbol's function definition is void". The correct declare form is `(declare (side-effect-free error-free))` only.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-04T03:10:40.694Z
Learning: In PEL's `bin/pel-lint-declarations.el`, wrapping `(require 'pel-elcode)` inside `(let ((load-prefer-newer t)) ...)` is not enough for byte-compiler function-definition discovery when `byte-compile-error-on-warn` is enabled. Either load `pel-elcode` at compile time with `(eval-and-compile (let ((load-prefer-newer t)) (require 'pel-elcode)))`, or have the Makefile's `bin/pel-lint-declarations.elc` recipe preload `pel-elcode` with `-l pel-elcode` before `-f batch-byte-compile`.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-03T21:42:55.545Z
Learning: In `pel-elcode.el` (PEL), `pel-elcode-properties-of-sexp` must not infer `(pure t)` solely from operator properties. It also needs to account for non-local variable reads: any unquoted free/global/user-option variable read makes the analyzed sexp depend on external state and must remove `pure`, although it does not by itself remove `side-effect-free`. A robust fix is to add a separate variable-read walker (tracking defun/lambda arguments and let/dolist/dotimes locals, skipping quote/function/declare and setq targets) and have `pel-elcode-properties-of-sexp` strip `pure` when that walker returns any non-local variable reads.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-04T04:35:00.603Z
Learning: In PEL's `bin/pel-lint-declarations.el`, if the Makefile byte/native compilation recipes preload `pel-elcode` with `-l pel-elcode`, the linter does not need extra `(eval-and-compile ...)` or `(let ((load-prefer-newer t)) ...)` logic around `(require 'pel-elcode)`. A plain top-level `(require 'pel-elcode)` is sufficient for standalone runtime loading, while the Makefile preload handles byte-compiler function-definition discovery. The `byte-opt` dependency belongs in `pel-elcode.el`, not in the linter.
Learnt from: pierre-rouleau
Repo: pierre-rouleau/pel PR: 0
File: :0-0
Timestamp: 2026-06-04T03:10:40.694Z
Learning: In PEL PR `#175`, the Makefile dependency for `bin/pel-lint-declarations.elc` must use the correctly spelled target name `bin/pel-lint-declarations.elc`, not `bin/pel-lint-delclarations.elc`. Otherwise GNU Make falls back to the generic `bin/%.elc: bin/%.el` rule and the linter’s dependency on `pel-elcode.elc` is not applied.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# 1. Find and show pel-skels-c++-doc-section-titles in pel-skels-cpp.el
echo "=== pel-skels-cpp.el around line 241 ==="
rg -n "pel-skels-c\+\+-doc-section-titles" pel-skels-cpp.el -A 30 -B 3
echo ""
echo "=== Full pel-elcode.el ==="
cat pel-elcode.el
echo ""
echo "=== Full bin/pel-lint-declarations.el ==="
cat bin/pel-lint-declarations.elLength of output: 44151 🏁 Script executed: #!/bin/bash
echo "=== Find definition of pel-append-to ==="
rg -n "defmacro pel-append-to\|defun pel-append-to" --type=elisp -A 10
echo ""
echo "=== What does pel-append-to expand to? (full definition) ==="
rg -n "pel-append-to" --type=elisp -B 1 -A 15 | head -80
echo ""
echo "=== Check what pel-skels-cpp.el requires ==="
rg -n "^(require\|eval-when-compile\|eval-and-compile)" pel-skels-cpp.el | head -30Length of output: 5459 🏁 Script executed: #!/bin/bash
echo "=== Find defmacro pel-append-to (no type filter) ==="
rg -n "defmacro pel-append-to" -A 10
echo ""
echo "=== pel-skels-cpp.el requires (broader search) ==="
grep -n "require\|eval-when\|eval-and" pel-skels-cpp.el | head -30
echo ""
echo "=== pel--macros.el full content ==="
fd "pel--macros.el" -x cat {}
echo ""
echo "=== pel-skels-cpp.el first 50 lines ==="
head -50 pel-skels-cpp.elLength of output: 14057
Root Cause:
|
Owner
Author
|
@coderabbitai While this new code reduce the probability of false positives, it does not eliminate that, right? The issue being the use of unknown macros. Would it be possible for the linter to warn about the unknown macros and allow the user to specify a list of files that contain these unknown macros to allow the linter to load them and proceed with all operators known? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Current bin/pel-lint-declarations.el - startup and main ==="
cat bin/pel-lint-declarations.elLength of output: 14561
Plan
🐇 ₍˶ˆ꒳ˆ˶₎ ✏️ Learnings added
🧠 Learnings usedFailed to handle agent chat message. Please try again. |
Owner
Author
|
@coderabbitai. Also with the proposed change the pel-elcode now identifies a lot of defun as being side-effect-free even though they are not becuase the call insert or other functions that have side effect. Therefore your analaysis identified the root cause but your original code proposal to update the pcase is not the right solution. The solution would be to have the linter be able to warn when an operator is unknown as a warning, and allow the user to specify a list of functions the linter would load to get the needed macro definitions. One way would be to create a project-specialized linter. For PEL that could mean to load all macro defining files in the linter. |
|
Tip For best results, initiate chat on the files or code changes.
Why the
|
Owner
Author
|
@coderabbitai Since the source code analyzed does load the macro code because it byte-compiles cleanly, and the linter does not see that, would it be possible to placed hints inside the source file being analyzed that the linter should load the same files? Or perhaps the linter should recognize all |
|
Tip For best results, initiate chat on the files or code changes.
Why Auto-Loading
|
Owner
Author
|
@coderabbitai. If a defun uses the If my assumption is correct, the current pel-elcode.el implementation does not take that fact into account. Please propose (here, in this PR) code modification to pel-elcode.el to fix that. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== pel-elcode.el: requires and defvars ==="
sed -n '1,140p' pel-elcode.el
echo
echo "=== pel-elcode.el: allocating operators and properties-of-sexp ==="
rg -n "pel-elcode-allocating-operators|defun pel-elcode-properties-of-sexp|function-get|side-effect-free|pure" -C 8 pel-elcode.el
echo
echo "=== pel-elcode.el: full properties-of-sexp area ==="
sed -n '450,590p' pel-elcode.elLength of output: 20580 🏁 Script executed: #!/bin/bash
set -e
echo "=== pel-elcode.el: requires and defvars ==="
sed -n '1,140p' pel-elcode.el
echo
echo "=== pel-elcode.el: allocating operators and properties-of-sexp ==="
rg -n "pel-elcode-allocating-operators|defun pel-elcode-properties-of-sexp|function-get|side-effect-free|pure" -C 8 pel-elcode.el
echo
echo "=== pel-elcode.el: full properties-of-sexp area ==="
sed -n '450,590p' pel-elcode.elLength of output: 20580
A function like: (defun example ()
(list 1 2 3))is side-effect-free: calling it does not mutate any existing state, buffer, variable, file, process, etc. It allocates a fresh object, but allocation of the returned value is not considered a side effect for Emacs’ However, it is not pure in the strict optimizer sense, because each call returns a fresh cons structure: (eq (example) (example)) ;; => nilSo the right inference is: (declare (side-effect-free error-free))or at least: (declare (side-effect-free t))depending on whether all called operators are known not to signal. What is wrong in current
|
Owner
Author
|
@coderabbitai Are you really sure that defuns that call |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query:
Length of output: 17273 🌐 Web query:
Length of output: 17273
The key distinction is:
So: (defun f ()
(make-hash-table))should infer: (declare (side-effect-free t))but not: (declare (pure t))and probably not: (declare (side-effect-free error-free))unless the analyzer can prove the arguments cannot signal. For example, bad keyword arguments or bad test specs can signal, so The evidence is consistent:
So the correct ;; allocation => remove pure
;; allocation does NOT => remove side-effect-freeThe code should keep Suggested comment/docstring correction: (defconst pel-elcode-allocating-operators
'(make-hash-table
copy-hash-table
make-vector
make-list
cons
list
vector
concat
format
format-message
;; ...
)
"Functions that allocate fresh objects.
Allocation does not make a function side-effecting under Emacs
byte-compiler semantics: if the allocated object is ignored, no
existing Lisp-visible state is changed.
However, allocation prevents `pure' inference because repeated calls may
return fresh, non-`eq' objects. For example, two calls to
`make-hash-table' or `list' produce distinct objects.")And the inference logic should remain shaped like this: ;; Allocation functions may still be side-effect-free, but they are not pure:
;; repeated calls may return fresh, non-`eq' objects.
(when (memq op pel-elcode-allocating-operators)
(setq defun-props (delq 'pure defun-props)))So yes: ✏️ Learnings added
🧠 Learnings used |
Owner
Author
|
@coderabbitai. Well I am getting a very different point of view when asking the question to Gemini. Gemini states that functions that call make-hash-table are NOT side-effect-free. See what it wrote: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query:
Length of output: 14250 🌐 Web query:
Length of output: 18964
For Emacs, these are different properties:
The manual text from your web result is the key point:
Gemini’s example says:
That is the incorrect step. In Emacs, duplicate-call substitution / constant folding is tied to (if (byte-opt--fget fn 'pure)
(byte-optimize-constant-args form)
form)So Also, your second search result shows Emacs itself lists Concrete PEL implicationFor a function like: (defun pel-example ()
(make-hash-table))
(declare (side-effect-free t))but not: (declare (pure t))and generally not: (declare (side-effect-free error-free))unless the analyzer can prove the call cannot signal. So the current design principle remains: ;; Allocation invalidates `pure`, but not `side-effect-free`.
(when (memq op pel-elcode-allocating-operators)
(setq defun-props (delq 'pure defun-props)))The important fix is to ensure comments/docstrings in 🧠 Learnings used |
Owner
Author
|
@coderabbitai. What pel-elcode.el does at the moment is irrelevant. I want to ensure that what is does is correct. But I checked again with Gemini and got it to agree with the fact that make-table-hash does not prevent a defun from being considered side-effect-free. Would you agree with the following statement? "Because side-effect-free only governs dead code elimination (deleting the call if the value is unused), a function that uses make-hash-table and nothing else satisfies this property perfectly." |
|
Tip For best results, initiate chat on the files or code changes.
For Emacs Lisp So this function: (defun example ()
(make-hash-table))can be considered: (declare (side-effect-free t))because if But it should not be considered: (declare (pure t))because each call returns a fresh, distinct hash table: (eq (example) (example)) ; => nilAlso, So the design rule is:
🐇💭 🧠 Learnings used |
* Add declarations to defun that were missing them as identified by running the pel-int-declarations.el code - Note that to handle elisp macros, the linter loads the pel--macros.el file making PEL macros known and also making the tool PEL specialized. A better implementation would load the files required by the analyzed files and that would load the necessary macro definitions. To do next.
Owner
Author
|
@coderabbitai. Please analyze the latest version of all files modified in this PR and validate if the new or modified declarations forms are valid. |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
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