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

Fix keyword internalization in defsystem and defpackage #47

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kilianmh
Copy link

Replaced all keyword arguments with uninterned symbols in DEFPACKAGE and IN-PACKAGE. This should reduce pollution of namespace and enable consistent highlighting in editors.

@gefjon
Copy link

gefjon commented Feb 22, 2022

Personally, I am of the opinion that package names should be written as keywords, because I value having them autocompleted.

For symbol names in clauses like :export, I certainly favor using uninterned symbols, and will gladly merge those changes.

I find it both aesthetically pleasing and useful that, when a package definition lists package names as keywords and symbol names as uninterned symbols, my editor highlights them differently; it makes skimming the package definitions much easier.

I'd like input from some other people, ideally, as to whether it's preferable to write defpackage :cl-ppcre or defpackage #:cl-ppcre.

@phoe
Copy link
Contributor

phoe commented Feb 22, 2022

Two cents from me: I don't care much for package autocompletion since my interactive package traversal happens mostly via the slime REPL shortcut ,in and via C-c ~; as for programmatic use, I write in-package forms seldom enough to not really think about autocompletion for them. I'd personally use gensyms, but I have no strong opinion against keywords in the case you mentioned.

@hanshuebner
Copy link
Member

Why is "polluting the namespace" a problem?

@phoe
Copy link
Contributor

phoe commented Feb 22, 2022

I guess it's about a) keeping keyword syntax completions minimal and b) microoptimizations, since you can't portably unintern a keyword without UB.

@gefjon
Copy link

gefjon commented Feb 22, 2022

Why is "polluting the namespace" a problem?

Editors (i.e. Emacs with SLIME or SLY) examine the accessible symbols in a package when autocompleting. With no package prefix, they search the interned symbols in *package*; with a single-colon package prefix, they search the external symbols of that package. This is exactly what you want for every package except keyword, where all symbols are automatically exported. When you begin typing a keyword and trigger autocomplete, the editor searches through every keyword that's ever been interned. This set is often quite large, and contains many bad suggestions; this means that a user must type more characters before autocomplete on keywords finds the intended symbol. This problem can be partially negated by avoiding interning keywords that you'll never use again. No one will ever use any of the keywords in the :export clause of the defpackage :cl-ppcre form, so they should instead be uninterned symbols.

I write in-package forms seldom enough to not really think about autocompletion for them

I favor a package-per-file style with ASDF's package-inferred-system, so I find myself writing defpackage or uiop:define-package forms relatively frequently, and listing relatively many packages in :use or:import-from clauses. For a package name as short as cl-ppcre it doesn't matter much, but in general I like having autocomplete so I can type e.g. :tri-ba M-i rather than all of :trivial-backtrace.

@hanshuebner
Copy link
Member

No one will ever use any of the keywords in the :export clause of the defpackage :cl-ppcre form, so they should instead be uninterned symbols.

Thank you for explaining, that is a good reason.

@kilianmh
Copy link
Author

Thank you for the valuable feedback! According to this post from Rainer Joswig, another problem might be:

The usual expectation is that unreferenced symbols interned in packages might not be removed by a Garbage Collector (GC) - the Common Lisp standard says nothing about this topic and has no required behavior. So adding new symbols to a package often keeps it growing. This could be a memory leak in some applications.

Now in-package arguments and defpackage name, :nicknames, :use clauses are reverted back to keywords. The pull request now comprises uninterened symbols in the defpackage clauses :shadow :shadowing-import-from :import-from :export.

@kilianmh
Copy link
Author

Reverted :SHADOWING-IMPORT-FROM and :IMPORT-FROM package names to KEYWORD, which should improve readability without unnecessary expansion of *PACKAGE*.

- Avoid cl-ppcre/test as keyword in :in-order-to clauses
-> avoid internalization in keyword package

- Simplify :perform clause in defpackage
-> avoid internalization of cl-ppcre-test and run-all-tests in keyword package
@kilianmh kilianmh changed the title Fix namespace pollution and highlighting by usage of uninterned symbols Fix keyword internalization in defsystem and defpackage Aug 7, 2022
@kilianmh
Copy link
Author

kilianmh commented Aug 7, 2022

Summary:

Fix keyword internalization in defsystem and defpackage

  • Replace keywords in defpackage and defsystem (Remaining: library names)
  • Simplify :perform clause in defsystem

-> 35 less keywords interned when loading cl-ppcre
-> 37 less keywords interned when loading cl-ppcre-unicode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants