Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve recording of source information for most kinds of definitions <http://abcl.org/trac/ticket/415> #5

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
2 participants
@alanruttenberg
Copy link
Collaborator

commented Oct 30, 2016

e.g.

(describe 'split-at-char)
SPLIT-AT-CHAR is an internal symbol in the COMMON-LISP-USER package.
Its function binding is #<SPLIT-AT-CHAR {4D969FD4}>.
The function's lambda list is:
  (STRING CHAR)
The symbol's property list contains these indicator/value pairs:
  SYSTEM::%SOURCE-BY-TYPE ((:COMPILER-MACRO "/Users/alanr/repos/lsw2/util/string.lisp" 1039)
                           ((:FUNCTION SPLIT-AT-CHAR) "/Users/alanr/repos/lsw2/util/string.lisp" 687))
  SYSTEM::%SOURCE (#P"/Users/lori/repos/lsw2/util/string.lisp" . 687)

I followed the source types in slime's swank/sbcl.lisp

This is preliminary to adding support for using this information in slime.

What should be recorded:

  • packages
  • classes
  • functions
  • macros
  • compiler-macros
  • setf-expanders
  • methods
  • conditions
  • structures
  • types
  • source-transforms
  • variables
  • constants

Implementation

fdefinition.lisp has the function record-source-information-for-type For interactive evaluation it does the work. For file compiling there are additions to compile-file.lisp that add forms after the prologue that add the information when fasl loading, during which recording by record-source-information-for-type is disabled, so it doesn't record the source positions in the fasl header.

As you see, I haven't got rid of the now redundant sys:%source property. Once slime has been adjusted it might be worth doing so.

Right now the absolute pathnames are recorded, but it might make sense to use logical pathnames if there were ones set up, though I expect the slime support with do some dwimming in any case.

@easye

This comment has been minimized.

Copy link
Collaborator

commented Oct 30, 2016

@alanruttenberg: As I understand your intention after this patch is finished, we will have two properties on symbols with associated source:

Symbol Contents
SYSTEM::%SOURCE (PATHNAME . SOURCE-LINE)
SYSTEM::%SOURCE-BY-TYPE ((KEYWORD-FOR-TYPE-OR-FUNCTION-INFO PATHNAME SOURCE-LINE) …))

where KEYWORD-FOR-TYPE-OR-FUNCTION-INFO would be one of :CONDITION, :VARIABLE, :KEYWORD, :MACRO, :COMPILER-MACRO, :PACKAGE, :DEFSTRUCT, :SETF-EXPANDER, :CONSTANT, :SOURCE-TRANSFORM and for both functions and generic functions a list of the form (:FUNCTION name-of-function)

Once the code works well enough for SLIME, we would no longer need SYSTEM::%SOURCE (assuming that no one else consumes such symbol properties, which is a bit of a stretch, so I would go for an announced deprecation route for abcl-1.6.0 or something).

  1. Shouldn't we take the opportunity to export and normalize the property list key we are using?

Instead of SYSTEM::%SOURCE-BY-TYPE let's move to SYSTEM:SOURCE, which will be the standard interface going forwards.

  1. Uncomment code for arglist?

Why does your code comment out the call to SET-ARGLIST in the precompiler? Does this need additional work, or an oversight on your part?

  1. Inconsistent KEYWORD-FOR-TYPE with DEFMETHOD

Why do functions need to record their names with a cons in place of a simple keyword (probably something basic than I am not getting)?

Shouldn't we distinguish between DEFUN, DEFMETHOD, and DEFGENERIC definitions for functions?

i.e. shouldn't we just replace the (:FUNCTION MOP:ADD-DIRECT-METHOD) with :GENERIC-FUNCTION?

(get 'mop:add-direct-method 'system::%source-by-type)
(((:FUNCTION MOP:ADD-DIRECT-METHOD)
  "/Users/evenson/work/abcl.git/src/org/armedbear/lisp/clos.lisp"
  100215))

If you agree on the symbol property key being 'SYSTEM:SOURCE', and can explain the bit about function source location using a cons to record its type, I'd go for merging this as a work in progress.

@alanruttenberg

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 30, 2016

I had some trouble building with the arglist being set there. I think it is set elsewhere. Try getting arglists in the new build.


mop:add-direct-method is at least defined with defun at that location. It is later defined using atomic-defgeneric and looking at the expansion of that I can see that it is doing some funny business. It will need to be special-cased. It defines the generic function on a gensym.

;;; To be redefined as generic functions later
(defun add-direct-method (specializer method) ...

(describe 'print-object) to see methods and generic-functions recorded

  ((:METHOD PRINT-OBJECT NIL (STANDARD-OBJECT T))
   "/Users/lori/repos/abcl/src/org/armedbear/lisp/print-object.lisp" 1878)
  ((:METHOD PRINT-OBJECT NIL (T T))
   "/Users/lori/repos/abcl/src/org/armedbear/lisp/print-object.lisp" 1715)
  ((:GENERIC-FUNCTION PRINT-OBJECT)
   "/Users/lori/repos/abcl/src/org/armedbear/lisp/print-object.lisp" 1672))

Functions are defined as cons because sometimes they are the symbol and sometimes they are the (setf xxx). In the latter case the source information is also kept on xxx.

e.g.

(defun (setf jfield) (newvalue class-ref-or-field field-or-instance
              &optional (instance nil instance-supplied-p) unused-value)
  (declare (ignore unused-value))
  (if instance-supplied-p
      (jfield class-ref-or-field field-or-instance instance newvalue)
      (jfield class-ref-or-field field-or-instance nil newvalue)))

:defstruct should be :structure. I'm trying to follow this bit in swank/sbcl.lisp. Will fix.

(defparameter *definition-types*
  '(:variable defvar
    :constant defconstant
    :type deftype
    :symbol-macro define-symbol-macro
    :macro defmacro
    :compiler-macro define-compiler-macro
    :function defun
    :generic-function defgeneric
    :method defmethod
    :setf-expander define-setf-expander
    :structure defstruct
    :condition define-condition
    :class defclass
    :method-combination define-method-combination
    :package defpackage
    :transform :deftransform
    :optimizer :defoptimizer
    :vop :define-vop
    :source-transform :define-source-transform
    :ir1-convert :def-ir1-translator
    :declaration declaim
    :alien-type :define-alien-type)
  "Map SB-INTROSPECT definition type names to Slime-friendly forms")

I'm fine with system:source

Fix atomic-defgeneric. Use sys:source vs. sys::%source-by-type. Recor…
…d source for methods defined in defgeneric. Fix some cases where file-compile recorded source but load didn't
@alanruttenberg

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 30, 2016

Committed changes to address some issues discussed.

@alanruttenberg alanruttenberg force-pushed the alanruttenberg:doc branch from 6970522 to 2cfbbbf Nov 4, 2016

@alanruttenberg

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 4, 2016

squashed commits and added better doc to commit message.

record source file information about many kinds of things. Stored on
property sys:source.

Implementation

record-source-information-by-type (name type &optional source-pathname source-position)

type is either a symbol or list.

Functions, methods, and generic functions are lists:

(:generic-function function-name)
(:function function-name)
(:method method-name qualifiers specializers)

function-name or method name can be a 'symbol or '(setf symbol)

Other types are just symbols, and follow sbcl's in slime
(https://github.com/slime/slime/blob/bad2acf672c33b913aabc1a7facb9c3c16a4afe9/swank/sbcl.lisp#L748)

:class, :variable, :condition, :constant, :compiler-macro, :macro,
:package, :structure, :type, :setf-expander, :source-transform

Modifications are in two places, one at the definitions, calling
record-source-information-by-type and then again in the file-compiler,
which writes forms like

(put 'source name (cons (list type pathname position) (get 'source name)))

