bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

emacs -Q
 
Put point at position 3061 in file thingatpt.el (i.e., just before the
`a' of `condition-case').
 
M-: (bounds-of-thing-at-point 'comment)
returns (3061 . 3061), which represents an empty thing, "".
 
This happens at most positions, probably all positions outside a comment.
 
The code should handle this kind of case correctly (in other respects it
seems to work OK for comments).
 
The problem is that there is no `beginning-op' or `end-op' and all of
the calls to `forward-thing' (with 1 and -1) do not move point at all,
and return nil.  So the code falls through to the end:
 
(let ((end (point))
      (real-beg
       (progn
        (funcall
         (or (get thing 'beginning-op)
             (lambda () (forward-thing thing -1))))
        (point))))
  ;; real-beg = end = (point).  Result is (cons 3061 . 3061).
  (if (and real-beg end (<= real-beg orig) (<= orig end))
      (cons real-beg end)))

`bounds-of-thing-at-point' should foresee such a case (that `forward-THING' does
nothing).  IOW, it should of course be fixed for this problem.

But it sounds like `forward-comment' should also be fixed so that it acts like
other `forward-THING' functions, and not just be a no-op when outside a comment.
 
In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2011-05-10 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.5) --no-opt --cflags
-Ic:/build/include'
 

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> The problem is that there is no `beginning-op' or `end-op' and all of
> the calls to `forward-thing' (with 1 and -1) do not move point at all,
> and return nil.  So the code falls through to the end:
>  
> (let ((end (point))
>       (real-beg
>        (progn
>         (funcall
>          (or (get thing 'beginning-op)
>              (lambda () (forward-thing thing -1))))
>         (point))))
>   ;; real-beg = end = (point).  Result is (cons 3061 . 3061).
>   (if (and real-beg end (<= real-beg orig) (<= orig end))
>       (cons real-beg end)))
> 
> `bounds-of-thing-at-point' should foresee such a case (that 
> `forward-THING' does nothing).  IOW, it should of course be
>  fixed for this problem.

This should be all that's needed, if it wasn't obvious:

- (if (and beg real-end (<= beg orig) (<= orig real-end))
-     (cons beg real-end))

+ (and beg real-end (<= beg orig) (<= orig real-end)
+      (/= beg read-end)
+      (cons beg real-end))

and

- (if (and real-beg end (<= real-beg orig) (<= orig end))
-     (cons real-beg end)))

+ (and real-beg end (<= real-beg orig) (<= orig end)
+      (/= real-beg end)
+      (cons real-beg end))

(Dunno why some people insist on using `(if (and...) singleton)'.  It gets in
the way of readability and just represents extra noise.  Binary `if' is
generally an impediment to readability and communicating intention.)

> But it sounds like `forward-comment' should also be fixed so 
> that it acts like other `forward-THING' functions, and not
> just be a no-op when outside a comment.

And it seems that `forward-comment' is otherwise buggy, in that it moves only
over whitespace when point is within a comment.

E.g.:
;; Some comment with some     whitespace somewhere.

Put point before any of the whitespace and (forward-comment 1) moves to the end
of the whitespace.  Put point after any of the whitespace and (forward-comment
-1) moves to the beginning of that whitespace.  And in these cases swapping 1
and -1 produces a no-op.

The only time `forward-comment' actually moves over a whole (Lisp) comment is
when point is before `;'.  And even then, if point is between the two `;' above,
then (forward-comment -1) is a no-op.

