Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Enhanced error highlighting, errors in red, warnings in yellow. #307

Merged
merged 2 commits into from

3 participants

@sw1nn

This change enhances error highlighting in nrepl.

Key Features:

  • Highlights both errors and warnings (warnings are typically seen when *warn-on-reflection* is set)
  • Highlights only the containing sexp rather than whole line, which allows a warning and error on the same line. If the compiler started emitting multiple errors, each would show separately
  • Displays compilation error message in minibuf on M-x next-error and mouse hover.

In emacs < 24.3 it looks like this:
Screenshot - 150313 - 18:06:43

In emacs 24.3 and later, you can use the wave underline property. Add this:

(set-face-attribute 'nrepl-error-highlight-face nil :inherit nil :underline '(:color "red4" :style wave))
(set-face-attribute 'nrepl-warning-highlight-face nil :inherit nil :underline '(:color "gold4" :style wave))

to init.el (or equivalent) and it looks like this:
Screenshot - 150313 - 18:29:39

Warning and error on same line:
Screenshot - 150313 - 18:36:41

nrepl.el
@@ -875,38 +887,56 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (matchspecs message)
+ "Extract error information with MATCHSPECS against MESSAGE."
+ (first (delq nil
@bbatsov Owner
bbatsov added a note

It'd be better to use remove or remove-if here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
@@ -875,38 +887,56 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (matchspecs message)
+ "Extract error information with MATCHSPECS against MESSAGE."
+ (first (delq nil
+ (mapcar
+ (lambda (matchspec)
+ (let ((file (nth 1 matchspec))
@bbatsov Owner
bbatsov added a note

The use of so many positional arguments is pretty confusing. Why not use a plist/alist/map instead?

@sw1nn
sw1nn added a note

agreed, but this is the format of the error matches in compilation-error-regexps that I'm trying to stay consistent with. This (nth .. ) style code is lifted from compile.el

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
@@ -875,38 +887,56 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (matchspecs message)
+ "Extract error information with MATCHSPECS against MESSAGE."
+ (first (delq nil
+ (mapcar
+ (lambda (matchspec)
+ (let ((file (nth 1 matchspec))
+ (line (nth 2 matchspec))
+ (col (nth 3 matchspec))
+ (type (nth 4 matchspec))
+ (highlight (nth 5 matchspec))
+ (pat (car matchspec)))
@bbatsov Owner
bbatsov added a note

Use first instead of car.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
@@ -875,38 +887,56 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (matchspecs message)
+ "Extract error information with MATCHSPECS against MESSAGE."
+ (first (delq nil
+ (mapcar
+ (lambda (matchspec)
+ (let ((file (nth 1 matchspec))
+ (line (nth 2 matchspec))
+ (col (nth 3 matchspec))
@bbatsov Owner
bbatsov added a note

Is the column something new in Clojure 1.5. I don't recall 1.4 stacktraces having a column in them.

@sw1nn
sw1nn added a note

yes, it seems that 1.4 stacktraces, don't have column. This explains why highlighting isn't working for me in HEAD. I've added further logic to handle both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
((13 lines not shown))
+ (type (nth 4 matchspec))
+ (highlight (nth 5 matchspec))
+ (pat (car matchspec)))
+ (when (string-match pat message)
+ (list
+ (match-string-no-properties file message)
+ (string-to-number (match-string-no-properties line message))
+ (string-to-number (match-string-no-properties col message))
+ (aref [nrepl-warning-highlight-face
+ nrepl-warning-highlight-face
+ nrepl-error-highlight-face]
+ (or type 2))
+ (match-string-no-properties highlight message)))))
+ matchspecs))))
+
+(defun nrepl-highlight-compilation-errors (buffer message)
@bbatsov Owner
bbatsov added a note

We cannot actually highlight more than 1 error, so I guess nrepl-highlight-compilation-error would be a more accurate name.

@sw1nn
sw1nn added a note

Actually we can highlight more than one error, it's just that the compiler only generates at most one error at the moment. Maybe the compiler will at some future point emit more errors and we should be ready. There's a screenshot in the PR description with an example of a errror and warning on the same line.

@bbatsov Owner
bbatsov added a note

Fair point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
((48 lines not shown))
- (overlay-put overlay 'nrepl-note message)
- (overlay-put overlay 'help-echo message)))))))
-
-(defun nrepl-extract-error-line (stacktrace)
- "Extract the error line number from STACKTRACE."
- (string-match "compiling:(.+:\\([0-9]+\\)" stacktrace)
- (string-to-number (match-string 1 stacktrace)))
-
-(defun nrepl-extract-error-filename (stacktrace)
- "Extract the error filename from STACKTRACE."
- (string-match "compiling:(\\(.+\\):" stacktrace)
- (substring-no-properties (match-string 1 stacktrace)))
+ (let ((matchspecs (rest (assq 'nrepl compilation-error-regexp-alist-alist))))
+ (let ((info (nrepl-extract-error-info matchspecs message)))
+ (when info
+ (let ((file (nth 0 info))
@bbatsov Owner
bbatsov added a note

The same point I made earlier about too many positional arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/nrepl-tests.el
((18 lines not shown))
+ (should (string= (nth 0 info) "/some/test/file/core.clj"))
+ (should (= (nth 1 info) 31))
+ (should (= (nth 2 info) 3))
+ (should (equal (nth 3 info) 'nrepl-error-highlight-face))
+ (should (string= (nth 4 info) "Unable to resolve symbol: dummy in this context")))))
+
+(ert-deftest test-nrepl-extract-warning-info ()
+ (let ((message "Reflection warning, /some/othertest/file/core.clj:24:43 - reference to field getCanonicalPath can't be resolved.
+"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexps-alist message)))
+ (should (string= (nth 0 info) "/some/othertest/file/core.clj"))
+ (should (= (nth 1 info) 24))
+ (should (= (nth 2 info) 43))
+ (should (equal (nth 3 info) 'nrepl-warning-highlight-face))
+ (should (string= (nth 4 info) "reference to field getCanonicalPath can't be resolved.")))))
+
@bbatsov Owner
bbatsov added a note

You should have tests covering names like NO_SOURCE_PATH and you should handle the possibility for a missing column number.

@sw1nn
sw1nn added a note

agreed. I'll add them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bbatsov
Owner

Looks better. I've listed my problems with the current state of affairs inline. What worries me the most is that the stacktraces I see in Clojure 1.4 don't have a column in them (only a line). I guess you might be using 1.5. You'll definitely want to cover the missing column info scenario.

@bbatsov
Owner

The code looks good to me at this point. Squash the two commits and I we'll be ready to go.

@sw1nn

I squashed the commits. I also changed (paredit-backward-up) to (backward-up-list) in case paredit not available.

@bbatsov
Owner

Yep, we should not use paredit function, since nrepl.el does not depend on it.

@bbatsov bbatsov commented on the diff
@@ -875,38 +887,57 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (regexp message)
@bbatsov Owner
bbatsov added a note

The indentation of the body of the function seems pretty messed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
nrepl.el
((63 lines not shown))
+ (when info
+ (let ((file (nth 0 info))
+ (line (nth 1 info))
+ (col (nth 2 info))
+ (face (nth 3 info))
+ (note (nth 4 info)))
+ (save-excursion
+ ;; when we don't have a filename the line number
+ ;; is relative to form start
+ (if file
+ (goto-char (point-min)) ; start of file
+ (beginning-of-defun))
+ (forward-line (1- line))
+ ;; if have column, highlight sexp at that point otherwise whole line.
+ (move-to-column (or col 0))
+ (let ((begin (progn (if col (paredit-backward-up) (back-to-indentation)) (point)))
@bbatsov Owner
bbatsov added a note

Didn't you say you removed paredit-backward-up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bbatsov
Owner

It seems that the last commit is breaking a few tests.

@bbatsov
Owner

@kingtim I suggest we merge this.

@BlanceXR

The stock highlight does not seem to work for me so I merged this one with mine, and it works! Thank!

However, highlight only the sexp does not make sense to me sometimes when the sexp is big, for example, (let ...) (cond ...) and so. I wonder if it is better to alter between sexp highlighting for one-line sexps and line highlighting for longer sexps.

@sw1nn

I've been using this for 2 months without incident. wrt highlight whole sexp, iirc when there's an error the compiler reports the line of the start of the sexp rather than the exact line of the error. So highlight whole sexp is the best we can do with the information currently provided by the compiler. happy to be proved wrong tho.

@bbatsov bbatsov merged commit d2e72ad into clojure-emacs:master
@bbatsov
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2013
Commits on Apr 4, 2013
  1. fix handling of missing column better

    Neale Swinnerton authored
This page is out of date. Refresh to see the latest.
Showing with 142 additions and 50 deletions.
  1. +2 −0  CHANGELOG.md
  2. +73 −39 nrepl.el
  3. +67 −11 test/nrepl-tests.el
View
2  CHANGELOG.md
@@ -4,6 +4,8 @@
### New features
+* Add highlighting of compilation warnings in addition to existing highlighting of errors
+
### Bugs fixed
## 0.1.7 / 2013-03-13
View
112 nrepl.el
@@ -165,6 +165,11 @@ buffer will be hidden.")
"Face used to highlight compilation errors in Clojure buffers."
:group 'nrepl)
+(defface nrepl-warning-highlight-face
+ '((t (:inherit font-lock-warning-face :underline (:color "yellow"))))
+ "Face used to highlight compilation warnings in Clojure buffers."
+ :group 'nrepl)
+
(defmacro nrepl-propertize-region (props &rest body)
"Add PROPS to all text inserted by executing BODY.
More precisely, PROPS are added to the region between the point's
@@ -770,7 +775,7 @@ DONE-HANDLER, and EVAL-ERROR-HANDLER as appropriate."
(nrepl-emit-interactive-output value))
(lambda (buffer err)
(message "%s" err)
- (nrepl-highlight-compilation-error-line
+ (nrepl-highlight-compilation-errors
buffer err))
'()))
@@ -787,7 +792,7 @@ DONE-HANDLER, and EVAL-ERROR-HANDLER as appropriate."
(nrepl-emit-interactive-output value))
(lambda (buffer err)
(message "%s" err)
- (nrepl-highlight-compilation-error-line
+ (nrepl-highlight-compilation-errors
buffer err))
'()
(lambda (buffer ex root-ex session)
@@ -849,10 +854,17 @@ Returns the position at which PROPERTY was found, or nil if not found."
ARG and RESET are ignored, as there is only ever one compilation error.
They exist for compatibility with `next-error'."
(interactive)
- (let ((p (or (nrepl-find-property 'nrepl-note)
- (nrepl-find-property 'nrepl-note t))))
- (when p
- (goto-char p))))
+ (flet ((goto-next-note-boundary ()
+ (let ((p (or (nrepl-find-property 'nrepl-note)
+ (nrepl-find-property 'nrepl-note t))))
+ (when p
+ (goto-char p)
+ (message (get-char-property p 'nrepl-note))))))
+ ;; if we're already on a compilation error, first jump to the end of
+ ;; it, so that we find the next error.
+ (when (get-char-property (point) 'nrepl-note)
+ (goto-next-note-boundary))
+ (goto-next-note-boundary)))
(defun nrepl-default-err-handler (buffer ex root-ex session)
"Make an error handler for BUFFER, EX, ROOT-EX and SESSION."
@@ -875,38 +887,59 @@ They exist for compatibility with `next-error'."
(with-current-buffer nrepl-error-buffer
(compilation-minor-mode +1))))))
-(defun nrepl-highlight-compilation-error-line (buffer message)
+(defun nrepl-extract-error-info (regexp message)
@bbatsov Owner
bbatsov added a note

The indentation of the body of the function seems pretty messed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ "Extract error information with REGEXP against MESSAGE."
+ (let ((file (nth 1 regexp))
+ (line (nth 2 regexp))
+ (col (nth 3 regexp))
+ (type (nth 4 regexp))
+ (pat (first regexp)))
+ (when (string-match pat message)
+ ;; special processing for type (1.2) style
+ (setq type (if (consp type)
+ (or (and (first type) (match-end (first type)) 1)
+ (and (rest type) (match-end (rest type)) 0)
+ 2)))
+ (list
+ (when file
+ (let ((val (match-string-no-properties file message)))
+ (unless (string= val "NO_SOURCE_PATH") val)))
+ (when line (string-to-number (match-string-no-properties line message)))
+ (when col
+ (let ((val (match-string-no-properties col message)))
+ (when val (string-to-number val))))
+ (aref [nrepl-warning-highlight-face
+ nrepl-warning-highlight-face
+ nrepl-error-highlight-face]
+ (or type 2))
+ (when message)))))
+
+(defun nrepl-highlight-compilation-errors (buffer message)
"Highlight compilation error line in BUFFER, using MESSAGE."
(with-current-buffer buffer
- (let ((error-line-number (nrepl-extract-error-line message))
- (error-filename (nrepl-extract-error-filename message)))
- (when (and (> error-line-number 0)
- (or (string= (buffer-file-name buffer) error-filename)
- (string= "NO_SOURCE_PATH" error-filename)))
- (save-excursion
- ;; when we don't have a filename the line number
- ;; is relative to form start
- (if (string= error-filename "NO_SOURCE_PATH")
- (beginning-of-defun)
- ;; else we go to the top of the file
- (goto-char (point-min)))
- (forward-line (1- error-line-number))
- (let ((overlay (make-overlay (progn (back-to-indentation) (point))
- (progn (move-end-of-line nil) (point)))))
- (overlay-put overlay 'nrepl-note-p t)
- (overlay-put overlay 'face 'nrepl-error-highlight-face)
- (overlay-put overlay 'nrepl-note message)
- (overlay-put overlay 'help-echo message)))))))
-
-(defun nrepl-extract-error-line (stacktrace)
- "Extract the error line number from STACKTRACE."
- (string-match "compiling:(.+:\\([0-9]+\\)" stacktrace)
- (string-to-number (match-string 1 stacktrace)))
-
-(defun nrepl-extract-error-filename (stacktrace)
- "Extract the error filename from STACKTRACE."
- (string-match "compiling:(\\(.+\\):" stacktrace)
- (substring-no-properties (match-string 1 stacktrace)))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (when info
+ (let ((file (nth 0 info))
+ (line (nth 1 info))
+ (col (nth 2 info))
+ (face (nth 3 info))
+ (note (nth 4 info)))
+ (save-excursion
+ ;; when we don't have a filename the line number
+ ;; is relative to form start
+ (if file
+ (goto-char (point-min)) ; start of file
+ (beginning-of-defun))
+ (forward-line (1- line))
+ ;; if have column, highlight sexp at that point otherwise whole line.
+ (move-to-column (or col 0))
+ (let ((begin (progn (if col (backward-up-list) (back-to-indentation)) (point)))
+ (end (progn (if col (forward-sexp) (move-end-of-line nil)) (point))))
+ (let ((overlay (make-overlay begin end)))
+ (overlay-put overlay 'nrepl-note-p t)
+ (overlay-put overlay 'face face)
+ (overlay-put overlay 'nrepl-note note)
+ (overlay-put overlay 'help-echo note)))))))))
(defun nrepl-need-input (buffer)
"Handle an need-input request from BUFFER."
@@ -1561,12 +1594,13 @@ Useful in hooks."
Useful in hooks."
(nrepl-interaction-mode -1))
-(defvar nrepl-compilation-regexps '("(\\([^)]+\\):\\([[:digit:]]+\\))" 1 2)
- "Specifications for matching errors in Clojure stacktraces.
+(defvar nrepl-compilation-regexp
+ '("\\(?:.*\\(warning, \\)\\|.*?\\(, compiling\\):(\\)\\([^:]*\\):\\([[:digit:]]+\\)\\(?::\\([[:digit:]]+\\)\\)?\\(\\(?: - \\(.*\\)\\)\\|)\\)" 3 4 5 (1))
+ "Specifications for matching errors and warnings in Clojure stacktraces.
See `compilation-error-regexp-alist' for help on their format.")
(add-to-list 'compilation-error-regexp-alist-alist
- (cons 'nrepl nrepl-compilation-regexps))
+ (cons 'nrepl nrepl-compilation-regexp))
(add-to-list 'compilation-error-regexp-alist 'nrepl)
;;;###autoload
View
78 test/nrepl-tests.el
@@ -135,17 +135,73 @@
(let ((nrepl-hide-special-buffers t))
(should (equal (nrepl-server-buffer-name) " *nrepl-server*"))))
-(ert-deftest test-nrepl-extract-error-line ()
- (let ((st "this context, compiling:(/some/test/file.clj:20) ala bala"))
- (should (= (nrepl-extract-error-line st) 20)))
- (let ((st "this context, compiling:(NO_SOURCE_PATH:20) ala bala"))
- (should (= (nrepl-extract-error-line st) 20))))
-
-(ert-deftest test-nrepl-extract-error-filename ()
- (let ((st "this context, compiling:(/some/test/file.clj:20) ala bala"))
- (should (string= (nrepl-extract-error-filename st) "/some/test/file.clj")))
- (let ((st "this context, compiling:(NO_SOURCE_PATH:20) ala bala"))
- (should (string= (nrepl-extract-error-filename st) "NO_SOURCE_PATH"))))
+(ert-deftest test-nrepl-extract-error-info-14 ()
+ (let ((message "CompilerException java.lang.RuntimeException: Unable to resolve symbol: dummy in this context, compiling:(/some/test/file/core.clj:31)"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (string= (nth 0 info) "/some/test/file/core.clj"))
+ (should (= (nth 1 info) 31))
+ (should (equal (nth 2 info) nil))
+ (should (equal (nth 3 info) 'nrepl-error-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-error-info-14-no-file ()
+ (let ((message "CompilerException java.lang.RuntimeException: Unable to resolve symbol: dummy in this context, compiling:(NO_SOURCE_PATH:31)"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (equal (nth 0 info) nil))
+ (should (= (nth 1 info) 31))
+ (should (equal (nth 2 info) nil))
+ (should (equal (nth 3 info) 'nrepl-error-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-warning-info-14 ()
+ (let ((message "Reflection warning, /some/othertest/file/core.clj:24 - reference to field getCanonicalPath can't be resolved.
+"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (string= (nth 0 info) "/some/othertest/file/core.clj"))
+ (should (= (nth 1 info) 24))
+ (should (equal (nth 2 info) nil))
+ (should (equal (nth 3 info) 'nrepl-warning-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-warning-info-14-no-file ()
+ (let ((message "Reflection warning, NO_SOURCE_PATH:24 - reference to field getCanonicalPath can't be resolved.
+"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (equal (nth 0 info) nil))
+ (should (= (nth 1 info) 24))
+ (should (equal (nth 2 info) nil))
+ (should (equal (nth 3 info) 'nrepl-warning-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-error-info-15 ()
+ (let ((message "CompilerException java.lang.RuntimeException: Unable to resolve symbol: dummy in this context, compiling:(/some/test/file/core.clj:31:3)"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (string= (nth 0 info) "/some/test/file/core.clj"))
+ (should (= (nth 1 info) 31))
+ (should (= (nth 2 info) 3))
+ (should (equal (nth 3 info) 'nrepl-error-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-error-info-15-no-file ()
+ (let ((message "CompilerException java.lang.RuntimeException: Unable to resolve symbol: dummy in this context, compiling:(NO_SOURCE_PATH:31:3)"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (equal (nth 0 info) nil))
+ (should (= (nth 1 info) 31))
+ (should (= (nth 2 info) 3))
+ (should (equal (nth 3 info) 'nrepl-error-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-warning-info-15 ()
+ (let ((message "Reflection warning, /some/othertest/file/core.clj:24:43 - reference to field getCanonicalPath can't be resolved.
+"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (string= (nth 0 info) "/some/othertest/file/core.clj"))
+ (should (= (nth 1 info) 24))
+ (should (= (nth 2 info) 43))
+ (should (equal (nth 3 info) 'nrepl-warning-highlight-face)))))
+
+(ert-deftest test-nrepl-extract-warning-info-15-no-file ()
+ (let ((message "Reflection warning, NO_SOURCE_PATH:24:43 - reference to field getCanonicalPath can't be resolved.
+"))
+ (let ((info (nrepl-extract-error-info nrepl-compilation-regexp message)))
+ (should (equal (nth 0 info) nil))
+ (should (= (nth 1 info) 24))
+ (should (= (nth 2 info) 43))
+ (should (equal (nth 3 info) 'nrepl-warning-highlight-face)))))
(defmacro nrepl-test-with-two-buffers (buffer-names &rest body)
(lexical-let ((create (lambda (b) (list b `(generate-new-buffer " *temp*")))))
Something went wrong with that request. Please try again.