bug#20241: 25.0.50; `setq' with only one argument

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

See for example: `(setq mark-active)' in `compilation-goto-locus'
(compile.el).

setq' should not be called with only one argument.  It "works", with the
effect of assigning a value of nil, but it is poor form.  Typically this
is a sign of a typo.

Beyond this particular occurrence, I think that a wrong-number-of-args
error should be raised when `setq' is called with an odd number of
arguments.  That certainly corresponds to the syntax that is documented.
And it corresponds to the doc explanations that using `setq' is the same
as using `set' and quoting the variable.  And it corresponds to the
definition and behavior of `setq' in Common Lisp.



In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

bug#20241: 25.0.50; `setq' with only one argument

[Stefan Monnier]

> Beyond this particular occurrence, I think that a wrong-number-of-args
> error should be raised when `setq' is called with an odd number of
> arguments.

At the very least the byte-compiler should warn about it, indeed.


        Stefan

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

In article <mailman.3138.1427900048.31049.bug-gnu-emacs@gnu.org> you wrote:
> See for example: `(setq mark-active)' in `compilation-goto-locus'
> (compile.el).

> setq' should not be called with only one argument.  It "works", with the
> effect of assigning a value of nil, but it is poor form.  Typically this
> is a sign of a typo.

Fixed in emacs-25.  The interpreter now signals a wrong-number-of-args
error, and the byte compiler issues a warning.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Glenn Morris]

Alan Mackenzie wrote:

>> setq' should not be called with only one argument.  It "works", with the
>> effect of assigning a value of nil, but it is poor form.  Typically this
>> is a sign of a typo.
>
> Fixed in emacs-25.  The interpreter now signals a wrong-number-of-args
> error, and the byte compiler issues a warning.

What on earth is this incompatible change doing in a feature-frozen
release branch?

Also, the combination of a compiler _warning_ and a run-time _error_
makes no sense.

bug#20241: 25.0.50; `setq' with only one argument

[Glenn Morris]

>> Fixed in emacs-25.  The interpreter now signals a wrong-number-of-args
>> error, and the byte compiler issues a warning.
>
> What on earth is this incompatible change doing in a feature-frozen
> release branch?

Surely the sensible way to do this is just start with the compilation
warning for now.

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Glenn Morris <rgm@gnu.org> writes:

> Surely the sensible way to do this is just start with the compilation
> warning for now.

I'm ready for it to be an error, Glenn. It should happen sometime, 25.1 is a
major release, and so it is a better time to do it than 25.2.

John

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Glenn.

On Mon, Nov 23, 2015 at 01:44:51PM -0500, Glenn Morris wrote:
> Alan Mackenzie wrote:

> >> setq' should not be called with only one argument.  It "works", with the
> >> effect of assigning a value of nil, but it is poor form.  Typically this
> >> is a sign of a typo.
> >
> > Fixed in emacs-25.  The interpreter now signals a wrong-number-of-args
> > error, and the byte compiler issues a warning.

> What on earth is this incompatible change doing in a feature-frozen
> release branch?

It's a bug fix.

> Also, the combination of a compiler _warning_ and a run-time _error_
> makes no sense.

Maybe, maybe not.  It's not the byte compiler's habit to signal errors:
it only does so in most exceptional circumstances.  Possibly the
thinking is that it's best to produce a .elc anyway, which can be
inspected, rather than just to error out.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Glenn.

On Mon, Nov 23, 2015 at 07:31:38PM +0000, Alan Mackenzie wrote:
> On Mon, Nov 23, 2015 at 01:44:51PM -0500, Glenn Morris wrote:
> > Alan Mackenzie wrote:

> > >> setq' should not be called with only one argument.  It "works", with the
> > >> effect of assigning a value of nil, but it is poor form.  Typically this
> > >> is a sign of a typo.
> > >
> > > Fixed in emacs-25.  The interpreter now signals a wrong-number-of-args
> > > error, and the byte compiler issues a warning.

> > What on earth is this incompatible change doing in a feature-frozen
> > release branch?

> It's a bug fix.

> > Also, the combination of a compiler _warning_ and a run-time _error_
> > makes no sense.

> Maybe, maybe not.  It's not the byte compiler's habit to signal errors:
> it only does so in most exceptional circumstances.  Possibly the
> thinking is that it's best to produce a .elc anyway, which can be
> inspected, rather than just to error out.

Apologies for the above: the byte compiler does indeed routinely signal
errors as errors.  I will change this "odd number of arguments" warning
into an error.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Glenn Morris]

I'm assuming that any uses of this form were intentional (or even if
not intentional, were doing the right thing), rather than errors.
(I have no actual data - do you?) So making such code stop working
overnight seems too harsh to me.

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Glenn.

On Tue, Nov 24, 2015 at 12:38:18PM -0500, Glenn Morris wrote:

> I'm assuming that any uses of this form were intentional (or even if
> not intentional, were doing the right thing), rather than errors.