`forward-comment' should behave the way other `forward-THING' functions behave.
It should do what we expect of a `forward-*' function.

Yes, I have read the `forward-comment' doc string.  If it really needs to behave
this way then it should have a different name.  That is, after fixing the bugged
behavior, if we still need a function that does what `forward-comment' does now,
then it needs a different name.

No, it's not too late to change, even if renaming would be bothersome.  The way
it is now breaks `thing-at-point' functions, and they should work regardless of
the `forward-*' functions.  Thing-at-point depends on this naming convention.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Kevin Rodgers]

On 5/12/11 9:00 PM, Drew Adams wrote:

> - (if (and beg real-end (<= beg orig) (<= orig real-end))
> -     (cons beg real-end))
>
> + (and beg real-end (<= beg orig) (<= orig real-end)
> +      (/= beg read-end)
> +      (cons beg real-end))
>
> and
>
> - (if (and real-beg end (<= real-beg orig) (<= orig end))
> -     (cons real-beg end)))
>
> + (and real-beg end (<= real-beg orig) (<= orig end)
> +      (/= real-beg end)
> +      (cons real-beg end))
>
> (Dunno why some people insist on using `(if (and...) singleton)'.  It gets in
> the way of readability and just represents extra noise.  Binary `if' is
> generally an impediment to readability and communicating intention.)

Readability is in the eye of the beholder, intention is in the mind of the
author.

Personally, I think (if (and...) result) communicates the intent more clearly
than (and ... result)

-- 
Kevin Rodgers
Denver, Colorado, USA

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Stefan Monnier]

> (Dunno why some people insist on using `(if (and...) singleton)'.

Probably because those people find it better communicates the intent.

> And it seems that `forward-comment' is otherwise buggy, in that it moves only
> over whitespace when point is within a comment.

Not only over whitespace: also over a nested comment.
What would you want it to do instead?  Jump to the end of the comment?
Kind of like an `up-comment'?

The forward-<thingy> functions all share the following property AFAIK:
when called with a positive argument with point *before* some
<thingies>, they will skip over that many <thingies> and when called
with a negative argument with point *after* some <thingies> they will
skip over that many <thingies>.

E.g. just like forward-comment, forward-sexp from within a sexp will not
skip to the end of that sexp.  And just like forward-sexp,
forward-comment called in front of a nested comment will jump over that
nested comment.

All calls with point elsewhere than before/after a <thingy> behave in
somewhat arbitrary ways which mostly depend on how the function is
implemented and what kind of structure <thingies> have (e.g. can they
nest?  Can we easily tell when we're in the middle of a <thingy>?).


        Stefan

bug#8670: OT: bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> > (Dunno why some people insist on using `(if (and...) 
> > singleton)'.  It gets in the way of readability and just
> > represents extra noise.  Binary `if' is generally an
> > impediment to readability and communicating intention.)
> 
> Readability is in the eye of the beholder, intention is in 
> the mind of the author.

Which is why I said "readability AND communicating intention".  And I said
"COMMUNICATING intention", not just "intention".  Communicating involves both
the writer and the reader.  If the writer's intent is to be communicated well
then readers need to be able to grasp it easily by reading.

> Personally, I think (if (and...) result) communicates the 
> intent more clearly than (and ... result)

Really?  What's the intent?  The result of evaluating
(and A B C D E) is pretty clear: nil or E.

You write that which way?  What does adding `if' do for you?
You can add `if' quite a bit, but what does it help?

(and A B C D E)
(if (and A B C D) E)           - clearer?
(if (if (and A B C) D) E)      - even clearer?
(if (if (if (and A B) C) D) E) - yet clearer?
(if (if (if (if A B) C) D) E)  - clearest?

To each his own...

The problem with binary `if' is that it requires more careful parsing, to
distinguish a single sexp from two (2 sexps from 3).  It can be pretty easy to
mistake a binary for a ternary `if', or vice versa, depending on the actual
argument sexps.

But if you know that a writer systematically uses:

 (a) `when' and `unless' to indicate that the result is
     unimportant/unused (only side effects matter),
 (b) `if' only as ternary, never binary,
 (c) `and' and `or' when args are to be eval'd in order
     and the result is significant/used

then it is very quick to follow the code's meaning and author's intent.  Coming
across a binary `if' in this context then raises a red flag.  Of course, when
debugging a section of code that is problematic you must always double-check
that the writer actually respected the convention, but otherwise it's a breeze.

Is this a widespread convention?  Yes and no.  Many writers of Common Lisp
follow it; some (many?) do not.  It helps when you pretty much know that the
writer follows it (e.g. when I read my own code).  All bets are off if no
convention is followed wrt these functions.

Personally, I consider use of `if' when the result is not important, and use of
`when' or `unless' when the result matters, to be perverse.  The other parts of
the convention are less important/useful, to me.

