1.0.46.4: redefinition warnings for macros

  Similar logic as when not to warn as DEFUN has.

  Also refactor the existing redefinition conditions and
  uninterestingness-tests a bit.

diff --git a/NEWS b/NEWS
index ed8aad6..7cb8194 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,7 @@
 ;;;; -*- coding: utf-8; fill-column: 78 -*-
 changes relative to sbcl-1.0.46:
   * enhancement: --script muffles style-warnings and compiler notes. (lp#677779)
+  * enhancement: redefinition warnings for macros from different files. (lp#434657)
   * bug fix: SB-DEBUG:BACKTRACE-AS-LIST guards against potentially leaking
     stack-allocated values out of their dynamic-extent. (lp#310175)
   * bug fix: attempts to use SB-SPROF for wallclock profiling on threaded

diff --git a/src/code/condition.lisp b/src/code/condition.lisp
index 53f6b18..a2d690a 100644 (file)
--- a/src/code/condition.lisp
+++ b/src/code/condition.lisp
@@ -1346,42 +1346,41 @@ handled by any other handler, it will be muffled.")
 ;; redefinitions, but other redefinitions could be done later
 ;; (e.g. methods).
 (define-condition redefinition-warning (style-warning)
-  ())
+  ((name
+    :initarg :name
+    :reader redefinition-warning-name)
+   (new-location
+    :initarg :new-location
+    :reader redefinition-warning-new-location)))
 
 (define-condition function-redefinition-warning (redefinition-warning)
-  ((name :initarg :name :reader function-redefinition-warning-name)
-   (old :initarg :old :reader function-redefinition-warning-old-fdefinition)
-   ;; For DEFGENERIC and perhaps others, the redefinition
-   ;; destructively modifies the original, rather than storing a new
-   ;; object, so there's no NEW here, but only in subclasses.
-   ))
+  ((new-function
+    :initarg :new-function
+    :reader function-redefinition-warning-new-function)))
 
 (define-condition redefinition-with-defun (function-redefinition-warning)
-  ((new :initarg :new :reader redefinition-with-defun-new-fdefinition)
-   ;; KLUDGE: it would be nice to fix the unreasonably late
-   ;; back-patching of DEBUG-SOURCEs in the DEBUG-INFO during
-   ;; fasloading and just use the new fdefinition, but for the moment
-   ;; we'll compare the SOURCE-LOCATION created during DEFUN with the
-   ;; previous DEBUG-SOURCE.
-   (new-location :initarg :new-location
-              :reader redefinition-with-defun-new-location))
+  ()
   (:report (lambda (warning stream)
              (format stream "redefining ~/sb-impl::print-symbol-with-prefix/ ~
                              in DEFUN"
-                     (function-redefinition-warning-name warning)))))
+                     (redefinition-warning-name warning)))))
 