In theory this can lead to redundancy if a fasl is loaded again and
again. I'm not sure how to fix this yet. Forms in the __loader__ get
called early in build when many of the sequence functions aren't
present.  Will probably just filter when presenting in slime.

@alanruttenberg alanruttenberg force-pushed the alanruttenberg:doc branch from 2cfbbbf to ae6a16a Nov 5, 2016

@easye

This comment has been minimized.

Copy link
Collaborator

commented Nov 12, 2016

Still testing; and thinking. Apologies.

@easye easye changed the title This patch adds recording of source information for most kinds of definitions Improve recording of source information for most kinds of definitions Nov 17, 2016

@easye easye changed the title Improve recording of source information for most kinds of definitions Improve recording of source information for most kinds of definitions <http://abcl.org/trac/ticket/415> Nov 21, 2016

easye pushed a commit that referenced this pull request Nov 24, 2016

mevenson@1c010e3e-69d0-11dd-93a8-456734b0d56f
Dramatically improve source recording on SYS::SOURCE plist for a symb…
…ol (Alan Ruttenberg)

The interface to recording information on the SYS:%SOURCE plist for a
symbol is now deprecated and will be removed with abcl-1.7.

Implementation
--------------

Source information for ABCL is now recorded on the SYS::SOURCE
property.  The appropiate information for type is recorded by the
SYS::RECORD-SOURCE-INFORMATION-BY-TYPE function:

    record-source-information-by-type (name type &optional source-pathname source-position)

TYPE is either a symbol or list.

Source information for functions, methods, and generic functions are
represented as lists of the following form:

    (:generic-function function-name)
    (:function function-name)
    (:method method-name qualifiers specializers)

Where FUNCTION-NAME or METHOD-NAME can be a either be of the form
'symbol or '(setf symbol).

Source information for all other forms have a symbol for TYPE which is
one of the following:

:class, :variable, :condition, :constant, :compiler-macro, :macro
:package, :structure, :type, :setf-expander, :source-transform

