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

[Drew Adams <drew.adams@oracle.com>]

> > 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.