-(define-condition redefinition-with-defgeneric (function-redefinition-warning)
-  ((new-location :initarg :new-location
-                 :reader redefinition-with-defgeneric-new-location))
+(define-condition redefinition-with-defmacro (function-redefinition-warning)
+  ()
+  (:report (lambda (warning stream)
+             (format stream "redefining ~/sb-impl::print-symbol-with-prefix/ ~
+                             in DEFMACRO"
+                     (redefinition-warning-name warning)))))
+
+(define-condition redefinition-with-defgeneric (redefinition-warning)
+  ()
   (:report (lambda (warning stream)
              (format stream "redefining ~/sb-impl::print-symbol-with-prefix/ ~
                              in DEFGENERIC"
-                     (function-redefinition-warning-name warning)))))
+                     (redefinition-warning-name warning)))))
 
 (define-condition redefinition-with-defmethod (redefinition-warning)
-  ((gf :initarg :generic-function
-       :reader redefinition-with-defmethod-generic-function)
-   (qualifiers :initarg :qualifiers
+  ((qualifiers :initarg :qualifiers
                :reader redefinition-with-defmethod-qualifiers)
    (specializers :initarg :specializers
                  :reader redefinition-with-defmethod-specializers)
@@ -1391,135 +1390,103 @@ handled by any other handler, it will be muffled.")
                :reader redefinition-with-defmethod-old-method))
   (:report (lambda (warning stream)
              (format stream "redefining ~S~{ ~S~} ~S in DEFMETHOD"
-                     (redefinition-with-defmethod-generic-function warning)
+                     (redefinition-warning-name warning)
                      (redefinition-with-defmethod-qualifiers warning)
                      (redefinition-with-defmethod-specializers warning)))))
 
-;; FIXME: see the FIXMEs in defmacro.lisp, then maybe instantiate this.
-(define-condition redefinition-with-defmacro (function-redefinition-warning)
-  ())
+;;;; Deciding which redefinitions are "interesting".
+
+(defun function-file-namestring (function)
+  #!+sb-eval
+  (when (typep function 'sb!eval:interpreted-function)
+    (return-from function-file-namestring
+      (sb!c:definition-source-location-namestring
+          (sb!eval:interpreted-function-source-location function))))
+  (let* ((fun (sb!kernel:%fun-fun function))
+         (code (sb!kernel:fun-code-header fun))
+         (debug-info (sb!kernel:%code-debug-info code))
+         (debug-source (when debug-info
+                         (sb!c::debug-info-source debug-info)))
+         (namestring (when debug-source
+                       (sb!c::debug-source-namestring debug-source))))
+    namestring))
+
+(defun interesting-function-redefinition-warning-p (warning old)
+  (let ((new (function-redefinition-warning-new-function warning))
+        (source-location (redefinition-warning-new-location warning)))
+    (or
+     ;; Compiled->Interpreted is interesting.
+     (and (typep old 'compiled-function)
+          (typep new '(not compiled-function)))
+     ;; FIN->Regular is interesting.
+     (and (typep old 'funcallable-instance)
+          (typep new '(not funcallable-instance)))
+     ;; Different file or unknown location is interesting.
+     (let* ((old-namestring (function-file-namestring old))
+            (new-namestring
+             (or (function-file-namestring new)
+                 (when source-location
+                   (sb!c::definition-source-location-namestring source-location)))))
+       (and (or (not old-namestring)
+                (not new-namestring)
+                (not (string= old-namestring new-namestring))))))))
 
-;; Here are a few predicates for what people might find interesting
-;; about redefinitions.
-
-;; DEFUN can replace a generic function with an ordinary function.
-;; (Attempting to replace an ordinary function with a generic one
-;; causes an error, though.)
-(defun redefinition-replaces-generic-function-p (warning)
-  (and (typep warning 'redefinition-with-defun)
-       (typep (function-redefinition-warning-old-fdefinition warning)
-              'generic-function)))
-
-(defun redefinition-replaces-compiled-function-with-interpreted-p (warning)
-  (and (typep warning 'redefinition-with-defun)
-       (compiled-function-p
-        (function-redefinition-warning-old-fdefinition warning))
-       (not (compiled-function-p
-             (redefinition-with-defun-new-fdefinition warning)))))
-
-;; Most people seem to agree that re-running a DEFUN in a file is
-;; completely uninteresting.
 (defun uninteresting-ordinary-function-redefinition-p (warning)
-  ;; OAOO violation: this duplicates code in SB-INTROSPECT.
-  ;; Additionally, there are some functions that aren't
-  ;; funcallable-instances for which finding the source location is
-  ;; complicated (e.g. DEFSTRUCT-defined predicates and accessors),
-  ;; but I don't think they're defined with %DEFUN, so the warning
-  ;; isn't raised.
-  (flet ((fdefinition-file-namestring (fdefn)
-           #!+sb-eval
-           (when (typep fdefn 'sb!eval:interpreted-function)
-             (return-from fdefinition-file-namestring
-               (sb!c:definition-source-location-namestring
-                   (sb!eval:interpreted-function-source-location fdefn))))
-           ;; All the following accesses are guarded with conditionals
-           ;; because it's not clear whether any of the slots we're
-           ;; chasing down are guaranteed to be filled in.
-           (let* ((fdefn
-                   ;; KLUDGE: although this looks like it only works
-                   ;; for %SIMPLE-FUNs, in fact there's a pun such
-                   ;; that %SIMPLE-FUN-SELF returns the simple-fun
-                   ;; object for closures and
-                   ;; funcallable-instances. -- CSR, circa 2005
-                   (sb!kernel:%simple-fun-self fdefn))
-                  (code (if fdefn (sb!kernel:fun-code-header fdefn)))
-                  (debug-info (if code (sb!kernel:%code-debug-info code)))
-                  (debug-source (if debug-info
-                                    (sb!c::debug-info-source debug-info)))
-                  (namestring (if debug-source
-                                  (sb!c::debug-source-namestring debug-source))))
-             namestring)))
-    (and
-     ;; There's garbage in various places when the first DEFUN runs in
-     ;; cold-init.
-     sb!kernel::*cold-init-complete-p*
-     (typep warning 'redefinition-with-defun)
-     (let ((old-fdefn
-            (function-redefinition-warning-old-fdefinition warning))
-           (new-fdefn
-            (redefinition-with-defun-new-fdefinition warning)))
-       ;; Replacing a compiled function with a compiled function is
-       ;; clearly uninteresting, and we'll say arbitrarily that
-       ;; replacing an interpreted function with an interpreted
-       ;; function is uninteresting, too, but leave out the
-       ;; compiled-to-interpreted case.
-       (when (or (typep
-                  old-fdefn
-                  '(or #!+sb-eval sb!eval:interpreted-function))
-                 (and (typep old-fdefn
-                             '(and compiled-function
-                               (not funcallable-instance)))
-                      ;; Since this is a REDEFINITION-WITH-DEFUN,
-                      ;; NEW-FDEFN can't be a FUNCALLABLE-INSTANCE.
-                      (typep new-fdefn 'compiled-function)))
-         (let* ((old-namestring (fdefinition-file-namestring old-fdefn))
-                (new-namestring
-                 (or (fdefinition-file-namestring new-fdefn)
-                     (let ((srcloc
-                            (redefinition-with-defun-new-location warning)))
-                       (if srcloc
-                            (sb!c::definition-source-location-namestring
-                                srcloc))))))
-           (and old-namestring
-                new-namestring
-                (equal old-namestring new-namestring))))))))
+  (and
+   ;; There's garbage in various places when the first DEFUN runs in
+   ;; cold-init.
+   sb!kernel::*cold-init-complete-p*
+   (typep warning 'redefinition-with-defun)
+   ;; Shared logic.
+   (let ((name (redefinition-warning-name warning)))
+     (not (interesting-function-redefinition-warning-p
+           warning (or (fdefinition name) (macro-function name)))))))
+
+(defun uninteresting-macro-redefinition-p (warning)
+  (and
+   (typep warning 'redefinition-with-defmacro)
+   ;; Shared logic.
+   (let ((name (redefinition-warning-name warning)))
+     (not (interesting-function-redefinition-warning-p
+           warning (or (macro-function name) (fdefinition name)))))))
 
 (defun uninteresting-generic-function-redefinition-p (warning)
-  (and (typep warning 'redefinition-with-defgeneric)
-       (let* ((old-fdefn
-               (function-redefinition-warning-old-fdefinition warning))
-              (old-location
-               (if (typep old-fdefn 'generic-function)
-                   (sb!pcl::definition-source old-fdefn)))
-              (old-namestring
-               (if old-location
-                   (sb!c:definition-source-location-namestring old-location)))
-              (new-location
-               (redefinition-with-defgeneric-new-location warning))
-              (new-namestring
-               (if new-location
-                   (sb!c:definition-source-location-namestring new-location))))
-         (and old-namestring
-              new-namestring
-              (equal old-namestring new-namestring)))))
+  (and
+   (typep warning 'redefinition-with-defgeneric)
+   ;; Can't use the shared logic above, since GF's don't get a "new"
+   ;; definition -- rather the FIN-FUNCTION is set.
+   (let* ((name (redefinition-warning-name warning))
+          (old (fdefinition name))
+          (old-location (when (typep old 'generic-function)
+                          (sb!pcl::definition-source old)))
+          (old-namestring (when old-location
+                            (sb!c:definition-source-location-namestring old-location)))
+          (new-location (redefinition-warning-new-location warning))
+          (new-namestring (when new-location
+                           (sb!c:definition-source-location-namestring new-location))))
+     (and old-namestring
+          new-namestring
+          (string= old-namestring new-namestring)))))
 
 (defun uninteresting-method-redefinition-p (warning)
-  (and (typep warning 'redefinition-with-defmethod)
-       (let* ((old-method (redefinition-with-defmethod-old-method warning))
-              (old-location (sb!pcl::definition-source old-method))
-              (old-namestring (if old-location
-                                  (sb!c:definition-source-location-namestring
-                                      old-location)))
-              (new-location (redefinition-with-defmethod-new-location warning))
-              (new-namestring (if new-location
-                                  (sb!c:definition-source-location-namestring
-                                      new-location))))
+  (and
+   (typep warning 'redefinition-with-defmethod)
+   ;; Can't use the shared logic above, since GF's don't get a "new"
+   ;; definition -- rather the FIN-FUNCTION is set.
+   (let* ((old-method (redefinition-with-defmethod-old-method warning))
+          (old-location (sb!pcl::definition-source old-method))
+          (old-namestring (when old-location
+                            (sb!c:definition-source-location-namestring old-location)))
+          (new-location (redefinition-warning-new-location warning))
+          (new-namestring (when new-location
+                            (sb!c:definition-source-location-namestring new-location))))
          (and new-namestring
               old-namestring
-              (equal new-namestring old-namestring)))))
+              (string= new-namestring old-namestring)))))
 
 (deftype uninteresting-redefinition ()
   '(or (satisfies uninteresting-ordinary-function-redefinition-p)
+       (satisfies uninteresting-macro-redefinition-p)
        (satisfies uninteresting-generic-function-redefinition-p)
        (satisfies uninteresting-method-redefinition-p)))
 

diff --git a/src/code/defboot.lisp b/src/code/defboot.lisp
index a083276..c7113c4 100644 (file)
--- a/src/code/defboot.lisp
+++ b/src/code/defboot.lisp
@@ -224,9 +224,10 @@ evaluated as a PROGN."
   (sb!c:%compiler-defun name inline-lambda nil)
   (when (fboundp name)
     (/show0 "redefining NAME in %DEFUN")
-    (style-warn 'sb!kernel::redefinition-with-defun :name name
-                :old (fdefinition name) :new def
-                :new-location source-location))
+    (warn 'sb!kernel::redefinition-with-defun
+          :name name
+          :new-function def
+          :new-location source-location))
   (setf (sb!xc:fdefinition name) def)
 
   (sb!c::note-name-defined name :function)

diff --git a/src/code/defmacro.lisp b/src/code/defmacro.lisp
index 033cc58..92fbb48 100644 (file)
--- a/src/code/defmacro.lisp
+++ b/src/code/defmacro.lisp
@@ -48,14 +48,16 @@
                       ,@local-decs
                       ,new-body))
               (debug-name (sb!c::debug-name 'macro-function name)))
-          `(eval-when (:compile-toplevel :load-toplevel :execute)
-             (sb!c::%defmacro ',name #',def ',lambda-list
-                              ,doc ',debug-name)))))))
+          `(progn
+             (eval-when (:compile-toplevel :load-toplevel :execute)
+               (sb!c::%defmacro ',name #',def ',lambda-list ,doc ',debug-name
+                                (sb!c:source-location)))))))))
 
 (macrolet
     ((def (times set-p)
        `(eval-when (,@times)
-          (defun sb!c::%defmacro (name definition lambda-list doc debug-name)
+          (defun sb!c::%defmacro (name definition lambda-list doc debug-name
+                                  source-location)
             ;; old note (ca. 1985, maybe:-): "Eventually %%DEFMACRO
             ;; should deal with clearing old compiler information for
             ;; the functional value."
@@ -74,23 +76,16 @@
                    name (info :function :where-from name))
                   (undefine-fun-name name))
                 (clear-info :function :where-from name)
-
-               ;; FIXME: It would be nice to warn about DEFMACRO of an
-               ;; already-defined macro, but that's slightly hard to do
-               ;; because in common usage DEFMACRO is defined at compile
-               ;; time and then redefined at load time. We'd need to make a
-               ;; distinction between the defined-at-compile-time state and
-               ;; the defined-at-load-time state to make this work. (Trying
-               ;; to warn about duplicate DEFTYPEs runs into the same
-               ;; problem.)
-               #+nil
-               (when (sb!xc:macro-function name)
-                 ;; Someday we could check for macro arguments
-                 ;; being incompatibly redefined. Doing this right
-                 ;; will involve finding the old macro lambda-list
-                 ;; and comparing it with the new one.
-                 (style-warn "redefining ~/sb-impl::print-symbol-with-prefix/ ~
-                                 in DEFMACRO" name))
+                #-sb-xc-host
+                (when (fboundp name)
+                  ;; Someday we could check for macro arguments
+                  ;; being incompatibly redefined. Doing this right
+                  ;; will involve finding the old macro lambda-list
+                  ;; and comparing it with the new one.
+                  (warn 'sb!kernel::redefinition-with-defmacro
+                        :name name
+                        :new-function definition
+                        :new-location source-location))
                (setf (sb!xc:macro-function name) definition)
                ,(when set-p
                       `(setf (%fun-doc definition) doc

diff --git a/src/pcl/boot.lisp b/src/pcl/boot.lisp
index d77709b..3c9fe4f 100644 (file)
--- a/src/pcl/boot.lisp
+++ b/src/pcl/boot.lisp
@@ -252,9 +252,10 @@ bootstrapping.
 
 (defun load-defgeneric (fun-name lambda-list source-location &rest initargs)
   (when (fboundp fun-name)
+    (warn 'sb-kernel:redefinition-with-defgeneric
+          :name fun-name
+          :new-location source-location)
     (let ((fun (fdefinition fun-name)))
-      (warn 'sb-kernel:redefinition-with-defgeneric :name fun-name
-            :old fun :new-location source-location)
       (when (generic-function-p fun)
         (loop for method in (generic-function-initial-methods fun)
               do (remove-method fun method))
@@ -1562,10 +1563,11 @@ bootstrapping.
                         (generic-function-methods gf)
                         (find-method gf qualifiers specializers nil))))
       (when method
-        (style-warn 'sb-kernel:redefinition-with-defmethod
-                    :generic-function gf-spec :old-method method
-                    :qualifiers qualifiers :specializers specializers
-                    :new-location source-location))))
+        (warn 'sb-kernel:redefinition-with-defmethod
+              :name gf-spec
+              :new-location source-location
+              :old-method method
+              :qualifiers qualifiers :specializers specializers))))
   (let ((method (apply #'add-named-method
                        gf-spec qualifiers specializers lambda-list
                        :definition-source source-location

diff --git a/version.lisp-expr b/version.lisp-expr
index 46c146e..3445334 100644 (file)
--- a/version.lisp-expr
+++ b/version.lisp-expr
@@ -20,4 +20,4 @@
 ;;; checkins which aren't released. (And occasionally for internal
 ;;; versions, especially for internal versions off the main CVS
 ;;; branch, it gets hairier, e.g. "0.pre7.14.flaky4.13".)
-"1.0.46.3"
+"1.0.46.4"