These values follow SBCL'S implemenation in SLIME
c.f. <https://github.com/slime/slime/blob/bad2acf672c33b913aabc1a7facb9c3c16a4afe9/swank/sbcl.lisp#L748>

Modifications are in two places, one at the definitions, calling
record-source-information-by-type and then again in the file-compiler,
which writes forms like

    (put 'source name (cons (list type pathname position) (get 'source name)))

In theory this can lead to redundancy if a fasl is loaded again and
again. I'm not sure how to fix this yet. Forms in the __loader__ get
called early in build when many of the sequence functions aren't
present.  Will probably just filter when presenting in slime.

<> :closes <http://abcl.org/trac/ticket/421> .
<> :merges <#5> .
@easye

This comment has been minimized.

Copy link
Collaborator

commented Nov 24, 2016

Merged with commits through 9e27334

@easye easye closed this Nov 24, 2016

svn2github pushed a commit to svn2github/abcl that referenced this pull request Nov 24, 2016

mevenson
Dramatically improve source recording on SYS::SOURCE plist for a symb…
…ol (Alan Ruttenberg)

The interface to recording information on the SYS:%SOURCE plist for a
symbol is now deprecated and will be removed with abcl-1.7.

Implementation
--------------

Source information for ABCL is now recorded on the SYS::SOURCE
property.  The appropiate information for type is recorded by the
SYS::RECORD-SOURCE-INFORMATION-BY-TYPE function:

    record-source-information-by-type (name type &optional source-pathname source-position)

TYPE is either a symbol or list.

Source information for functions, methods, and generic functions are
represented as lists of the following form:

    (:generic-function function-name)
    (:function function-name)
    (:method method-name qualifiers specializers)

Where FUNCTION-NAME or METHOD-NAME can be a either be of the form
'symbol or '(setf symbol).

Source information for all other forms have a symbol for TYPE which is
one of the following:

:class, :variable, :condition, :constant, :compiler-macro, :macro
:package, :structure, :type, :setf-expander, :source-transform

These values follow SBCL'S implemenation in SLIME
c.f. <https://github.com/slime/slime/blob/bad2acf672c33b913aabc1a7facb9c3c16a4afe9/swank/sbcl.lisp#L748>

Modifications are in two places, one at the definitions, calling
record-source-information-by-type and then again in the file-compiler,
which writes forms like

    (put 'source name (cons (list type pathname position) (get 'source name)))

In theory this can lead to redundancy if a fasl is loaded again and
again. I'm not sure how to fix this yet. Forms in the __loader__ get
called early in build when many of the sequence functions aren't
present.  Will probably just filter when presenting in slime.

<> :closes <http://abcl.org/trac/ticket/421> .
<> :merges <armedbear/abcl#5> .

git-svn-id: http://abcl.org/svn@14914 1c010e3e-69d0-11dd-93a8-456734b0d56f

slyrus pushed a commit to slyrus/abcl that referenced this pull request May 3, 2017

mevenson
Dramatically improve source recording on SYS::SOURCE plist for a symb…
…ol (Alan Ruttenberg)

The interface to recording information on the SYS:%SOURCE plist for a
symbol is now deprecated and will be removed with abcl-1.7.

Implementation
--------------

Source information for ABCL is now recorded on the SYS::SOURCE
property.  The appropiate information for type is recorded by the
SYS::RECORD-SOURCE-INFORMATION-BY-TYPE function:

    record-source-information-by-type (name type &optional source-pathname source-position)

TYPE is either a symbol or list.

Source information for functions, methods, and generic functions are
represented as lists of the following form:

    (:generic-function function-name)
    (:function function-name)
    (:method method-name qualifiers specializers)

Where FUNCTION-NAME or METHOD-NAME can be a either be of the form
'symbol or '(setf symbol).

Source information for all other forms have a symbol for TYPE which is
one of the following:

:class, :variable, :condition, :constant, :compiler-macro, :macro
:package, :structure, :type, :setf-expander, :source-transform

These values follow SBCL'S implemenation in SLIME
c.f. <https://github.com/slime/slime/blob/bad2acf672c33b913aabc1a7facb9c3c16a4afe9/swank/sbcl.lisp#L748>

Modifications are in two places, one at the definitions, calling
record-source-information-by-type and then again in the file-compiler,
which writes forms like

    (put 'source name (cons (list type pathname position) (get 'source name)))

In theory this can lead to redundancy if a fasl is loaded again and
again. I'm not sure how to fix this yet. Forms in the __loader__ get
called early in build when many of the sequence functions aren't
present.  Will probably just filter when presenting in slime.

<> :closes <http://abcl.org/trac/ticket/421> .
<> :merges <armedbear/abcl#5> .

git-svn-id: http://abcl.org/svn/trunk/abcl@14914 1c010e3e-69d0-11dd-93a8-456734b0d56f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.