That wasn't the case.  There were 5 deliberate uses of `setq' without
the nil, there was one error in bytecomp.el, which had the effect of
suppressing warnings given out by the byte compiler.

> (I have no actual data - do you?) So making such code stop working
> overnight seems too harsh to me.

I have data, yes, see above.  The point is, that error in bytecomp.el
could have been anywhere, causing random breakage.  I only noticed it by
accident.

I don't think the value of allowing an implicit, but undocumented, nil
at the end of a `setq' outweighs the danger of accidentally setting a
variable to nil.  There were, after all, only five uses of this
"feature" in the entire Emacs code base.  And one of these was in
.../obsolete.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

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

On 24 Nov 2015 6:04 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> There were, after all, only five uses of this
> "feature" in the entire Emacs code base.  And one of these was in
> .../obsolete.

A few months ago Stefan changed save-excursion to no longer restore the
mark. IIRC (though I may be forgetting), there were zero uses of this in
the codebase, and he even asked about it on the list and nobody said "I use
this".
Still, after the change was applied, some issues came up on a couple of
highly used packages out there, and my guess is that more will come up once
25 is released.

I'm not speaking against that change. It was a good change because it fixed
more problems than it created. And the same is true here. This change will
be good.

However, if there are 5 uses of it in the core, then it's likely there are
30 or 100 uses of it in the wild. This change _will_ break things. So it is
my opinion that it should go in the master branch. The feature freeze
exists for a reason.

The emacs 25 branch should only get the warning, instead. A warning really
goes a long way, and it doesn't break anything.

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

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Artur.

On Tue, Nov 24, 2015 at 07:26:16PM +0000, Artur Malabarba wrote:
> On 24 Nov 2015 6:04 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> > There were, after all, only five uses of this
> > "feature" in the entire Emacs code base.  And one of these was in
> > .../obsolete.

> A few months ago Stefan changed save-excursion to no longer restore the
> mark. IIRC (though I may be forgetting), there were zero uses of this in
> the codebase, and he even asked about it on the list and nobody said "I use
> this".
> Still, after the change was applied, some issues came up on a couple of
> highly used packages out there, and my guess is that more will come up once
> 25 is released.

> I'm not speaking against that change. It was a good change because it fixed
> more problems than it created. And the same is true here. This change will
> be good.

> However, if there are 5 uses of it in the core, then it's likely there are
> 30 or 100 uses of it in the wild. This change _will_ break things. So it is
> my opinion that it should go in the master branch. The feature freeze
> exists for a reason.

This is a bug fix.  One might debate whether or not allowing implicit
nils in setq is a bug or not.  But the consequence was that our byte
compiler was broken.  Maybe not badly broken, but broken nonetheless.

On finding a bug, I always try to fix not just the bug itself, but the
cause of the bug.  It seems clear to me that the ultimate cause of the
broken byte compiler was the lack of a check on `setq'.

> The emacs 25 branch should only get the warning, instead. A warning really
> goes a long way, and it doesn't break anything.

A warning does indeed go a long way.  However, the breakage which will
happen out in the field is not serious.  A small number of .el files
will fail to compile, and will do so with a clear error message.  I
suspect a larger number of bugs will be revealed than working code
broken.

I favour leaving the situation as it is.  However, it is for John to
decide.  If he comes round to the view that an error is too strong, I am
willing to turn it back into a warning.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Alan Mackenzie <acm@muc.de> writes:

> I favour leaving the situation as it is. However, it is for John to decide.
> If he comes round to the view that an error is too strong, I am willing to
> turn it back into a warning.

I would like it to appear in 25.1. I'm willing to err against caution this
time. It would be stranger to me to have it go from a warning to an error
within 25.x, which then means waiting until 26 for it to be an error.

Now that it's in the release branch, how about we try out some large packages
and see if it's actually a problem? Since I can't imagine anyone intentionally
using this just to save themselves from typing "nil", I'd expect people would
want to know that their setq is ill-formed. It might indicate a real problem.

John

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, John.

On Tue, Nov 24, 2015 at 05:46:23PM -0800, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:

> > I favour leaving the situation as it is. However, it is for John to decide.
> > If he comes round to the view that an error is too strong, I am willing to
> > turn it back into a warning.

> I would like it to appear in 25.1. I'm willing to err against caution this
> time. It would be stranger to me to have it go from a warning to an error
> within 25.x, which then means waiting until 26 for it to be an error.

> Now that it's in the release branch, how about we try out some large packages
> and see if it's actually a problem?

We won't see any problem in our own sources, because all the occurrences
here have been expunged.  Any problems will be with external libraries
(or possibly in *ELPA) which people expect just to download, compile and
run.  I think that was what Artur is worried about.

> Since I can't imagine anyone intentionally using this just to save
> themselves from typing "nil", I'd expect people would want to know
> that their setq is ill-formed. It might indicate a real problem.

