Continue analysis of liner / trying to fix#177
Conversation
WalkthroughTop-level require forms were moved in the lint script; ChangesDependency, inference, and declare tweaks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
@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 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# 1. Look at pel-expression-p definition in pel--base.el around line 515
grep -n "pel-expression-p" pel--base.el | head -30
echo "--- pel-expression-p defun ---"
# Find the defun and show ~30 lines
awk '/^\(defun pel-expression-p/,/^\(def/' pel--base.el | head -50Length of output: 445 🏁 Script executed: #!/bin/bash
# 2. Look at how pel-lint-declarations-main processes files
cat bin/pel-lint-declarations.elLength of output: 14588 🏁 Script executed: #!/bin/bash
# 3. Look at pel-elcode-properties-of-sexp - particularly what it looks at to infer pure/side-effect-free
grep -n "pure\|side-effect-free\|error-free\|emacs-lisp-mode\|major-mode\|syntax-table\|with-temp-buffer\|find-file\|insert-file" pel-elcode.el | head -60Length of output: 2199 🏁 Script executed: #!/bin/bash
# 4. Show the full pel-elcode-properties-of-sexp implementation
awk '/^\(defun pel-elcode-properties-of-sexp/,/^\(def[a-z]/' pel-elcode.el | head -120Length of output: 313 🏁 Script executed: #!/bin/bash
# Get line numbers for key functions in pel-elcode.el
grep -n "^(defun\|^(defsubst\|^(defalias" pel-elcode.el | head -40Length of output: 717 🏁 Script executed: #!/bin/bash
# Show pel-expression-p definition fully
sed -n '510,570p' pel--base.elLength of output: 2262 🏁 Script executed: #!/bin/bash
# Show the full pel-elcode-properties-of-sexp body by line number
grep -n "pel-elcode-properties-of-sexp" pel-elcode.elLength of output: 637 🏁 Script executed: #!/bin/bash
# Show what operators pel-expression-p calls (to understand what function-get checks are needed)
sed -n '510,560p' pel--base.elLength of output: 1971 🏁 Script executed: #!/bin/bash
# Show the full pel-elcode-properties-of-sexp implementation
sed -n '475,537p' pel-elcode.elLength of output: 3312 🏁 Script executed: #!/bin/bash
# Also show pel-elcode-variables-read-in and pel-elcode-operators-in to understand what they do
sed -n '155,300p' pel-elcode.elLength of output: 6511 🏁 Script executed: #!/bin/bash
# Check: which operators does pel-expression-p use, and what are their pure/side-effect-free properties
# In batch mode, are `not`, `eq`, `symbolp`, `consp` marked as pure?
emacs -Q --batch --eval '
(dolist (fn (list (quote not) (quote eq) (quote symbolp) (quote consp) (quote and) (quote or)))
(princ (format "%s: pure=%S side-effect-free=%S\n"
fn
(function-get fn (quote pure))
(function-get fn (quote side-effect-free)))))
'Length of output: 779 🏁 Script executed: #!/bin/bash
# Show pel-elcode-operators-in (lines 300-465)
sed -n '300,465p' pel-elcode.elLength of output: 7958 🏁 Script executed: #!/bin/bash
# Show the require/provide lines and the two key variable definitions in pel-elcode.el
grep -n "require\|provide\|pel-elcode-non-impacting-operators\|pel-elcode-structural-forms\|pel-elcode-allocating" pel-elcode.el | head -40Length of output: 1283 🏁 Script executed: #!/bin/bash
# Show the definitions of the two key variables
awk '/pel-elcode-non-impacting-operators/,/^$/' pel-elcode.el | head -30
echo "---"
awk '/pel-elcode-structural-forms/,/^$/' pel-elcode.el | head -30Length of output: 2138 🏁 Script executed: #!/bin/bash
# Show the top of pel-elcode.el (requires, defvars, etc.)
sed -n '1,107p' pel-elcode.elLength of output: 3809
Root Cause: Missing
|
|
@coderabbitai. Good catch! How did you figure that out? |
|
Tip For best results, initiate chat on the files or code changes.
Your question about major mode was actually a useful clue in the right direction — it pointed at "something different between interactive and batch environments". The major mode difference turned out to be a red herring (the linter already sets (ᐢ•ᴗ•ᐢ) 🐇 🧠 Learnings used |
|
@coderabbitai. In interactive Emacs where is byte-opt.el loaded? |
|
Tip For best results, initiate chat on the files or code changes.
(require 'byte-opt)So
In batch mode ( That is why 🧠 Learnings used |
|
@coderabbitai Does that mean that the pel-elcode functionality would not work if it was never byte-compiled and just interpreted? |
|
Tip For best results, initiate chat on the files or code changes.
What matters is not whether Here is the breakdown:
In an interactive session, In a bare batch session, neither the byte-compiler machinery nor its autoloads are activated, so That is precisely why 🧠 Learnings used |
|
@coderabbitai Perhaps, but then, what loads bytecomp.el? The byte compiler? Is this used when Emacs is just interpreting elisp code? |
|
Tip For best results, initiate chat on the files or code changes.
The Emacs Lisp Interpreter vs. the Byte CompilerThe Emacs Lisp interpreter is implemented entirely in C — in files like
So Why Is
|
|
@coderabbitai OK but saying that the file is loaded because the byte compiler is invoked is also saying that if the byte compiler was never invoked running the pel-elcode.el would not have loaded it if this was the very first thing done inside an Emacs session, no? |
|
Tip For best results, initiate chat on the files or code changes.
If So before this fix: (require 'pel-elcode)would load (require 'byte-opt)and therefore built-in function properties such as: (function-get 'not 'pure)
(function-get 'eq 'pure)
(function-get 'consp 'side-effect-free)could still be The important nuance is:
So the dependency is a runtime dependency of the analysis algorithm, not merely a compile-time dependency. A precise statement would be:
So yes, your conclusion is correct: without 🐇💭 🧠 Learnings used |
|
@coderabbitai OK, then would it not make sense to document this inside pel-elcode.el and require byte-opt in pel-elcode.el. to prevent this from happening? |
|
Tip For best results, initiate chat on the files or code changes.
A good placement would be near the existing (require 'pel--base) ; use: `pel-delqs'
(require 'seq) ; use: `seq-filter' (not autoloaded in Emacs 26)
;; `seq-partition', `seq-every-p'
(require 'pel-elisp) ; use: `pel-elisp-beginning-of-previous-form'
;; `pel-elisp-beginning-of-next-form'
;; `pel-elcode-properties-of-sexp' infers declaration properties by
;; querying function symbol properties such as `pure' and
;; `side-effect-free' with `function-get'. For many built-in functions
;; (`not', `eq', `symbolp', `consp', etc.) these properties are installed
;; by `byte-opt.el'. A fresh interpreted or `emacs -Q --batch' session
;; does not necessarily load the byte compiler machinery first, so require
;; `byte-opt' explicitly to make the inference environment-independent.
(require 'byte-opt)The key point is that this is not a compile-time-only dependency. It is a runtime dependency of the analysis algorithm. Without this explicit require, 🐇💭 ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai Also, the extra logic around the `(require pel-elcode) inside the linter is not really required in any case, is it? |
|
Tip For best results, initiate chat on the files or code changes.
The clean split is:
So I would simplify the linter from the special form to just: (require 'elisp-mode)
(require 'pel-elcode)The
Also, after adding So the final design is cleaner:
🐇💭 ✏️ Learnings added
🧠 Learnings used |
* The real fix for the linter is to ensure that pel-elcode.el requires byte-opt.el: so did that and removed the code not really needed in the linter. * Update the declarations of 2 functions as the current pel-elcode identifies. I don't think this is valid but I'll check next.
|
@coderabbitai The current pel-elcode was identifying the following via the linter: However both functions use the format function which could fail and signal an error if the object passed by argument is not the expected type. Therefore I think the pel-elcode analysis is incorect in that respect. Please analyze and report. I assume there is a bug in pel-elcode.el. Please propose a code replacement that would fix this and report that the 2 functions are only side-effect-free and not error-free. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===== pel-time.el: pel--time-fmt ====="
rg -n "defun pel--time-fmt|declare|format" -C 8 --iglob 'pel-time.el'
echo
echo "===== pel-yang.el: pel-yang-regexp-for ====="
rg -n "defun pel-yang-regexp-for|declare|format" -C 10 --iglob 'pel-yang.el'
echo
echo "===== pel-elcode.el: requires and property-related definitions ====="
rg -n "require|pel-elcode-properties-of-sexp|function-get|side-effect-free|error-free|pure|format|defcustom|defvar|defconst" -C 3 --iglob 'pel-elcode.el'
echo
echo "===== pel-elcode.el: operators/property inference area ====="
sed -n '1,130p' pel-elcode.el
echo "-----"
sed -n '130,340p' pel-elcode.el
echo "-----"
sed -n '340,570p' pel-elcode.elLength of output: 40210
The issue is that (format "%d:" "not-a-number")can signal because Root cause in
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pel-elcode.el (1)
542-549: ⚡ Quick winAdd a focused regression test for
format/format-messageinference.Please add an ERT case that asserts a defun using
format(orformat-message) infers only(declare (side-effect-free t)), noterror-free, to lock this new branch behavior.🧪 Proposed test addition
+ (ert-deftest ert-test-pel-elcode-properties-of-sexp--format-not-error-free () + "A defun using `format' is side-effect-free but not error-free." + (should (equal + (pel-elcode-properties-of-sexp + '(defun pel--fmt-test (n) + (declare (side-effect-free t)) + (format "%d:" n))) + '(declare (side-effect-free t)))))Based on learnings:
formatandformat-messagemust not preserve inferrederror-freeinpel-elcode.🤖 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 542 - 549, Add an ERT test that defines a small defun which calls format (and another that calls format-message) and asserts the inference result yields only (side-effect-free t) and not error-free; specifically, create an ert-deftest (e.g. ert-pel-elcode-infer-format-side-effect) that uses the same test helper used elsewhere in the test suite to run the pel-elcode inference on the defun, inspect the returned defun-props, and assert (equal defun-props '(side-effect-free t)) (and similarly for format-message), ensuring the new branch in the pel-elcode logic around pel-elcode-potentially-signaling-operators / (function-get op 'side-effect-free) is covered.
🤖 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.
Nitpick comments:
In `@pel-elcode.el`:
- Around line 542-549: Add an ERT test that defines a small defun which calls
format (and another that calls format-message) and asserts the inference result
yields only (side-effect-free t) and not error-free; specifically, create an
ert-deftest (e.g. ert-pel-elcode-infer-format-side-effect) that uses the same
test helper used elsewhere in the test suite to run the pel-elcode inference on
the defun, inspect the returned defun-props, and assert (equal defun-props
'(side-effect-free t)) (and similarly for format-message), ensuring the new
branch in the pel-elcode logic around pel-elcode-potentially-signaling-operators
/ (function-get op 'side-effect-free) is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e49172f0-e4bc-4c62-a319-dfc87b7ca2c1
📒 Files selected for processing (5)
pel-elcode.elpel-sudo-edit.elpel-text-transform.elpel-time.elpel-yang.el
✅ Files skipped from review due to trivial changes (3)
- pel-yang.el
- pel-sudo-edit.el
- pel-text-transform.el
|
@coderabbitai With the latest changes the problem with format is fixed. However a new problem appears:
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. |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Summary by CodeRabbit
Refactor
Chores