If you want to super-if-ify the Emacs source code, as above, feel free.  Reduce
all uses of `and' to binary `and' if you want, or eliminate use of `and'
altogether.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> > And it seems that `forward-comment' is otherwise buggy, in 
> > that it moves only over whitespace when point is within a comment.
> 
> Not only over whitespace: also over a nested comment.
> What would you want it to do instead?  Jump to the end of the comment?
> Kind of like an `up-comment'?
> 
> The forward-<thingy> functions all share the following property AFAIK:
> when called with a positive argument with point *before* some
> <thingies>, they will skip over that many <thingies> and when called
> with a negative argument with point *after* some <thingies> they will
> skip over that many <thingies>.
> 
> E.g. just like forward-comment, forward-sexp from within a 
> sexp will not skip to the end of that sexp.  And just like forward-sexp,
> forward-comment called in front of a nested comment will jump 
> over that nested comment.
> 
> All calls with point elsewhere than before/after a <thingy> behave in
> somewhat arbitrary ways which mostly depend on how the function is
> implemented and what kind of structure <thingies> have (e.g. can they
> nest?  Can we easily tell when we're in the middle of a <thingy>?).

You're right, and I was aware of that.  While dealing with the
`bounds-of-thing-at-point' bug I mistakenly got the impression that
`forward-char' was also misbehaving.

Let's please fix `bounds-of-thing-at-point', though.  The fix is trivial.  It's
OK for `b-o-t-a-p' to return a purely whitespace thing (`whitespace' is even a
proper thing), but it's not OK for it to return an empty thing (""), IMO.

--

