Implement package-local nicknames #188

phoe · 2019-01-31T15:30:00Z

This PR adds package-local nicknames to Clozure Common Lisp and aims to fix #111.

This implementation is adapted from SBCL code.

Bootstrapping steps:

Clone this repository
Put 1.12.dev1 bootstrapping binaries in the directory and launch them
Evaluate code from https://plaster.tymoon.eu/view/1144#1144 to bootstrap enough package-local nicknames in the live CCL image
(rebuild-ccl :full t)
Nothing should catch fire

The CCL image resulting from building 082ef8e passes all 16 tests specified in https://github.com/phoe/package-local-nicknames-tests.

Tested on linux64. No platform-dependent functionality was changed though, so other platforms should behave consistently.

TODO:

code review
confirm that this builds for anyone else
documentation
add tests to ccl-tests
~~add CHECK-TYPE assertions to find-package, %find-pkg, pkg-arg~~ ✔️
~~performance tests against ACL2~~ ✔️

…local nicknames

phoe · 2019-01-31T15:52:10Z

Running the ccl-tests from commit 95bd171e3b983 suite shows no tests were broken.

$ make CCL=/home/phoe/Projects/Git/ccl/lx86cl64 test
make clean
make[1]: Wejście do katalogu '/home/phoe/Projects/Git/ccl-tests'
(cd ansi-tests && make clean)
make[2]: Wejście do katalogu '/home/phoe/Projects/Git/ccl-tests/ansi-tests'
make[3]: Wejście do katalogu '/home/phoe/Projects/Git/ccl-tests/ansi-tests/beyond-ansi'
make[3]: Opuszczenie katalogu '/home/phoe/Projects/Git/ccl-tests/ansi-tests/beyond-ansi'
make[2]: Opuszczenie katalogu '/home/phoe/Projects/Git/ccl-tests/ansi-tests'
make[1]: Opuszczenie katalogu '/home/phoe/Projects/Git/ccl-tests'
/home/phoe/Projects/Git/ccl/lx86cl64 --no-init --batch -l load.lisp -e "(run-tests :exit t)"
; Warning: Redefining test CCL.ISSUE\#167
; While executing: REGRESSION-TEST::REPORT-ERROR, in process listener(1).
Doing 21908 pending tests of 21908 tests total.
Invoking restart: #<RESTART CL-TEST::FOO #x7FBE655922BD>
Invoking restart: #<RESTART CL-TEST::FOO #x7FBE655922BD>
Invoking restart: #<RESTART CL-TEST::FOO #x7FBE655922BD>
Invoking restart: #<RESTART CL-TEST::FOO #x7FBE655922BD>
Invoking restart: #<RESTART CL-TEST::FOO #x7FBE655922BD>

=============== All tests succeeded ===============

(FUNCALL DO-TESTS :COMPILE COMPILE :VERBOSE VERBOSE :CATCH-ERRORS T)
took 128,665,770 microseconds (128.665770 seconds) to run.
       1,735,654 microseconds (  1.735654 seconds, 1.35%) of which was spent in GC.
During that period, and with 8 available CPU cores,
     121,963,139 microseconds (121.963135 seconds) were spent in user mode
         605,143 microseconds (  0.605143 seconds) were spent in system mode
 8,124,620,656 bytes of memory allocated.
 6,134 minor page faults, 0 major page faults, 0 swaps.

sirherrbatka

Please, consider adding check-type assertions in the following places: find-package, %find-pkg, pkg-arg. It is not mandatory and exceeds scope of this task, but it will be beneficial to the overall quality of the code.

Thank you.

phoe · 2019-01-31T15:54:43Z

@sirherrbatka Added to the TODO list.

phoe · 2019-02-01T20:41:59Z

I have run ACL2's certify-books on my computer with CCL 1.12.1 without PLNs (the original binary) and with PLNs. There does not seem to be any difference in run time after a single test run.

CCL 1.12.dev without PLNs:
real	226m8,535s
user	792m5,427s
sys	9m28,350s

CCL 1.12.dev with PLNs:
real	225m41,707s
user	796m12,919s
sys	9m26,029s

Please note that both of the runs have completed unsuccessfully (the make command failed); I will be opening a separate ticket for that as soon as I have the complete logs.

xrme

A couple of trivial comments.

