bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

As far as I understand Emacs doesn't add "~/.emacs.d/elpa" to
`load-path`, when started with -Q flag, but this bug report needs a
fully established `load-path`. So to reproduce the bug please create an
empty ".emacs" file and start Emacs.

Then:

- C-x C-f ~/.emacs
- M-x flymake-mode
- in .emacs buffer type require expression, requiring any library on
  your elpa path, e.g. `(require 'dash)`.

Observe that flymake reports "Cannot open load file: No such file or
directory, dash", even though `load-path` contains
"~/.emacs.d/elpa/dash-<version>/" directory. Screenshot below
illustrates this:


[-- Attachment #2: screenshot --]
[-- Type: image/png, Size: 48489 bytes --]

[-- Attachment #3: Type: text/plain, Size: 4564 bytes --]



In GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.29, cairo version 1.17.4)
 of 2021-05-09 built on arch-max
Repository revision: 5eb27833c498584797822838f00b87e52bad1c22
Repository branch: makepkg
Windowing system distributor 'System Description: Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
 --with-sound=alsa --with-modules --without-gconf --without-gsettings
 --with-native-compilation --with-pgtk --with-x-toolkit=gtk3
 --without-xaw3d --without-m17n-flt --with-cairo --with-xwidgets
 --without-compress-install 'CFLAGS=-march=x86-64 -mtune=generic -O2
 -pipe -fno-plt -fexceptions
 -Wp,-D_FORTIFY_SOURCE=2,-D_GLIBCXX_ASSERTIONS -Wformat
 -Werror=format-security -fstack-clash-protection -fcf-protection -g
 -fuse-ld=gold -g -fuse-ld=gold'
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PGTK PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XIM
XWIDGETS GTK3 ZLIB

Important settings:
  value of $LC_MESSAGES: 
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  yas-global-mode: t
  show-paren-mode: t
  global-company-mode: t
  display-battery-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/max/.emacs.d/elpa/transient-20210426.2141/transient hides /usr/share/emacs/28.0.50/lisp/transient

Features:
(shadow comp comp-cstr rx sort vc-mtn vc-hg vc-git diff-mode easy-mmode
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs vc vc-dispatcher mail-extr
emacsbug message rmc puny dired dired-loaddefs rfc822 mml mml-sec epa
derived epg epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail cl-extra company-oddmuse
company-keywords company-etags etags fileloop generator company-gtags
company-dabbrev-code company-dabbrev company-files company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-bbdb format-spec
solarized-light-high-contrast-theme solarized-palettes solarized
solarized-faces color yasnippet quail paren gnus nnheader gnus-util
rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums time-date mail-utils
mm-util mail-prsvr wid-edit company edmacro kmacro battery dbus xml
cus-load eglot array filenotify jsonrpc ert pp ewoc debug backtrace
help-mode find-func xref flymake-proc flymake thingatpt warnings compile
text-property-search comint ansi-color ring pcase project imenu info
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib iso-transl tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/pgtk-win
pgtk-win term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads xwidget-internal dbusbind
inotify dynamic-setting font-render-setting cairo move-toolbar gtk
x-toolkit pgtk lcms2 multi-tty make-network-process native-compile
emacs)

Memory information:
((conses 16 251103 8902)
 (symbols 48 19072 1)
 (strings 32 54692 4270)
 (string-bytes 1 1713885)
 (vectors 16 37517)
 (vector-slots 8 1351452 110243)
 (floats 8 246 233)
 (intervals 56 486 0)
 (buffers 992 14))

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

Max Brieiev <max.brieiev@gmail.com> writes:

> As far as I understand Emacs doesn't add "~/.emacs.d/elpa" to
> `load-path`, when started with -Q flag, but this bug report needs a
> fully established `load-path`. So to reproduce the bug please create an
> empty ".emacs" file and start Emacs.
>
> Then:
>
> - C-x C-f ~/.emacs
> - M-x flymake-mode
> - in .emacs buffer type require expression, requiring any library on
>   your elpa path, e.g. `(require 'dash)`.
>
> Observe that flymake reports "Cannot open load file: No such file or
> directory, dash", even though `load-path` contains
> "~/.emacs.d/elpa/dash-<version>/" directory.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I'm unable to reproduce this in Emacs 29.  Do you still see this problem
in recent Emacs versions?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm unable to reproduce this in Emacs 29.  Do you still see this problem
> in recent Emacs versions?

Yes, I am able to reproduce this on Emacs 29 (two days old build).

Please, check the screenshot below.


[-- Attachment #2: screenshot --]
[-- Type: image/png, Size: 73811 bytes --]

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

Max Brieiev <max.brieiev@gmail.com> writes:

> Yes, I am able to reproduce this on Emacs 29 (two days old build).
>
> Please, check the screenshot below.

Do you have a complete step by step recipe that demonstrates the
problem?  I tried the original instructions, but got no messages from
flymake (except complaining that the .emacs file doesn't start with ;;;
Commentary).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Do you have a complete step by step recipe that demonstrates the
> problem?  I tried the original instructions, but got no messages from
> flymake (except complaining that the .emacs file doesn't start with ;;;
> Commentary).

I think this issue can not be reproduced with emacs -Q, because in this
case Elpa packages are not added to load-path. With emacs -Q, flymake
will rightfully complain "No such file or directory", when you `(require
'any-elpa-package)`

However, during the normal Emacs session, all Elpa packages are on the
load-path, but flymake complains as if they were not.

For me the following reproduces the issue:
1. Start Emacs
2. Switch to scratch buffer
3. Enable flymake: M-x flymake-mode
4. Type:

    (require 'subr-x)

  Observe that flymake does not complain

5. Now load anything from Elpa:

    (require 'compat)

  Observe that flymake starts complaining, even though compat is on
  load-path and the expression above can be successfully evaluated.

screenshot:


[-- Attachment #2: screenshot --]
[-- Type: image/png, Size: 27131 bytes --]

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Do you have a complete step by step recipe that demonstrates the
> problem?  I tried the original instructions, but got no messages from
> flymake (except complaining that the .emacs file doesn't start with ;;;
> Commentary).

`elisp-flymake-byte-compile' is a flymake backend for the elisp-mode.

It runs emacs in batch mode with -Q flag as a child process to provide
diagnostics for the current buffer.

In this case the `load-path' of the child process includes only builtin
packages, while the `load-path' of the parent process includes all the
directories added by the normal bootstrap process.

The load-path for the child process is controlled with
`elisp-flymake-byte-compile-load-path'. By default, it contains only
current directory.

The following hack "fixed" the issue for me:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: elisp-mode.el.diff --]
[-- Type: text/x-patch, Size: 663 bytes --]

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 0c4a9bfdbe..db3592b903 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -2145,7 +2145,7 @@ current buffer state and calls REPORT-FN when done."
                    "--batch"
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
                    ,@(mapcan (lambda (path) (list "-L" path))
-                             elisp-flymake-byte-compile-load-path)
+                             load-path)
                    "-f" "elisp-flymake--batch-compile-for-flymake"
                    ,temp-file)
         :connection-type 'pipe

[-- Attachment #3: Type: text/plain, Size: 74 bytes --]


Can we add some user-friendly knobs to control this behavior of flymake?

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

Max Brieiev <max.brieiev@gmail.com> writes:

> I think this issue can not be reproduced with emacs -Q, because in this
> case Elpa packages are not added to load-path. With emacs -Q, flymake
> will rightfully complain "No such file or directory", when you `(require
> 'any-elpa-package)`

OK; so a way to reproduce this is to say (with "emacs -Q"):

M-: (push (expand-file-name "~/.emacs.d/elpa/compat-28.1.1.1/") load-path)
M-x flymake-mode

and then notice that there's a warning on

(require 'subr-x)
(require 'compat)

even though it can be required fine.

I've added João to the CCs; perhaps he has some comments.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Max Brieiev <max.brieiev@gmail.com> writes:
>
>> I think this issue can not be reproduced with emacs -Q, because in this
>> case Elpa packages are not added to load-path. With emacs -Q, flymake
>> will rightfully complain "No such file or directory", when you `(require
>> 'any-elpa-package)`
>
> OK; so a way to reproduce this is to say (with "emacs -Q"):
>
> M-: (push (expand-file-name "~/.emacs.d/elpa/compat-28.1.1.1/") load-path)
> M-x flymake-mode
>
> and then notice that there's a warning on
>
> (require 'subr-x)
> (require 'compat)
>
> even though it can be required fine.
>
> I've added João to the CCs; perhaps he has some comments.

I think elisp-flymake-byte-compile-load-path is relevant here.

   elisp-flymake-byte-compile-load-path is a variable defined in `elisp-mode.el'.
    
   Its value is ("./")
    
   Like `load-path' but used by `elisp-flymake-byte-compile'.
   The default value contains just "./" which includes the default
   directory of the buffer being compiled, and nothing else.
    
     This variable is safe as a file local variable if its value
     satisfies the predicate which is a byte-compiled expression.

I don't usually develop packages in ~/.emacs.d/elpa, in fact developing
packages there is kind of questionable IMHO, as they are normally not
git checkouts.  This is, AFAIK, straight.el's main raison d'etre,
although I don't use that either.

But it could make sense to add ~/.emacs.d/elpa/* to the variable, if the
package you're developing somewhere else has a dependency on other Elpa
packages.

Although it's a safety hazard I guess (don't forget that the
elisp-flymake-byte-compile backend runs compile-time code!).  Or maybe
add a `package-initialize` call in the Emacs -Q that
elisp-flymake-byte-compile runs.  But that'll probably slow checking
down a bit, so i'd like to see some timings for the slowdown
before&after.

Or maybe, Max, you can just set this variable it in your file-local
variables or the dir-locals.el of the package you're developing.

João

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> I think elisp-flymake-byte-compile-load-path is relevant here.

I think the question here was -- why doesn't that just default to
`load-path'?  I'd expect the flymake environment to be similar to the
environment in my running Emacs.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

On Fri, Jul 15, 2022 at 10:47 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > I think elisp-flymake-byte-compile-load-path is relevant here.
>
> I think the question here was -- why doesn't that just default to
> `load-path'?  I'd expect the flymake environment to be similar to the
> environment in my running Emacs.
>

When I was first coding this backend, I had that expectation too.
But it's not very useful, as you normally, when developing an elisp
file in a package, you want to be made aware of the potential compilation
problems arising when compiling that file in a much simpler environment,
very often the Emacs -Q environment where Makefiles usually byte-compile
files.

It's really not very useful, in my opinion and experience, if the same
elisp file
in the same project visited from two different Emacs sessions shows
different
sets of errors.

And there's also the question of security.  Flymake runs compile-time
code every time by simply modifying the buffer.  So being conservative
here is also a good idea because of that.

Once I was working on a macro and I temporarily put a `delete-file` in
there,
which I later deleted because I had given it the wrong argument.
The macro was being expanded at top level. The file was gone.

Anyway, because the directories under ~/.emacs.d/elpa are somewhat special
and/or security-vetted it _could_ make sense to add them to the default
value of the variable. This would amount to more or less the same as
calling the underlying process with `-f package-initialize` I think.

But I'm still not sure this should be the default, or merely an option to
the flymake-elisp-byte-compile backend.  I think the second is safer.

BTW, this is very similar to other "on-the-fly" backends like C/C++
checkers
which add some "system" things to the include path considered when checking,
but still need to know about the user's include paths.  Google
compilation_commands.json or "compilation database".  These are normally
files checked into the repository, or very easy to generate. Of course in
Elisp,
we probably don't need such complexity: merely adjusting the variable I gave
and/or putting that in the repo's dir-locals.el suffices.


João

[-- Attachment #2: Type: text/html, Size: 3186 bytes --]

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

João Távora <joaotavora@gmail.com> writes:

> But it could make sense to add ~/.emacs.d/elpa/* to the variable, if the
> package you're developing somewhere else has a dependency on other Elpa
> packages.

Yes, to me it seems very common to have a depandency on an Elpa
package, so I was wondering why flymake was complaining about requiring
installed package.

> Or maybe, Max, you can just set this variable it in your file-local
> variables or the dir-locals.el of the package you're developing.

This could work, but doesn't it mean that with each new version of a
dependency, I'll have to change my dir-locals.el, because the version of
the installed package is part of its file path?

> Anyway, because the directories under ~/.emacs.d/elpa are somewhat
> special and/or security-vetted it _could_ make sense to add them to
> the default value of the variable. This would amount to more or less
> the same as calling the underlying process with `-f
> package-initialize` I think.
>
> But I'm still not sure this should be the default, or merely an option
> to the flymake-elisp-byte-compile backend.  I think the second is
> safer.

Both possibilities are fine to me.

Another option could be to parse the header section of the current
buffer for `Package-Requires:' clause, and then automatically add listed
dependencies to the `elisp-flymake-byte-compile-load-path'.

In this case, flymake would still had operated in quite restricted
environment, but at the same time it'd recognize package dependencies.

Would that make sense?

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> When I was first coding this backend, I had that expectation too.
> But it's not very useful, as you normally, when developing an elisp
> file in a package, you want to be made aware of the potential compilation
> problems arising when compiling that file in a much simpler environment,
> very often the Emacs -Q environment where Makefiles usually byte-compile 
> files.

That's a good point.

> And there's also the question of security.  Flymake runs compile-time 
> code every time by simply modifying the buffer.  So being conservative
> here is also a good idea because of that.

Speaking of which, I was surprised that flymake adds "./" to the load
path -- we never use that in real Emacsen exactly because of security
considerations (we don't want to pick up stray files when working under
/tmp/, for instance).  

> Anyway, because the directories under ~/.emacs.d/elpa are somewhat special
> and/or security-vetted it _could_ make sense to add them to the default
> value of the variable. This would amount to more or less the same as 
> calling the underlying process with `-f package-initialize` I think.
>
> But I'm still not sure this should be the default, or merely an option to 
> the flymake-elisp-byte-compile backend.  I think the second is safer.

An option here would be nice, yes.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> João Távora <joaotavora@gmail.com> writes:

> Speaking of which, I was surprised that flymake adds "./" to the load
> path -- we never use that in real Emacsen exactly because of security
> considerations (we don't want to pick up stray files when working under
> /tmp/, for instance).

Right.  Adding anything to the load path is "dangerous".  The default
"./" is a good compromise, as it enables developing packages with
multiple .el files that require each other in the same dir, which is a
very common thing IME.

>> Anyway, because the directories under ~/.emacs.d/elpa are somewhat special
>> and/or security-vetted it _could_ make sense to add them to the default
>> value of the variable. This would amount to more or less the same as 
>> calling the underlying process with `-f package-initialize` I think.
>>
>> But I'm still not sure this should be the default, or merely an option to 
>> the flymake-elisp-byte-compile backend.  I think the second is safer.
>
> An option here would be nice, yes.

Here's a very minimally tested patch:

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 0c4a9bfdbe..7e1141acf1 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -2119,6 +2119,11 @@ elisp-flymake-byte-compile-load-path
                                   (dolist (path x t) (unless (stringp path)
                                                        (throw 'tag nil)))))))
 
+(defcustom elisp-flymake-byte-compile-use-elpa-dirs nil
+  "If non-nil, add ELPA package dirs to elisp Flymake load path."
+  :type 'boolean
+  :group 'lisp)
+
 ;;;###autoload
 (defun elisp-flymake-byte-compile (report-fn &rest _args)
   "A Flymake backend for elisp byte compilation.
@@ -2146,6 +2151,8 @@ elisp-flymake-byte-compile
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
                    ,@(mapcan (lambda (path) (list "-L" path))
                              elisp-flymake-byte-compile-load-path)
+                   ,@(when elisp-flymake-byte-compile-use-elpa-dirs
+                       `("-f" "package-initialize"))
                    "-f" "elisp-flymake--batch-compile-for-flymake"
                    ,temp-file)
         :connection-type 'pipe

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> Right.  Adding anything to the load path is "dangerous".  The default
> "./" is a good compromise, as it enables developing packages with
> multiple .el files that require each other in the same dir, which is a
> very common thing IME.

I'm not sure that's a good compromise at all -- the user has surely set
up the correct load path to use, and overriding that with "./" sounds
like a recipe for disaster.

> Here's a very minimally tested patch:

[...]

> +(defcustom elisp-flymake-byte-compile-use-elpa-dirs nil
> +  "If non-nil, add ELPA package dirs to elisp Flymake load path."
> +  :type 'boolean
> +  :group 'lisp)

I think it would make more sense to have an option to use the load path
from the current Emacs incantation also in the flymake Emacsen.  But
that would probably be more difficult to achieve, as you have to somehow
convey that to the flymake Emacsen (and the load path can be very long,
so it's probably problematic to have that on the command line).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Fri, Jul 22, 2022, 20:52 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > Right.  Adding anything to the load path is "dangerous".  The default
> > "./" is a good compromise, as it enables developing packages with
> > multiple .el files that require each other in the same dir, which is a
> > very common thing IME.
>
> I'm not sure that's a good compromise at all -- the user has surely set
> up the correct load path to use, and overriding that with "./" sounds
> like a recipe for disaster.
>

We seem to be regressing. I thought we had established that the subprocess
elisp load path has no useful relation to the load path of the session.
This is how one achieves consistency  of diagnostics in the same file, but
across sessions.

I've never had disaster struck because of load paths. What disaster are you
thinking about?

> Here's a very minimally tested patch:
>
> [...]
>
> > +(defcustom elisp-flymake-byte-compile-use-elpa-dirs nil
> > +  "If non-nil, add ELPA package dirs to elisp Flymake load path."
> > +  :type 'boolean
> > +  :group 'lisp)
>
> I think it would make more sense to have an option to use the load path
> from the current Emacs incantation also in the flymake Emacsen.
>

For reasons already explained, i completely disagree. My proposal would fix
exactly this bug report. But you can add other options, as long as you
don't break the current default. Do you use Flymake mode for elisp? I've
been using it for many years, and the current default is very good. I don't
use ELPA or develop against it, though, as the bug reporter fired. We could
also see what Flycheck does in this regard. It also has an elisp checker.

João

>

[-- Attachment #2: Type: text/html, Size: 2775 bytes --]

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> I've never had disaster struck because of load paths. What disaster
> are you thinking about?

The same disaster that having "./" in `load-path' in general would lead
to.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On Fri, Jul 22, 2022, 22:12 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > I've never had disaster struck because of load paths. What disaster
> > are you thinking about?
>
> The same disaster that having "./" in `load-path' in general would lead
> to.
>

If you don't elaborate, we have no way of understanding whether this is a
genuine expansion of the "disaster vector" that is already intrinsic to
this particular Flymake backend.

João

>

[-- Attachment #2: Type: text/html, Size: 1156 bytes --]

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> If you don't elaborate, we have no way of understanding whether this
> is a genuine expansion of the "disaster vector" that is already
> intrinsic to this particular Flymake backend.

I think I already mentioned the problem of editing files in /tmp/?
That's the whole point of not having ./ in load-path -- you can
inadvertently load code under control of an attacker.

It seems to me that there's two useful values for load-path in the
Flymake backend: Either just the standard load-path (so that you
actually get the same results as when doing a batch byte-compile) or the
current running load-path (so that you get the same results as when you
`require' the file from your .emacs, say).  Altering the load-path to
also include the ELPA directories doesn't really help much, because
people have all kinds of code that's not in ELPA (but is in their
load-path).

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> If you don't elaborate, we have no way of understanding whether this
>> is a genuine expansion of the "disaster vector" that is already
>> intrinsic to this particular Flymake backend.
>
> I think I already mentioned the problem of editing files in /tmp/?
> That's the whole point of not having ./ in load-path -- you can
> inadvertently load code under control of an attacker.

As I've been trying to explain, flymake-elisp-byte-compile is all about
"inadvertently loading code": it's -- literally -- constantly and
largely unpredictably byte-compiling a file containing the transient
contents of your buffer.  As you know, in Lisp, that is always running
code, arbitrary code.

It's not at all comparable to the interactive Emacs usage where the user
takes voluntary action to execute something.

Flymake in Elisp files is inherently insecure in this respect.  This is
why we don't turn it on by default.  Maybe this should feature more
prominently somewhere: "Users must NOT turn on
flymake-elisp-byte-compile automatically if they are not aware of the
risks."

At some point in the past, Stefan was working on a "sandboxed" Emacs
that could, in theory, pave the way for automatically enabled Elisp
Flymake, but I haven't heard of that effort lately.

> It seems to me that there's two useful values for load-path in the
> Flymake backend: Either just the standard load-path (so that you
> actually get the same results as when doing a batch byte-compile) or the
> current running load-path (so that you get the same results as when you
> `require' the file from your .emacs, say).  Altering the load-path to
> also include the ELPA directories doesn't really help much, because
> people have all kinds of code that's not in ELPA (but is in their
> load-path).

I think we have to ask ourselves: what is Flymake used for?  The most
useful answers will come from the people who actually use it, though
potential uses are also interesting.

I for one use it to develop Elisp package.el packages that I later
publish to GNU ELPA and MELPA so that other users, my "clients", are
to be able to use in their Emacsen.

The ELPA packages _I_ develop only ever depend on packages in Emacs core
(at most they are :core ELPA packages).  But it seems, quite reasonably,
that Max's packages do also depend on packages that are not in Emacs
core, but in some xELPA repo.

Why is Flymake useful to me in the state it is now?  Because, given a
transient state of my Elisp buffers, it helps accurately predict the
byte-compilation warnings and errors that those clients would experience
were they to grab and install my package at state I have it in front of
me.

Having './' in the default load-path for elisp-flymake-byte-compile is
fundamental for the accuracy of this prediction.  Why?  Because the
clients of my packages -- regardless if they use package.el,
straight.el, etc or just simply using a git checkout -- will always have
the the files I have in some directory in some other directory in their
machines, and _that_ directory will be in the load-path.

Can Flymake be useful in some other situation?  Perhaps, who knows?  But
I for one never use it to obtain largely the same results I can already
get with M-x elisp-byte-compile-file.  But if you do want to use it like
that, feel free to add an option to inherit the current session's load
path.

I'd say we should first fix Max's problem, this bug report's problem,
which I can perfectly understand.  I think the correct way is with the
patch I submitted, though we can also ask max to add the path of each of
his package's dependencies to some dir-local value of
elisp-flymake-byte-compile-load-path.  Not very practical, IMO.

Finally, if you want to remove "./" from the automatic load path of
elisp-flymake-byte-compile, no problem.  It's of course your call: I'll
just add it to elisp-flymake-byte-compile-load-path in my session and be
done with it.  IMO Something like this could be justified if it were
plugging an important security hole.  But in my opinion we're closing a
window in a house with no roof and doesn't justify breaking people's
workflows.

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> At some point in the past, Stefan was working on a "sandboxed" Emacs
> that could, in theory, pave the way for automatically enabled Elisp
> Flymake, but I haven't heard of that effort lately.

I need to get back to that indeed :-)

>> It seems to me that there's two useful values for load-path in the
>> Flymake backend: Either just the standard load-path (so that you
>> actually get the same results as when doing a batch byte-compile) or the
>> current running load-path (so that you get the same results as when you
>> `require' the file from your .emacs, say).  Altering the load-path to
>> also include the ELPA directories doesn't really help much, because
>> people have all kinds of code that's not in ELPA (but is in their
>> load-path).

AFAIC, it's not just `load-path`: the set of autoloaded functions (and
a few other similar things) is also relevant.

> I think we have to ask ourselves: what is Flymake used for?  The most
> useful answers will come from the people who actually use it, though
> potential uses are also interesting.

I don't think we can hope to make flymake-elisp work correctly in all
existing cases, because there are conflicting requirements there.

So, we should take it for granted that some use-cases will be considered
as "unsupported", and the important thing is to figure out what behavior
to provide such that all(?) use-cases can be adapted (and such that the
behavior is sane enough to be described, understandable, and
predictable).

> Having './' in the default load-path for elisp-flymake-byte-compile is
> fundamental for the accuracy of this prediction.  Why?  Because the
> clients of my packages -- regardless if they use package.el,
> straight.el, etc or just simply using a git checkout -- will always have
> the the files I have in some directory in some other directory in their
> machines, and _that_ directory will be in the load-path.

BTW, while the GNUmakefile of `elpa-admin` also adds `.` to the
`load-path`, there are cases where this is harmful.
E.g. the "pcase benchmark" in `elisp-benchmarks` used to be in the file
.../benchmarks/pcase.el and it (of course) required Emacs to load
`pcase.el` (the other one).

This required the hideous workaround:

    (eval-and-compile
      ;; ¡FIXME!  The GNUmakefile of elpa.git uses:
      ;;
      ;;    ... -L $(dir $@) -f batch-byte-compile $<
      ;;
      ;; to compile each file.  This is handy for some cases such as files in
      ;; `contrib' subdirectories but for this `pcase.el' file it causes this
      ;; `pcase.el' to hide the *real* `pcase.el'.  So we workaround this problem
      ;; here by removing the offending element from `load-path'.  Yuck!
      ;;
      ;; We should probably change GNUmakefile instead so it doesn't forcefully
      ;; add the directory to `load-path', e.g. make this dependent on the
      ;; presence of special file like `.dont-add-to-load-path'. 
      (when load-file-name
        (setq load-path (remove (file-name-directory load-file-name) load-path))))

We have several files in `lisp` whose directory is not in `load-path`
(most of them under `lisp/cedet`).

But, note that I decided to use the above hack (later replaced by
the simpler solution of renaming the file to `elb-pcase.el`) in
preference to changing the GNUmakefile not to add `.` to `load-path`.


        Stefan

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Max Brieiev]

João Távora <joaotavora@gmail.com> writes:

> I think we have to ask ourselves: what is Flymake used for?  The most
> useful answers will come from the people who actually use it, though
> potential uses are also interesting.

For a regular user like me, the most obvious use cases are:

1. Utilities for my own use. This can be sloppy code, heavily dependent
on my current Emacs setup. But it works, or at least it can be required
into init.el. So it is counter-intuitive, when flymake displays the
error, but running `restart-emacs' reveals no errors.

2. If I would decide to distribute my code through (M)ELPA, this is
where I'd like flymake to be more finicky: probably compile my code only
against standard load-path _and_ dependencies (packages listed in
"Package-Requires" header section).

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

Max Brieiev <max.brieiev@gmail.com> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> I think we have to ask ourselves: what is Flymake used for?  The most
>> useful answers will come from the people who actually use it, though
>> potential uses are also interesting.
>
> For a regular user like me, the most obvious use cases are:
>
> 1. Utilities for my own use. This can be sloppy code, heavily dependent
> on my current Emacs setup. But it works, or at least it can be required
> into init.el. So it is counter-intuitive, when flymake displays the
> error, but running `restart-emacs' reveals no errors.

Another way to see this is elisp-flymake-byte-compile is rooting for
less sloppy code :-), i.e. code that you can share with me.

> 2. If I would decide to distribute my code through (M)ELPA, this is
> where I'd like flymake to be more finicky: probably compile my code only
> against standard load-path _and_ dependencies (packages listed in
> "Package-Requires" header section).

... and if your hypothetical.el package comes more than one file, say
hypothetical-tests.el or hypothetical-utils.el which require each other,
you'd probably also like "./" to be in the load-path for
elisp-flymake-byte-compile.

João

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> At some point in the past, Stefan was working on a "sandboxed" Emacs
>> that could, in theory, pave the way for automatically enabled Elisp
>> Flymake, but I haven't heard of that effort lately.
>
> I need to get back to that indeed :-)
>
>>> It seems to me that there's two useful values for load-path in the
>>> Flymake backend: Either just the standard load-path (so that you
>>> actually get the same results as when doing a batch byte-compile) or the
>>> current running load-path (so that you get the same results as when you
>>> `require' the file from your .emacs, say).  Altering the load-path to
>>> also include the ELPA directories doesn't really help much, because
>>> people have all kinds of code that's not in ELPA (but is in their
>>> load-path).
>
> AFAIC, it's not just `load-path`: the set of autoloaded functions (and
> a few other similar things) is also relevant.

I presume those relevant things are setup by package-initialize, right?
My proposed patch uses that.

>> I think we have to ask ourselves: what is Flymake used for?  The most
>> useful answers will come from the people who actually use it, though
>> potential uses are also interesting.
>
> I don't think we can hope to make flymake-elisp work correctly in all
> existing cases, because there are conflicting requirements there.
>
> So, we should take it for granted that some use-cases will be considered
> as "unsupported", and the important thing is to figure out what behavior
> to provide such that all(?) use-cases can be adapted (and such that the
> behavior is sane enough to be described, understandable, and
> predictable).
>
>> Having './' in the default load-path for elisp-flymake-byte-compile is
>> fundamental for the accuracy of this prediction.  Why?  Because the
>> clients of my packages -- regardless if they use package.el,
>> straight.el, etc or just simply using a git checkout -- will always have
>> the the files I have in some directory in some other directory in their
>> machines, and _that_ directory will be in the load-path.
>
> BTW, while the GNUmakefile of `elpa-admin` also adds `.` to the
> `load-path`, there are cases where this is harmful.

I don't dispute that, but in my experience (and as far as I can see) it
is _not_ functionally harmful to have ./ in the
elisp-flymake-byte-compile-load-path for the use case that I described:
developing package.el packages that can be distributed and installed
through a number of ways.

But is it potentially "dangerous/disastrous"?  I don't think so either,
but I haven't a clear picture of the disaster scenario.  Lars mentioned
"editing files in /tmp".

Maybe Lars is worried about some user e.g. having /tmp/bad.el and
opening some /tmp/good.el and slowly typing in

   (require 'badminton)

By the time (require 'bad) is in the buffer, disaster strikes.  But this
disaster could just as well happen by simply visiting /tmp/bad.el to see
what's in it.  Except that malicious files don't advertise themselves
like that in their file names...

Or maybe -- again, I'm just guessing -- the danger is that that bad.el
is disguised under /tmp/pcase.el and /tmp/good.el has a perfectly
legitimate.

   (require 'pcase)

Simply visitng /tmp/good.el with Flymake on would lead to disaster.  If
that's the case, it's as easy as applying this patch

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 0c4a9bfdbe..01c0679c76 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -2144,7 +2144,7 @@ elisp-flymake-byte-compile
                    "-Q"
                    "--batch"
                    ;; "--eval" "(setq load-prefer-newer t)" ; for testing
-                   ,@(mapcan (lambda (path) (list "-L" path))
+                   ,@(mapcan (lambda (path) (list "-L" (format ":%s" path)))
                              elisp-flymake-byte-compile-load-path)
                    "-f" "elisp-flymake--batch-compile-for-flymake"
                    ,temp-file)

João

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> AFAIC, it's not just `load-path`: the set of autoloaded functions (and
>> a few other similar things) is also relevant.
> I presume those relevant things are setup by package-initialize, right?

Typically, yes (tho nowadays you don't want to call `package-initialize`
but `package-activate-all` which can do much less work).


        Stefan

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> Or maybe -- again, I'm just guessing -- the danger is that that bad.el
> is disguised under /tmp/pcase.el and /tmp/good.el has a perfectly
> legitimate.
>
>    (require 'pcase)
>
> Simply visitng /tmp/good.el with Flymake on would lead to disaster.

Yes.  Sorry, I thought it was self-evident that that's the problem I was
talking about with having "./" in load-path.

> If
> that's the case, it's as easy as applying this patch

[...]

> -                   ,@(mapcan (lambda (path) (list "-L" path))
> +                   ,@(mapcan (lambda (path) (list "-L" (format ":%s" path)))
>                               elisp-flymake-byte-compile-load-path)

That would be a distinct improvement; yes.  (But with a comment about
what that does, because it's not self explanatory.)

bug#48452: 28.0.50; flymake for elisp does not respect `load-path`

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Sun, Jul 24, 2022 at 10:18 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > Or maybe -- again, I'm just guessing -- the danger is that that bad.el
> > is disguised under /tmp/pcase.el and /tmp/good.el has a perfectly
> > legitimate.
> >
> >    (require 'pcase)
> >
> > Simply visitng /tmp/good.el with Flymake on would lead to disaster.
>
> Yes.  Sorry, I thought it was self-evident that that's the problem I was
> talking about with having "./" in load-path.
>

OK. I just hope that this thread has left it clear that simply visiting that
/tmp/pcase-not-malicious-at-all.el can lead to "disaster" regardless
of the value in elisp-flymake-byte-compile-load-path .

> If
> > that's the case, it's as easy as applying this patch
>
> [...]
>
> > -                   ,@(mapcan (lambda (path) (list "-L" path))
> > +                   ,@(mapcan (lambda (path) (list "-L" (format ":%s"
> path)))
> >                               elisp-flymake-byte-compile-load-path)
>
> That would be a distinct improvement; yes.  (But with a comment about
> what that does, because it's not self explanatory.)
>
> OK, I can do that.

João Távora

[-- Attachment #2: Type: text/html, Size: 1989 bytes --]