BTW (do I need to create a separate bug report for this?), I think
`forward-whitespace' is incorrect: \n should be \n+, like this:

(defun forward-whitespace (arg)
  (interactive "p")
  (if (natnump arg)
      (re-search-forward "[ \t]+\\|\n+" nil 'move arg)
    (while (< arg 0)
      (if (re-search-backward "[ \t]+\\|\n+" nil 'move)
	  (or (eq (char-after (match-beginning 0)) 10)
	      (skip-chars-backward " \t")))
      (setq arg (1+ arg)))))

Try `(bounds-of-thing-at-point 'whitespace)' at various places, in particular at
the end of a line followed by an empty line.  The current definition does not
consider contiguous whitespace from newlines to be part of the same whitespace
thing - it treats \n as separating whitespace things.

Dunno whether that was the intention (why?).  Without any indication of the
reason/intention (no doc string), I'd say that any sequence of contiguous
whitespace, including newlines, should form a single whitespace thing.

Can we also please add doc strings to functions such as `forward-whitespace'?
They are missing, generally, in thingatpt.el.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> I think `forward-whitespace' is incorrect: \n should be \n+, like this:
> 
> (defun forward-whitespace (arg)
>   (interactive "p")
>   (if (natnump arg)
>       (re-search-forward "[ \t]+\\|\n+" nil 'move arg)
>     (while (< arg 0)
>       (if (re-search-backward "[ \t]+\\|\n+" nil 'move)
> 	  (or (eq (char-after (match-beginning 0)) 10)
> 	      (skip-chars-backward " \t")))
>       (setq arg (1+ arg)))))

Note too that `forward-whitespace' is not currently used anywhere in the Emacs
source files.  Can we please make this change, so that it always moves over all
contiguous whitespace and so takes point up to a non-whitespace char (or eob)?

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Stefan Monnier]

Installed a patch along the lines of what you suggested.

>> I think `forward-whitespace' is incorrect: \n should be \n+, like this:
>> 
>> (defun forward-whitespace (arg)
>> (interactive "p")
>> (if (natnump arg)
>> (re-search-forward "[ \t]+\\|\n+" nil 'move arg)
>> (while (< arg 0)
>> (if (re-search-backward "[ \t]+\\|\n+" nil 'move)
>> (or (eq (char-after (match-beginning 0)) 10)
>> (skip-chars-backward " \t")))
>> (setq arg (1+ arg)))))

The current behavior is clearly intentional (the (skip-chars-backward "
\t") shows that the function wants to treat newlines as
not-just-whitespace).  So I'd rather not change it.


        Stefan

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> Installed a patch along the lines of what you suggested.

No, I don't think you did.  I just downloaded the latest thingatpt.el.  There is
no fix AFAICT.

There are still the two problems reported for `bounds-of-thing-at-point' in this
bug report: (1) it can return a cons with equal car and cdr (representing an
empty thing), (2) it can return a cons representing only whitespace, instead of
returning nil when there is no comment at point.


1. You did not filter out the case where (= beg real-end) or
(= real-beg end), so `bounds-of-thing-at-point' can still return a cons with
equal car and cdr.

You need to add these conditions to the tests:

(and beg real-end (<= beg orig) (<= orig real-end)
                  (/= beg real-end) ; <===== NEEDED
                  (cons beg real-end))
...

(and real-beg end (<= real-beg orig) (<= orig end)
                  (/= real-beg end) ; <===== NEEDED
                  (cons real-beg end))


2. It is still the case that (bounds-of-thing-at-point 'comment) can return a
cons representing only whitespace, when called outside a comment.  Put point
here, for instance:

(defun bounds-of-thing-at-point...
       ^

It should always return nil, never return a cons, when there is no comment at
point.  This is a `b-o-t-a-p' bug (not a `forward-comment' bug).  In turn, this
bug causes (thing-at-point 'comment) to return " " at such a location.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> All calls with point elsewhere than before/after a <thingy> behave in
> somewhat arbitrary ways which mostly depend on how the function is
> implemented and what kind of structure <thingies> have (e.g. can they
> nest?  Can we easily tell when we're in the middle of a <thingy>?).

That's clearly no good for thing-at-point, in general.  `thing-at-point' uses
`bounds-of-thing-at-point', and that uses `forward-THING' in both directions (1,
-1).

`forward-comment' moves point even when there is no comment at point (before or
after).  And that causes (thing-at-point 'comment) to return a non-comment
string of text (e.g. only whitespace), instead of nil, when point is not on a
comment.

`thing-at-point' should always return nil or a comment, when THING is `comment'.

It is up to the `forward-THING' functions to each DTRT, or else they break
`thing-at-point'.  The alternative is to prevent `b-o-t-a-p' from relying on
`forward-comment'.

IOW, either (a) we need to fix `forward-comment' to DTRT in this case, (b) we
need to add a function `bounds-of-comment-at-point' and then (put 'comment
'bounds-of-thing-at-point 'bounds-of-comment-at-point), or (c) we need to define
adequate `beginning-op' and `end-op' functions for type `comment'.

(b) and (c) are the only ways that `bounds-of-thing-at-point' will avoid
(mis)using a `forward-THING' that does not DTRT for thing-at-point.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Stefan Monnier]

> No, I don't think you did.  I just downloaded the latest thingatpt.el.
> There is no fix AFAICT.

From which branch?

> You need to add these conditions to the tests:

> (and beg real-end (<= beg orig) (<= orig real-end)
>                   (/= beg real-end) ; <===== NEEDED
>                   (cons beg real-end))
> ...

> (and real-beg end (<= real-beg orig) (<= orig end)
>                   (/= real-beg end) ; <===== NEEDED
>                   (cons real-beg end))

The code you show here is not on the trunk any more (e.g. I removed the
non-null test of real-beg and end because they were redundant).


        Stefan

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Stefan Monnier]

> That's clearly no good for thing-at-point, in general.

Guess where I think the problem lies?


        Stefan

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> > That's clearly no good for thing-at-point, in general.
> 
> Guess where I think the problem lies?

Good, so you recognize there is a problem.  The bug is that `thing-at-point' and
`bounds-of-thing-at-point' can return non-nil for type `comment' when there is
no comment at point.  The former returns a purely whitespace string; the latter
returns its bounds as a cons.

You seem to suggest that the problem, and the proper fix, involves thingatpt.el,
not `forward-comment'.  That sounds fine to me.

One way or the other, however, this needs to be fixed.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> > No, I don't think you did.  I just downloaded the latest 
> > thingatpt.el.  There is no fix AFAICT.
> 
> From which branch?

From here:
http://repo.or.cz/w/emacs.git/tree/HEAD:/lisp

In the past that has always given the trunk AFAIK.  And in the past it has been
the _only_ HTTP access that has been reliable (works).  But apparently it is no
longer kept up-to-date.

I just now tried
http://bazaar.launchpad.net/~vcs-imports/emacs/trunk/files/head%3A/lisp/
and got this, which is typically the case:

 Please try again
 Sorry, there was a problem connecting to the Launchpad server. 

 Try reloading this page in a minute or two. If the problem persists,
 let us know in the #launchpad IRC channel on Freenode. 

 Thanks for your patience.

It used to be easy to access Emacs source code via HTTP (back when you used CVS,
IIRC).  Now it is difficult, even if occasionally still possible.  After
repeatedly trying that second URL, I was finally able to get your fixed file.

The first part of this bug report does seem to be fixed: no empty thing
returned.

The second part is not fixed.  `bounds-of-thing-at-point' can still return a
cons when it should return nil.  It can easily return a cons that represents a
purely whitespace, non-comment for thing-type `comment'.  This in turn means
that `thing-at-point' returns a whitespace, non-comment for thing-type
`comment'.

This is a bug in `thingatpt.el'.  It is either a bug in
`bounds-of-thing-at-point', if we expect that general function to DTRT in this
case, or it is a bug in that there is a missing function
`bounds-of-comment-at-point' and consequent (put 'comment
'bounds-of-thing-at-point 'bounds-of-comment-at-point).  Or some such fix.

I already gave the recipe to reproduce the bug.  I can repeat it if necessary.

`b-o-t-a-p' depends for its generic effect on functions such as `forward-THING'.
Sometimes that is enough to make `b-o-t-a-p' DTRT; sometimes it is not enough.
When it is not, as in this case, other measures need to be taken, such as
defining a THING-specific `bounds-of-THING-at-point' function and putting that
on the THING symbol as property `bounds-of-thing-at-point'.

One way or the other, and there are several alternatives, this needs to be
fixed.  It is a bug for `b-o-t-a-p' to return non-nil when there is no comment
at point, and thus for `thing-at-point' to return a non-nil, non-comment value.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Date: Sat, 21 May 2011 07:52:58 -0700
> Cc: 8667-close@debbugs.gnu.org
> 
> > From which branch?
> 
> >From here:
> http://repo.or.cz/w/emacs.git/tree/HEAD:/lisp
> 
> In the past that has always given the trunk AFAIK.  And in the past it has been
> the _only_ HTTP access that has been reliable (works).  But apparently it is no
> longer kept up-to-date.
> 
> I just now tried
> http://bazaar.launchpad.net/~vcs-imports/emacs/trunk/files/head%3A/lisp/
> and got this, which is typically the case:
> 
>  Please try again
>  Sorry, there was a problem connecting to the Launchpad server. 

That URL is obsolete.  This one is correct and working:

   http://bzr.savannah.gnu.org/lh/emacs/trunk/files

I'm quite sure I posted a message about that to emacs-devel... let me
see... yep, on Apr 10, Re: Development sources can be viewed in bzr
with a Web browser.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> That URL is obsolete.  This one is correct and working:
>    http://bzr.savannah.gnu.org/lh/emacs/trunk/files
> I'm quite sure I posted a message about that to emacs-devel... let me
> see... yep, on Apr 10, Re: Development sources can be viewed in bzr
> with a Web browser.

OK, thanks; must have missed that mail.  FYI, the "obsolete" one still works,
apparently (when it works ;-)).

BTW - Why not have a more or less permanent URL that you guys can redirect to
wherever, as needed?  Why so many changes to the user-end URL?  No need to
answer - just food for thought.  Ideally, users should not need to juggle
bookmarks to keep up with dev changes.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <8667@debbugs.gnu.org>
> Date: Sat, 21 May 2011 08:15:06 -0700
> 
> BTW - Why not have a more or less permanent URL that you guys can redirect to
> wherever, as needed?  Why so many changes to the user-end URL?

I have no idea why it changed, but the one I mentioned should stay
put.

bug#8667: 24.0.50; `bounds-of-thing-at-point' returns (N . N) for `comment'

[Drew Adams]

> > BTW - Why not have a more or less permanent URL that you 
> > guys can redirect to wherever, as needed?  Why so many
> > changes to the user-end URL?
> 
> I have no idea why it changed, but the one I mentioned
> should stay put.

Good to hear.  Thanks again.