search

joaotavora / sly

Sylvester the Cat's Common Lisp IDE
1.24k stars 140 forks source link

Move handling of SBCL's package variance warning to an around-compile hook #492

Closed daewok closed 2 years ago

daewok commented 2 years ago

Recompiling slynk-backend.lisp and slynk.lisp causes SBCL to emit package variance warnings. Currently, these are handled by wrapping (asdf:load-system "slynk"). Unfortunately, this does not cover use cases where slynk is loaded as a dependency of another library. At least 40ants-doc has added slynk as a dependency, making it much more likely that slynk will be recompiled outside of a call to load slynk directly.

We can handle this by moving the logic to handle the package variance warning to the around compile hook of the slynk-backend and slynk files. :AROUND-COMPILE with a LAMBDA was introduced in ASDF 2.019.

If the minimum required ASDF version is bumped to something closer to ASDF 3, more (better) options become available to handle this, as noted in the comments.

The only observable change should be that prior to this commit, (asdf:load-system "slynk") would signal a uiop::compile-warned-warning condition, but after this commit no warning at all is signaled and nothing is printed that indicates there was a warning. This is a bit unfortunate, as there could be other, non package-variance, warnings that would be silently missed. But it's the best I could come up with without requiring ASDF 3 or using a symbol from SB-INT.

joaotavora commented 2 years ago

I think there is no reason to block your PR, because you've described its advantages and disadvantages so well. I'm going to merge it. But I must admit I'm just not fully in the loop anymore. At one point I understood what that code was about, now I don't, that info has just made space for something else. Some basic questions:

Again, I'm sorry for my ignorance in this matter.

joaotavora commented 2 years ago

Some basic questions:

Actually, after reading and very slightly editing your commit message for formatting issues, I found that mostly answers my doubts about this. Thanks a lot!

daewok commented 2 years ago

Why are packages devoted to SBCL handling being compiled and recompiled in a different "variance"d form? Could this not be fixed there?

Sure! I think the better solution is to fix the defpackages, but assumed that was a non-starter given the work-around was placed into the .asd file instead of fixing the defpackages in the first place. The best way to do so would likely be to ban all use of cl:export and require that any newly exported symbol be added to the defpackage form. I'm happy to do that if you'd prefer. It would then get rid of all this special handling in the .asd.

What, in your opinion, could be the downsides of making ASDF3 a dependency. Last I used CL systems, ASDF seemed to be supported well, sometimes out of the box?

Every major implementation that I'm aware of ships with ASDF 3. Quicklisp also requires ASDF 3.2.1+. The biggest downside would be if someone is using Sly to interact with an ancient image. That does apparently happen with SLIME at least somewhat: https://github.com/slime/slime/pull/282. Not sure about Sly.

What, if anything, is SLIME/Swank doing here?

I believe SLIME doesn't have any of this handling because its packages are never in variance.

joaotavora commented 2 years ago

The best way to do so would likely be to ban all use of cl:export and require that any newly exported symbol be added to the defpackage form. I'm happy to do that if you'd prefer. It would then get rid of all this special handling in the .asd.

If this is feasible, then I'd be very grateful.

I believe SLIME doesn't have any of this handling because its packages are never in variance.

Find that suspicious, but that's too long ago for me to remember, so you're probably right.

daewok commented 2 years ago

Find that suspicious, but that's too long ago for me to remember, so you're probably right.

I think you're right. Looks like they are in variance at times. However, SLIME has pretty much eschewed ASDF in favor of its own loading mechanism. Its defsystem is a single file component and the perform method for the file just calls SLIME's init function. Within SLIME's own init function, it calls compile-file directly and ignores the second and third return values. While it makes me cringe, it does mean that so long as the .fasl is produced, regardless of warnings, it's happily loaded.