level-0/nfasload.lisp

solswords · 2019-02-14T19:56:48Z

Notes toward confirming this builds for anyone else: I built CCL following the directions above, but the PLN tests didn't all pass for me:

;;;; TEST-OWN-NICKNAME-AS-LOCAL-NICKNAME-INTERN: Success
;;;; TEST-OWN-NICKNAME-AS-LOCAL-NICKNAME-CERROR: Success
;;;; TEST-OWN-NAME-AS-LOCAL-NICKNAME-INTERN: Success
;;;; TEST-OWN-NAME-AS-LOCAL-NICKNAME-CERROR: Success
;;;; TEST-DELETE-PACKAGE-LOCALLY-NICKNAMED-BY-OTHERS: Success
;;;; TEST-DELETE-PACKAGE-LOCALLY-NICKNAMES-OTHERS: Success
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-NICKNAME-REMOVAL-READD-ANOTHER-SYMBOL-PRINTING: Failure: Failed assertion: (EQUAL "L:CONS" (PRIN1-TO-STRING PACKAGE-LOCAL-NICKNAMES-TESTS::CONS0))
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-NICKNAME-REMOVAL-READD-ANOTHER-PACKAGE-EQUALITY: Success
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-NICKNAME-REMOVAL-READD-ANOTHER-SYMBOL-EQUALITY: Failure: Failed assertion: (NOT (EQ 'CONS PACKAGE-LOCAL-NICKNAMES-TESTS::CONS0))
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-NICKNAME-REMOVAL: Failure: Failed assertion: (EQUAL PACKAGE-LOCAL-NICKNAMES-TESTS::+SYM-FULLNICKNAME+ (PRIN1-TO-STRING PACKAGE-LOCAL-NICKNAMES-TESTS::EXIT0))
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-NICKNAME-COLLISION: Success
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-SYMBOL-PRINTING: Failure: Failed assertion: (EQUAL "L:CONS" (PRIN1-TO-STRING PACKAGE-LOCAL-NICKNAMES-TESTS::CONS0))
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-PACKAGE-EQUALITY: Success
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-SYMBOL-EQUALITY: Success
;;;; TEST-PACKAGE-LOCAL-NICKNAMES-INTROSPECTION: Success
;;;;
;;;; 15 tests run, 4 failures.

Probably unrelated, but are the following warnings expected when loading the code from https://plaster.tymoon.eu/view/1144#1144 ?

;   In ADD-PACKAGE-LOCAL-NICKNAME: Undefined function SIGNAL-PACKAGE-ERROR
;   In ADD-PACKAGE-LOCAL-NICKNAME: Undefined function SIGNAL-PACKAGE-CERROR

They came up when I copied that code into a file and loaded it, but not when I pasted the contents into the REPL. Didn't seem to make a difference to the outcome of the tests. I assume they're harmless?

phoe · 2019-02-14T20:39:22Z

You are correct, I can reproduce these failures. I will resolve them and these warnings before we proceed.

phoe · 2019-02-15T14:36:55Z

@solswords I have fixed the tests - the packages created inside them were missing an explicit (:use) which resulted in implementation-defined behavior being invoked, and implementation-defined symbols being imported into the created packages, which broke the tests.

Currently the created packages use no packages and the test behavior should be consistent.

Please verify with commit dd74985 of package-local-nicknames-tests.

solswords · 2019-02-19T16:29:08Z

Yes, the CCL I previously built according to the instructions passes the new tests.

phoe · 2019-02-19T16:36:31Z

@solswords Thanks!

@pfdietz Could you take one more look at the contents of the tests, make sure that they do what they are supposed to do (they are based on SBCL sources), run them, and verify it yourself as well?

pfdietz · 2019-02-19T17:02:39Z

Test test-package-local-nicknames-introspection should bind package, so its behavior is not dependent on the context in which it is run.

You might consider a test that does some sanity checks with package bound to various packages, including CL, CL-USER, and KEYWORD.

if INTERN is having something special happen with a compier macro, you should check that by putting a call to INTERN in a lambda and compiling that.

(intern x :foo) ==> (funcall (compile () '(lambda (y) (intern y :foo))) x)

and also with package bound to various things when the compiler is called.

I think it is worth testing how PLNs interact with :use, :import, :export, :shadow, :shadowing-import-from, :import-from, and :intern. Do the local nicknames apply to the package specifiers in :import-from, :shadowing-import-from, and :use?

Still looking the tests over, but these are first thoughts.

pfdietz · 2019-04-14T17:18:55Z

I pushed some more test changes to

https://github.com/pfdietz/package-local-nicknames-tests-1

This include everything in #188 until the paragraph "I think it is worth testing how PLNs...".

phoe · 2019-04-15T15:45:57Z

I have merged the test PR and run it on the modified CCL.

;; 17 tests run, 0 failures.

I am now confident to say that package local nicknames have been tested and behave the same way on SBCL, ABCL, and CCL.

What do we do next in order to merge this?

xrme · 2019-04-17T01:45:40Z

We need to make new bootstrapping binaries very soon, but I wanted to get this in.

phoe · 2019-04-17T06:37:22Z

Thanks!

defunkydrummer · 2019-04-18T18:12:25Z

Thousands of thanks!! Wonderful!!

phoe added 11 commits January 29, 2019 00:44

Refactor DEFPACKAGE

809abc3

Remove unnecessary EVAL-WHEN

d95d199

Add :LOCAL-NICKNAMES to DEFPACKAGE

fd63e13

Make PKG-ARG recognize local nicknames

ecaf922

Make deleting packages, package lookup, and reader work with package-…

8ef07ac

…local nicknames

Add PLN protocol code, export PLN protocol from CCL package

a043916

Move internal package local nickname accessors to level0

05361aa

unscrew %FIND-PKG

0300292

fix level0/level1 issues

9eb3692

bugfix in ADD-PACKAGE-LOCAL-NICKNAME

e65c8f7

Make local nicknames printable

082ef8e

phoe mentioned this pull request Jan 31, 2019

Implement package local nicknames in CCL [$260] #111

Closed

sirherrbatka reviewed Jan 31, 2019

View reviewed changes

Add CHECK-TYPE to package functions

50135c8

xrme reviewed Feb 3, 2019

View reviewed changes

level-0/nfasload.lisp Outdated Show resolved Hide resolved

level-0/nfasload.lisp Outdated Show resolved Hide resolved

phoe added 3 commits February 4, 2019 18:21

s/check-type/ccl::require-type/ in nfasload.lisp

6a3e478

quote types to ccl::require-type

8e6074a

no deftype in level0

f2a42f5

xrme merged commit f120efb into Clozure:master Apr 17, 2019

phoe mentioned this pull request Apr 17, 2019

CCL now supports package-local-nicknames rpav/defpackage-plus#4

Closed

phoe mentioned this pull request Jul 16, 2019

Extend DEFPACKAGE tests phoe/trivial-package-local-nicknames#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement package-local nicknames #188

Implement package-local nicknames #188

phoe commented Jan 31, 2019 •

edited

phoe commented Jan 31, 2019

sirherrbatka left a comment

phoe commented Jan 31, 2019

phoe commented Feb 1, 2019

xrme left a comment

solswords commented Feb 14, 2019

phoe commented Feb 14, 2019

phoe commented Feb 15, 2019

solswords commented Feb 19, 2019

phoe commented Feb 19, 2019

pfdietz commented Feb 19, 2019

pfdietz commented Apr 14, 2019

phoe commented Apr 15, 2019

xrme commented Apr 17, 2019

phoe commented Apr 17, 2019

defunkydrummer commented Apr 18, 2019

Implement package-local nicknames #188

Implement package-local nicknames #188

Conversation

phoe commented Jan 31, 2019 • edited

phoe commented Jan 31, 2019

sirherrbatka left a comment

Choose a reason for hiding this comment

phoe commented Jan 31, 2019

phoe commented Feb 1, 2019

xrme left a comment

Choose a reason for hiding this comment

solswords commented Feb 14, 2019

phoe commented Feb 14, 2019

phoe commented Feb 15, 2019

solswords commented Feb 19, 2019

phoe commented Feb 19, 2019

pfdietz commented Feb 19, 2019

pfdietz commented Apr 14, 2019

phoe commented Apr 15, 2019

xrme commented Apr 17, 2019

phoe commented Apr 17, 2019

defunkydrummer commented Apr 18, 2019

phoe commented Jan 31, 2019 •

edited