Of the five non-error former occurrences, all were very old - the last
one was touched by Stefan in 2005 (according to git blame).  Some of
them, possibly all of them, were indeed deliberate: there was a space
left between the variable and the closing paren.

Personally, I think there will be very few in external sources, and we
are more likely to flag up real errors than break working code.

> John

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

> We won't see any problem in our own sources, because all the occurrences
> here have been expunged.  Any problems will be with external libraries
> (or possibly in *ELPA) which people expect just to download, compile and
> run.  I think that was what Artur is worried about.

Since we're going with this, I quickly grepped 4 occurrences on
elpa/packages (didn't try the external branches). We should probably
fix those too:

./hydra/hydra-test.el:1312:              (setq current-prefix-arg)
./tiny/tiny.el:364:                             (setq expect-fun)
./gnorb/gnorb-registry.el:294:    (setq id )
./context-coloring/benchmark/fixtures/subr.el:1448:     ,@(mapcar
(lambda (binder) `(setq ,@binder)) binders)

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Artur.

On Wed, Nov 25, 2015 at 10:36:25AM +0000, Artur Malabarba wrote:
> > We won't see any problem in our own sources, because all the occurrences
> > here have been expunged.  Any problems will be with external libraries
> > (or possibly in *ELPA) which people expect just to download, compile and
> > run.  I think that was what Artur is worried about.

> Since we're going with this, I quickly grepped 4 occurrences on
> elpa/packages (didn't try the external branches). We should probably
> fix those too:

> ./hydra/hydra-test.el:1312:              (setq current-prefix-arg)
> ./tiny/tiny.el:364:                             (setq expect-fun)
> ./gnorb/gnorb-registry.el:294:    (setq id )
> ./context-coloring/benchmark/fixtures/subr.el:1448:     ,@(mapcar
> (lambda (binder) `(setq ,@binder)) binders)

Hmm.  That's worrying.  Isn't the code in ELPA newer code than in the
core?

If you found four occurrences merely by grepping, there could well be
quite a few more.  In the last one, for example, can we be sure that nil
is intended, not that the real argument has just been missed out?

Is there a way of building the entire repository in batch mode?  I've
not had anything to do with ELPA, as yet.  Where do I find the address
of its repository?

Maybe having the byte compiler error out in this situation isn't such a
brilliant idea after all.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

(I'll try to address your other comments later)

> Is there a way of building the entire repository in batch mode?

I think the readme explains how to do that. There's a make command for
it that I can't recall right now.

> Where do I find the address
> of its repository?

SAVANNAH-USERNAME@git.sv.gnu.org:/srv/git/emacs/elpa.git

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Artur.

On Wed, Nov 25, 2015 at 11:13:41AM +0000, Artur Malabarba wrote:
> (I'll try to address your other comments later)

No hurry!

> > Is there a way of building the entire repository in batch mode?

> I think the readme explains how to do that. There's a make command for
> it that I can't recall right now.

> > Where do I find the address
> > of its repository?

> SAVANNAH-USERNAME@git.sv.gnu.org:/srv/git/emacs/elpa.git

Thanks, I've got it.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

> If you found four occurrences merely by grepping, there could well be
> quite a few more.  In the last one, for example, can we be sure that nil
> is intended, not that the real argument has just been missed out?

If you cannot now analyze the code properly to determine TRT, or
contact the author to make that determination, then do the obvious
(IMO): Assign a `nil' value explicitly where it is now assigned
implicitly.

AND flag that amended code with a comment saying what you've done,
and that you don't currently know whether (a) there is a bug here
or (b) `nil' is really what is needed.

IOW: (1) At least just ensure that the code does the same thing
that it does now, but respects the intended meaning of `setq'.
(2) If you have to punt the careful analysis for later or for
someone else, then add a comment to that effect.

> Maybe having the byte compiler error out in this situation isn't such a
> brilliant idea after all.

IMO, the bug should be fixed to raise an _error at runtime_, for
both interpreted and byte-compiled code.

Whatever else the byte-compiler might do is less important, as
long as it produces code that is comparable - e.g., code that
will also raise an _error at runtime_.

It's OK for a byte-compiler to be smart, but not smarter than
the actual source code ;-).  It should pretty much try to
produce code that does the same thing, but hopefully usually
does it quicker.

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Drew.

On Wed, Nov 25, 2015 at 07:18:17AM -0800, Drew Adams wrote:
> > If you found four occurrences merely by grepping, there could well be
> > quite a few more.  In the last one, for example, can we be sure that nil
> > is intended, not that the real argument has just been missed out?

> If you cannot now analyze the code properly to determine TRT, or
> contact the author to make that determination, then do the obvious
> (IMO): Assign a `nil' value explicitly where it is now assigned
> implicitly.

That would be the worst thing: it would leave the code possibly failing
exactly the way it did, but remove the evidence (which can be
mechanically found).

> AND flag that amended code with a comment saying what you've done,
> and that you don't currently know whether (a) there is a bug here
> or (b) `nil' is really what is needed.

Comments are less good than error messages from the byte compiler!

> IOW: (1) At least just ensure that the code does the same thing
> that it does now, but respects the intended meaning of `setq'.
> (2) If you have to punt the careful analysis for later or for
> someone else, then add a comment to that effect.

> > Maybe having the byte compiler error out in this situation isn't such a
> > brilliant idea after all.

> IMO, the bug should be fixed to raise an _error at runtime_, for
> both interpreted and byte-compiled code.

I've just tried it out.  The byte compiler generates an error message,
but the .elc is written anyway.  No runtime error happens from such
compiled code.  But running it interpreted would signal an error.

> Whatever else the byte-compiler might do is less important, as
> long as it produces code that is comparable - e.g., code that
> will also raise an _error at runtime_.

It currently doesn't do that.  I'm not convinced it should, either.

> It's OK for a byte-compiler to be smart, but not smarter than
> the actual source code ;-).  It should pretty much try to
> produce code that does the same thing, but hopefully usually
> does it quicker.

Why is this topic suddenly seeming complicated?  :-(

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

> > If you cannot now analyze the code properly to determine TRT, or
> > contact the author to make that determination, then do the obvious
> > (IMO): Assign a `nil' value explicitly where it is now assigned
> > implicitly.
> 
> That would be the worst thing: it would leave the code possibly failing
> exactly the way it did, but remove the evidence (which can be
> mechanically found).

It would leave the code behaving exactly as it did before.  AND it
would add information to developers that there is possibly an error
here - right here, in this sexp here.  AND it would say clearly what
that possible error is.

That doesn't "remove the evidence".  It puts a big sign around the
evidence saying "**EVIDENCE** - Check this code to see whether it
should really should assign a nil value.

But if you can DTRT now - analyze the code sufficiently and fix
it properly or contact the author for an opinion - then that's
the thing to do.

Or if you just leave the code as is, but ensure that a runtime
error is raised, that is also TRT.  It takes care of THIS bug,
but it does not fix that bug (indicated by the error occurrence).

That's fine.  When the error is raised someone can then work on
fixing that bug.  They can take the time to analyze and fix it
properly.  And they can ensure, while they are working on it,
that the code at least runs as it did before - by doing what I
suggested above: explicitly assign `nil' and add a comment.
 
> > AND flag that amended code with a comment saying what you've done,
> > and that you don't currently know whether (a) there is a bug here
> > or (b) `nil' is really what is needed.
> 
> Comments are less good than error messages from the byte compiler!

I disagree.  Each can be helpful in its own way.  In this case,
we have already located the potential problem, and the (right)
message to developers about it is that THERE IS an assignment
of `nil' and IT MIGHT BE PROBLEMATIC.  The source code is a good
place to convey that message, right next to the suspect assignment.

> > IOW: (1) At least just ensure that the code does the same thing
> > that it does now, but respects the intended meaning of `setq'.
> > (2) If you have to punt the careful analysis for later or for
> > someone else, then add a comment to that effect.
> 
> > > Maybe having the byte compiler error out in this situation
> > > isn't such a brilliant idea after all.
> 
> > IMO, the bug should be fixed to raise an _error at runtime_,
> > for both interpreted and byte-compiled code.
> 
> I've just tried it out.  The byte compiler generates an error message,
> but the .elc is written anyway.  No runtime error happens from such
> compiled code.  But running it interpreted would signal an error.

And who will run it interpreted?  THIS is worse than nothing, IMO.

What counts is the runtime behavior.  And that counts for both
source code and byte-compiled code.

> > Whatever else the byte-compiler might do is less important, as
> > long as it produces code that is comparable - e.g., code that
> > will also raise an _error at runtime_.
> 
> It currently doesn't do that.  I'm not convinced it should, either.

What would it take to convince you?  What is the purpose of
byte-compilation, in terms of the resulting code?  Is it to
change the behavior in ways other than optimization?  I don't
think so.

The byte-compiler is not just some QA tool to check whether i's
are dotted and t's are crossed.

It produces _code_ that is _run_, and that runtime behavior
should reflect the programmer's intention.  And that intention
is expressed in the source-code language (which can include
declarations to a compiler, but that's beside the point here).

> > It's OK for a byte-compiler to be smart, but not smarter than
> > the actual source code ;-).  It should pretty much try to
> > produce code that does the same thing, but hopefully usually
> > does it quicker.
> 
> Why is this topic suddenly seeming complicated?  :-(

I don't think it's complicated.  `setq' should raise an error
when it is passed an odd number of arguments.  _That's all._
The error should be raised when `setq' is invoked, obviously.

Anything else we might decide to do is extra communication
to programmers _about_ the code.  But the code itself should
DTRT.  Assigning `nil' for a missing value arg is not the
right thing; raising an error is the right thing.

If you want to leave the problematic code as is, but ensure
that a runtime error is raised, that's fine.  It is the right
thing.

But IF you decide to try to fix the problematic code now, and
IF you feel you are unsure about the fix, THEN the best thing
to do is to keep the behavior as it was (assign `nil'), but
make that behavior explicit and question it with a comment.

I would rather opt for just leaving the code as it is -
erroneous, and let it raise a runtime error.  But then we
will be in the same boat for fixing that error: analyze or
contact the author, etc.

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Drew.

On Wed, Nov 25, 2015 at 08:27:43AM -0800, Drew Adams wrote:

[ .... ]

> But if you can DTRT now - analyze the code sufficiently and fix
> it properly or contact the author for an opinion - then that's
> the thing to do.

That's already been done with the Emacs core, and that's what I'd like
to do with elpa.  But code over which we have no control might be
problematic.

> Or if you just leave the code as is, but ensure that a runtime
> error is raised, that is also TRT.  It takes care of THIS bug,
> but it does not fix that bug (indicated by the error occurrence).

That would mean the users will suffer an error rather than the
maintainers.  And the Emacs team will get blamed for the breakage.

[ .... ]

> > > IMO, the bug should be fixed to raise an _error at runtime_,
> > > for both interpreted and byte-compiled code.

I think that a compiler generating a runtime error for a compile time
problem isn't a good idea.

[ .... ]

> The byte-compiler is not just some QA tool to check whether i's
> are dotted and t's are crossed.

No, but that is a significant part of its job spec.

[ .... ]

> > Why is this topic suddenly seeming complicated?  :-(

> I don't think it's complicated.  `setq' should raise an error
> when it is passed an odd number of arguments.  _That's all._
> The error should be raised when `setq' is invoked, obviously.

There are several different behaviours possible in both the interpreter
and compiler:

1. Interpreter:
  (a) Current behviour: silently insert a nil into the `setq'.
  (b) Throw an error (current new behaviour in emacs-25).
  (c) Issue a warning, but continue otherwise with (a).

2. Compiler:
  (a) Silently compile code which slips in a nil (current behaviour).
  (b) Issue a warning, but compile in the nil.
  (c) Issue an error, but compile in the nil.  (current new behaviour
    in emacs-25.)
  (d) Issue an error, and abort the compilation:
    (i) immediately.
    (ii) at the end of the source file.
  (e) Issue an error, and generate code to:
     (i) issue a warning, but carry on with the nil.
     (ii) throw an error.

You seem to favour 1(b) and, I think, 2(d)(ii) or 2(e)(ii).  I favour
1(b) and probably 2(c), possibly 2(e)(i).  To me, these possibilities
are complicated.  It's not a technical problem, it's a social problem:
who's going to be notified of the problem and when.  I would far rather
maintainers were hassled than users.

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

> 1. Interpreter:
>   (a) Current behviour: silently insert a nil into the `setq'.
>   (b) Throw an error (current new behaviour in emacs-25).
>   (c) Issue a warning, but continue otherwise with (a).

1b is what I prefer.

> 2. Compiler:
>   (a) Silently compile code which slips in a nil (current behaviour).
>   (b) Issue a warning, but compile in the nil.
>   (c) Issue an error, but compile in the nil.  (current new behaviour
>     in emacs-25.)
>   (d) Issue an error, and abort the compilation:
>     (i) immediately.
>     (ii) at the end of the source file.
>   (e) Issue an error, and generate code to:
>      (i) issue a warning, but carry on with the nil.
>      (ii) throw an error.

Code that applies `setq' to an odd number of args is erroneous
syntactically.  The compiler can point out that syntax error.

And it need not (and should not) abort the compilation just
because it reports that syntax error - it can continue to
report other problems.

Or if by "abort" you mean just not produce compiled output (but
continue to analyze the code and report as many problems as
possible), I don't think that is necessary (or good) either,
in this case (provided that the resulting code raises a runtime
error.)

> You seem to favour 1(b) and, I think, 2(d)(ii) or 2(e)(ii).
> I favour 1(b) and probably 2(c), possibly 2(e)(i).

My preference for the compiler is to (a) let you know there
are syntax problems, and where they are, and (b) generate code
that acts the same as the interpreted code: in this case,
raise a runtime error.

> To me, these possibilities are complicated.  It's not a
> technical problem, it's a social problem: who's going to be
> notified of the problem and when.  I would far rather
> maintainers were hassled than users.

As long as the code is erroneous, users should get an error
from it.  If you can fix the code then they won't get an
error.  If you cannot fix it now, then they should get an
error.  If it's important enough that they not get the error
then it's important enough that the code should be fixed
(e.g., before the release).

Anyway, I'm no doubt repeating myself now.  At least you
know now what I prefer for the behavior, including for the
compiler.  FWIW.

And thanks for working on this.

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Drew Adams <drew.adams@oracle.com> writes:

> My preference for the compiler is to (a) let you know there are syntax
> problems, and where they are, and (b) generate code that acts the same as
> the interpreted code: in this case, raise a runtime error.

Let's take a different case as a behavorial example:

  The code: (funcall)

  Interpreted: Raises an error: eval: Wrong number of arguments: funcall, 0

  Byte-compilation: No warnings or errors printed.

  Loading of .elc: Raises an error: load: Wrong number of arguments: funcall, 0

I think that we should be consistent in our behavior. Mis-using Emacs Lisp is
not a reason to fail to byte-compile, even if it is a reason for it to fail to
evaluate or load.

A separate argument could be made that bad code shouldn't compile, but that's
orthogonal to this discussion, and too big a pill to swallow for 25.1.

John

bug#20241: 25.0.50; `setq' with only one argument

[John Mastro]

John Wiegley <jwiegley@gmail.com> wrote:
> Let's take a different case as a behavorial example:
>
>   The code: (funcall)
>
>   Interpreted: Raises an error: eval: Wrong number of arguments: funcall, 0
>
>   Byte-compilation: No warnings or errors printed.
>
>   Loading of .elc: Raises an error: load: Wrong number of arguments: funcall, 0
>
> I think that we should be consistent in our behavior. Mis-using Emacs Lisp is
> not a reason to fail to byte-compile, even if it is a reason for it to fail to
> evaluate or load.
>
> A separate argument could be made that bad code shouldn't compile, but that's
> orthogonal to this discussion, and too big a pill to swallow for 25.1.

As an additional point of comparison, the form `(if)' causes a warning
(but not an error) during byte-compilation.

bad-if.el:3:1:Warning: too few arguments for ‘if’

I checked its behavior since `funcall' is a function whereas `setq' is a
special form, so I was interested to see if the precedent there was
different.

-- 
john

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

> > My preference for the compiler is to (a) let you know there are syntax
> > problems, and where they are, and (b) generate code that acts the same
> > as the interpreted code: in this case, raise a runtime error.
> 
> Let's take a different case as a behavorial example: The code: (funcall)
>   Interpreted: Raises an error: eval: Wrong number of arguments:
>                funcall, 0
>   Byte-compilation: No warnings or errors printed.
>   Loading of .elc: Raises an error: load: Wrong number of arguments:
>                    funcall, 0
> 
> I think that we should be consistent in our behavior. Mis-using Emacs Lisp
> is not a reason to fail to byte-compile, even if it is a reason for it to
> fail to evaluate or load.
> 
> A separate argument could be made that bad code shouldn't compile, but
> that's orthogonal to this discussion, and too big a pill to swallow for
> 25.1.

I think we are agreeing, unless I'm missing something (?).

In the case of `funcall', which is a function, the byte-compiler
could conceivably, in some cases, point out a syntax error that
a missing FUNCTION arg is required for `funcall'.

It could not point that out in all cases.  For example:
(apply #'funcall (some-sexp)), might end up applying `funcall' to
().  But in many cases it could find syntax errors of this kind.

`setq' is not a function (you cannot pass `setq' to `apply'), so
it is generally easier to check its syntax.

Whatever problems the byte-compiler finds and reports are welcome,
but the mere fact of reporting problems does not obviate the need
for runtime errors when incorrect syntax is used.

And certainly byte-compilation should not try to "correct" code
(any more than the interpreter should) by, for example, implicitly
providing a nil arg for `setq'.  That's just plain wrong, IMO.

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

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

On 25 Nov 2015 7:06 pm, "John Wiegley" <jwiegley@gmail.com> wrote:
>
>   The code: (funcall)
>
>   Interpreted: Raises an error: eval: Wrong number of arguments: funcall,
0
>
>   Byte-compilation: No warnings or errors printed.

This is a bug then (probably in the advertised-calling-convention of
funcall). Functions called with the wrong number of arguments are usually
reported as warnings by the byte-compiler.

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

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

> This is a bug then (probably in the advertised-calling-convention of
> funcall). Functions called with the wrong number of arguments are usually
> reported as warnings by the byte-compiler.

Ok, let's go with this then:

    Evaluation:  Run-time error
    Compilation: Compile-time warning
    Execution:   Run-time error (same as Evaluation)

I think that's the convergence point for the three of us, amirite?

John

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

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

On 25 Nov 2015 7:40 pm, "John Wiegley" <jwiegley@gmail.com> wrote:
> Ok, let's go with this then:
>
>     Evaluation:  Run-time error
>     Compilation: Compile-time warning
>     Execution:   Run-time error (same as Evaluation)
>
> I think that's the convergence point for the three of us, amirite?

Yes, that's the final behaviour we should have.
(Though, like I said, I'd prefer it if the errors were postponed to the
next release. And Alan was having second thoughts about being so quick with
the errors too (IIUC, he's investigating Gelpa code now).)

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

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

> On 25 Nov 2015 7:40 pm, "John Wiegley" <jwiegley@gmail.com> wrote:
>> Ok, let's go with this then:
>> 
>> Evaluation: Run-time error
>> Compilation: Compile-time warning
>> Execution: Run-time error (same as Evaluation)
>> 
>> I think that's the convergence point for the three of us, amirite?

> Yes, that's the final behaviour we should have. 
> (Though, like I said, I'd prefer it if the errors were postponed to the next
> release. And Alan was having second thoughts about being so quick with the
> errors too (IIUC, he's investigating Gelpa code now).) 

I'd accept a deprecation warning that notifies it will become an error in
25.2, then.

John

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, John.

On Wed, Nov 25, 2015 at 12:37:20PM -0800, John Wiegley wrote:
> >>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

> > On 25 Nov 2015 7:40 pm, "John Wiegley" <jwiegley@gmail.com> wrote:
> >> Ok, let's go with this then:

> >> Evaluation: Run-time error
> >> Compilation: Compile-time warning
> >> Execution: Run-time error (same as Evaluation)

> >> I think that's the convergence point for the three of us, amirite?

I've committed a fix for the above.  The compiler outputs the same
message it previously did.  The generated code DOESN'T do the first n
assignements when there are 2n+1 arguments; it just signals an error
like Fsetq does.

> > Yes, that's the final behaviour we should have. 
> > (Though, like I said, I'd prefer it if the errors were postponed to the next
> > release. And Alan was having second thoughts about being so quick with the
> > errors too (IIUC, he's investigating Gelpa code now).) 

> I'd accept a deprecation warning that notifies it will become an error in
> 25.2, then.

I've committed the entire change.  If there's going to be pain, let's
just get it over with immediately.

> John

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, John.

On Wed, Nov 25, 2015 at 11:40:15AM -0800, John Wiegley wrote:
> >>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

> > This is a bug then (probably in the advertised-calling-convention of
> > funcall). Functions called with the wrong number of arguments are usually
> > reported as warnings by the byte-compiler.

> Ok, let's go with this then:

>     Evaluation:  Run-time error
>     Compilation: Compile-time warning
>     Execution:   Run-time error (same as Evaluation)

I can live with that.

> I think that's the convergence point for the three of us, amirite?

Things are never that simple.  In evaluation, we now get the signal
"wrong-number-of-args setq 17".  The first eight set operations are not
carried out.

In compilation, currently a warning message (called an "Error") is
emitted, and code is generated to perform 9 set operations (the last one
to nil).  This needs to change.

How about enhancing the definition of setq to say that with 17
arguments, NONE of the assignments are done?  This should be relatively
straight forward to code up in the byte compiler, even in the lexical
binding case.

Or, alternatively, we could explicitly say that whether the first 8
assignments are carried out before signaling the error is undefined.
This should deter over-clever hackers from trying to catch the
wrong-number-of-args error, expecting the code to have done something
predictable.

> John

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Drew Adams]

j> Ok, let's go with this then:
j>     Evaluation:  Run-time error
j>     Compilation: Compile-time warning
j>     Execution:   Run-time error (same as Evaluation)
j> I think that's the convergence point for the three of us, amirite?
 
Exactly what I prefer.

(Dunno the difference between a compiler warning and error in
our context.  To me, this is the compiler letting you know that
your code has a syntax error.  But the form that message takes
is probably a warning.)

a> Or, alternatively, we could explicitly say that whether the first 8
a> assignments are carried out before signaling the error is undefined.
a> This should deter over-clever hackers from trying to catch the
a> wrong-number-of-args error, expecting the code to have done something
a> predictable.

The behavior should altogether be undefined.  This is an
implementation detail that users should not count on.

Probably we do not need to, and should not, say anything
about what happens if you do not respect the syntax.
You will get an error, and it will be up to you to figure
out what side effects might have occurred before the error
was raised.

But IMO it would be best if the `setq' implementation tested
its arglist and raised an error at the outset, before doing
anything.  Even if it is fixed to do that, we should not
advertise it.

bug#20241: 25.0.50; `setq' with only one argument

[John Wiegley]

>>>>> Alan Mackenzie <acm@muc.de> writes:

> How about enhancing the definition of setq to say that with 17
> arguments, NONE of the assignments are done?  This should be relatively
> straight forward to code up in the byte compiler, even in the lexical
> binding case.

I prefer atomic operations whenever possible, so I'd rather than the whole
setq would fail. This catches the error earlier, and leaves the environment in
a condition that makes it easier to reproduce that error (it's possible that
some of the variable settings might change the code path such that the setq is
no longer performed).

John

bug#20241: 25.0.50; `setq' with only one argument

[Johan Bockgård]

John Wiegley <jwiegley@gmail.com> writes:

>>>>>> Drew Adams <drew.adams@oracle.com> writes:
>
>> My preference for the compiler is to (a) let you know there are syntax
>> problems, and where they are, and (b) generate code that acts the same as
>> the interpreted code: in this case, raise a runtime error.
>
> Let's take a different case as a behavorial example:
>
>   The code: (funcall)
>
>   Interpreted: Raises an error: eval: Wrong number of arguments: funcall, 0
>
>   Byte-compilation: No warnings or errors printed.
>
>   Loading of .elc: Raises an error: load: Wrong number of arguments: funcall, 0

(funcall) (print t)

Compile. Load .elc =>

This consistently gives me one of two results:
A segfault, or "Invalid function: 183795961" where the actual
number depends on the timestamp of the .elc file.

bug#20241: 25.0.50; `setq' with only one argument

[Andreas Schwab]

Johan Bockgård <bojohan@gnu.org> writes:

> (funcall) (print t)
>
> Compile. Load .elc =>
>
> This consistently gives me one of two results:
> A segfault, or "Invalid function: 183795961" where the actual
> number depends on the timestamp of the .elc file.

That's a bug in the byte compiler, it shouldn't generate call 0 without
an operand on the stack.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Hello, Andreas.

On Thu, Nov 26, 2015 at 09:58:39AM +0100, Andreas Schwab wrote:
> Johan Bockgård <bojohan@gnu.org> writes:

> > (funcall) (print t)

> > Compile. Load .elc =>

> > This consistently gives me one of two results:
> > A segfault, or "Invalid function: 183795961" where the actual
> > number depends on the timestamp of the .elc file.

> That's a bug in the byte compiler, it shouldn't generate call 0 without
> an operand on the stack.

Try the following fix:


diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index ffe73de..842e73d 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4012,8 +4012,12 @@ byte-compile-while
     (setq byte-compile--for-effect nil)))
 
 (defun byte-compile-funcall (form)
-  (mapc 'byte-compile-form (cdr form))
-  (byte-compile-out 'byte-call (length (cdr (cdr form)))))
+  (if (cdr form)
+      (progn
+        (mapc 'byte-compile-form (cdr form))
+        (byte-compile-out 'byte-call (length (cdr (cdr form)))))
+    (byte-compile-log-warning "`funcall' called with no arguments" nil :error)
+    (byte-compile-form '(signal 'wrong-number-of-arguments '(funcall 0)))))
 
 \f
 ;; let binding


> Andreas.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

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

> +        (mapc 'byte-compile-form (cdr form))

Use #' instead of ' please. ☺

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

bug#20241: 25.0.50; `setq' with only one argument

[Alan Mackenzie]

Evening, Artur.

On Thu, Nov 26, 2015 at 04:38:37PM +0000, Artur Malabarba wrote:
> > +        (mapc 'byte-compile-form (cdr form))

> Use #' instead of ' please. ☺

:-).  That wasn't my bit of code, I just had to reindent it.  It dates
from a time before using #' became fashionable.  Maybe there's something
to be said for replacing all such uses (or which there are 15) in the
file.

On a more serious note, given how there's potential for `funcall' to
segfault (without my latest proposed fix), perhaps I should go through
the entire byte compiler checking for similar occurrences.

On another note, I've managed to byte compile elpa, and there are indeed
three occurrences of bad `setq's, like you said.  There were
byte-compiler errors (which aborted the compilation) in packages elcs,
and rudel-mode, so there just might be more in there.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#20241: 25.0.50; `setq' with only one argument

[Artur Malabarba]

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

> On Thu, Nov 26, 2015 at 04:38:37PM +0000, Artur Malabarba wrote:
> > > +        (mapc 'byte-compile-form (cdr form))
>
> > Use #' instead of ' please. ☺
>
> :-).  That wasn't my bit of code,

I know. I just figured that since you changed the indentation, and the line
above this, and the line below this. Might as well take this chance to
sneak in this small improvement. :-)

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

bug#20241: 25.0.50; `setq' with only one argument

[Nicolas Richard]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

>> We won't see any problem in our own sources, because all the occurrences
>> here have been expunged.  Any problems will be with external libraries
>> (or possibly in *ELPA) which people expect just to download, compile and
>> run.  I think that was what Artur is worried about.
>
> Since we're going with this, I quickly grepped 4 occurrences on
> elpa/packages (didn't try the external branches). We should probably
> fix those too:
>
> ./hydra/hydra-test.el:1312:              (setq current-prefix-arg)
> ./tiny/tiny.el:364:                             (setq expect-fun)
> ./gnorb/gnorb-registry.el:294:    (setq id )
> ./context-coloring/benchmark/fixtures/subr.el:1448:     ,@(mapcar
> (lambda (binder) `(setq ,@binder)) binders)

I just looked at this one, and the docstring says :

    Each element of BINDERS is a list (SYMBOL VALUEFORM) which binds
    [...]

so IIUC it is correct.

-- 
Nico.