Set print-symbols-bare to t while saving the build cache #1149
Conversation
| ;; Print symbols with attached positions, which can occur | ||
| ;; during byte compilation, as bare symbols so they can be | ||
| ;; read when loading the cache. | ||
| (print-symbols-bare t)) |
There was a problem hiding this comment.
| (print-symbols-bare t)) | |
| (print-symbols-bare t)) | |
| (ignore print-symbols-bare) |
It looks like that variable was only introduced in Emacs 29.
That's why the CI is failing with a byte-compiler warning:
There was a problem hiding this comment.
That (at least partly) explains why I only noticed this problem recently!
There was a problem hiding this comment.
Do you have a reliable way of producing the problem or an example of output where the symbol positions are included? If so, it'd be good to see so we can possibly add a regression test or at least have it documented here.
There was a problem hiding this comment.
Yes, something like the following snippet should trigger the issue:
(defmacro test-macro (&rest args)
(straight-use-package args)
`(straight-use-package ',args))
(test-macro dash
:type git
:flavor melpa
:files ("dash.el" "dash.texi" "dash-pkg.el")
:host github :repo "magnars/dash.el")Byte-compiling this snippet should give the desired result. You could imagine why something like this might be plausible when the package needs to be installed at macro expansion time for whatever reason.
Interestingly, I couldn't get this to reproduce using eval-and-compile. This is because eval ignores positions when symbols-with-pos-enabled is non-nil (as it is during byte-compilation).
There was a problem hiding this comment.
You could imagine why something like this might be plausible when the package needs to be installed at macro expansion time for whatever reason.
I'm not sure I follow. When/why would one want to install packages during macro expansion?
@raxod502 any thoughts on this use case?
Interestingly, I couldn't get this to reproduce using eval-and-compile. This is because eval ignores positions when symbols-with-pos-enabled is non-nil (as it is during byte-compilation).
Why not have the macro expansion make use of eval-and-compile in that case?:
(defmacro test-macro (&rest args)
`(eval-and-compile ...))There was a problem hiding this comment.
I'm not sure I follow. When/why would one want to install packages during macro expansion? @raxod502 any thoughts on this use case?
Well, for example use-package does this. The reason is that the package may define macros which are used subsequently in the file. To ensure the later macros can be expanded, the package must be installed at compile time.
Why not have the macro expansion make use of
eval-and-compilein that case?:
I think it should be possible to use eval-and-compile or possibly eval-when-compile to achieve the desired effect, but since the fix is simple and self contained, I don't see a reason not to accommodate users who are not using those macros (or use packages that do not). Especially so given that the unpatched behavior 1. isn't obviously correct and 2. leads to a silent failure that is difficult to diagnose.
There was a problem hiding this comment.
To be clear, I'm not against the solution. I'm just trying to understand the problem fully before we apply a fix.
That byte compilation warning still needs to be addressed before we can merge, too.
There was a problem hiding this comment.
Got it! Let me take a stab at fixing the warning and see if it's to your liking.
There was a problem hiding this comment.
Installing packages during byte-compilation doesn't seem like a great idea to me, but it also doesn't seem so wrong that I would say we should not allow it. Of course we should fix the bug so I agree with the submitted patch.
PythonNut
force-pushed
the
print-symbols-bare
branch
2 times, most recently
from
370c463
to
ed499b8
Compare
|
@PythonNut see the suggested change above. It should be enough to let-bind the variable and then immediately pass it to ignore. |
|
Oh, do you prefer that to the |
Not a problem. Yes the let-binding is a simpler solution and won't create a global variable. |
PythonNut
force-pushed
the
print-symbols-bare
branch
from
ed499b8
to
b26befd
Compare
This fixes a bug where recipes may contain symbols with positions, which have a printed representation but no read syntax, causing the subsequent load of the build cache to fail. Symbols with positions may be generated during the execution of the byte compiler.
PythonNut
force-pushed
the
print-symbols-bare
branch
from
b26befd
to
1b1797c
Compare
|
@progfolio Is there anything else I need to do to get this across the finish line? |
|
It looks fine to me! |
This fixes a bug where recipes may contain symbols with positions, which have a printed representation but no read syntax, causing the subsequent load of the build cache to fail. Symbols with positions may be generated during the execution of the byte compiler.
The fix is to bind
print-symbols-baretotduring the printing of the build cache instraight--save-build-cache. This strips the attached positions from the symbols. The attached positions are not used bystraight.elin any way, so the build cache remains valid when loaded. The distinction between bare symbols and symbols with positions is only relevant here (during build cache save), sincesymbols-with-pos-enabledis bound totduring the execution of the byte compiler, causing symbols with positions to transparently behave as their contained bare symbols.