bugGNU roff - Bugs: bug #62776, [troff] want "style"...

 
 

bug #62776: [troff] want "style" warning category for self-deceptive inputs

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Sat 16 Jul 2022 03:56:33 PM UTC
   
 
Category:  Core Severity:  1 - Wish
Item Group:  Feature change Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 28 Apr 2024 11:43:59 PM UTC, comment #19: 

Another use case:

  • Warn when formatting-relevant escape sequences or text appear on an input line after "interruption", the output line continuation escape sequence `\c`.


Experimentally sticking this in revealed some issues with our own macro files and documents.


diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index f57a97225..8d5deb152 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -297,6 +297,14 @@ void environment::add_char(charinfo *ci)
 {
   int s;
   node *gc_np = 0 /* nullptr */;
+  if (line_interrupted) {
+  // XXX: Add this later once we've cleaned up our own macro packages.
+#if 0
+    warning(WARN_SYNTAX, "ignoring text input on interrupted line");
+    return; // XXX: should we?
+#endif
+    ;
+  }
   if (line_interrupted)
     ;
   // don't allow fields in dummy environments
@@ -492,8 +500,10 @@ void environment::space()

 void environment::space(hunits space_width, hunits sentence_space_width)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX, "ignoring space on interrupted line");
     return;
+  }
   if (has_current_field && padding_indicator_char == 0 /* nullptr */) {
     add_padding();
     return;
@@ -532,7 +542,7 @@ static node *do_underline_special(bool do_underline_spaces)
 bool environment::set_font(symbol nm)
 {
   if (line_interrupted) {
-    warning(WARN_FONT, "ignoring font selection on interrupted line");
+    warning(WARN_SYNTAX, "ignoring font selection on interrupted line");
     return true; // "no operation" is successful
   }
   if (nm == symbol("P") || nm.is_empty()) {
@@ -565,8 +575,10 @@ bool environment::set_font(symbol nm)
 bool environment::set_font(int n)
 {
-  if (line_interrupted)
-    return false;
+  if (line_interrupted) {
+    warning(WARN_SYNTAX, "ignoring font selection on interrupted line");
+    return true; // "no operation" is successful
+  }
   if (is_good_fontno(n)) {
     prev_fontno = fontno;
     fontno = n;
@@ -580,8 +592,15 @@ bool environment::set_font(int n)

 void environment::set_family(symbol fam)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+  // XXX: Add this later once we've cleaned up our own documents.
+  // groff_hdtbl.5.man:85 is the culprit here.
+#if 0
+    warning(WARN_SYNTAX,
+           "ignoring family selection on interrupted line");
+#endif
     return;
+  }
   if (fam.is_null() || fam.is_empty()) {
     int previous_mounting_position = prev_family->make_definite(fontno);
     assert(previous_mounting_position >= 0);
@@ -611,8 +630,11 @@ void environment::set_family(symbol fam)

 void environment::set_size(int n)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX,
+           "ignoring type size selection on interrupted line");
     return;
+  }
   if (n == 0 /* nullptr */) {
     font_size temp = prev_size;
     prev_size = size;
@@ -631,8 +653,11 @@ void environment::set_size(int n)

 void environment::set_char_height(int n)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX,
+           "ignoring font height selection on interrupted line");
     return;
+  }
   if (n == requested_size || n <= 0)
     char_height = 0;
   else
@@ -641,8 +666,11 @@ void environment::set_char_height(int n)

 void environment::set_char_slant(int n)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX,
+           "ignoring font slant selection on interrupted line");
     return;
+  }
   char_slant = n;
 }

@@ -668,16 +696,22 @@ color *environment::get_fill_color()

 void environment::set_glyph_color(color *c)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX,
+           "ignoring stroke color selection on interrupted line");
     return;
+  }
   curenv->prev_glyph_color = curenv->glyph_color;
   curenv->glyph_color = c;
 }

 void environment::set_fill_color(color *c)
 {
-  if (line_interrupted)
+  if (line_interrupted) {
+    warning(WARN_SYNTAX,
+           "ignoring fill color selection on interrupted line");
     return;
+  }
   curenv->prev_fill_color = curenv->fill_color;
   curenv->fill_color = c;
 }


G. Branden Robinson <gbranden>
Group administrator
Fri 29 Dec 2023 09:21:12 AM UTC, comment #18: 

Correction to comment #17.

The user wants `\e` or (if the escape character is not the default) `\[rs]` instead.

G. Branden Robinson <gbranden>
Group administrator
Fri 10 Nov 2023 06:57:31 AM UTC, comment #17: 

Following up on comment #4...

  • Warn when `\\` encountered in interpretation mode.  The user wants `\e` or (if the escape character is the default) `\[rs]` instead.


  • Warn when `\.` encountered in interpretation mode.
G. Branden Robinson <gbranden>
Group administrator
Mon 08 May 2023 12:11:07 AM UTC, comment #16: 

comment #15:

> I find this to be a deficiency in Texinfo's terminal rendering.


Yeah, I suspected little could be done about it.  Thanks for confirming.

Dave <barx>
Group Member
Fri 05 May 2023 05:23:46 PM UTC, comment #15: 


> (This is actually a little easier to parse in the source, as quoted, than in the plaintext rendering of that sentence:


> ''br' does nothing.


> )


Yes.  I find this to be a deficiency in Texinfo's terminal rendering.  It has a rigid sed of quoting conventions and if the syntax of the system being documented doesn't mesh nicely with it...sucks to be that system.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 May 2023 11:16:23 PM UTC, comment #14: 

comment #5:

> 'br
>
> It's not even clear what this should do


Heh, unless you, you know, read the manual, which states: "@samp{'br} does nothing."  (This is actually a little easier to parse in the source, as quoted, than in the plaintext rendering of that sentence:

''br' does nothing.

)  This information has been in the manual (though more verbosely worded) since commit e9b0d52d in 2000.  (It's a massive commit, but the string "'br" appears in it only once.)  Its behavior being well-defined and documented makes me think it doesn't warrant a warning after all.

Dave <barx>
Group Member
Mon 01 May 2023 11:15:05 PM UTC, comment #13: 


comment #12:

>
> comment #10:
> [...]
>
> N.B.  Your text lines are unnecessary long.
>
>   When deleting lines from output, (one) "sed -e '/<regex>/d' [...]" is
> simpler than (many) "grep -v ... || : ...


I still need the ||: to ignore the exit status of mandoc(1), since I don't
want to fail for messages that I later filter out.  I only want to fail,
if there's any remaining byte in stderr after my filtering takes place;
thus, the last grep ^ >&2.

Regarding the substitution of grep by sed, I don't think there's a clear
win: both lines take the same number of bytes ('//d' vs 'p-v'), and both
commands are equally well known.  Since grep is what I already have, I'll
keep it.

>
>   Or (simpler) use a file for the regular expressions, "sed -f <file>",
> where the file contains lines with


However, the idea of putting these patterns into a file is an interesting
thing; I could modify it without having to modify the Makefile (which
causes unnecessary rebuilds).  That would also reduce the number of
fork+exec.

>
> /<regex/d


Since POSIX grep(1) defines -v and -f, I'll do this with grep(1), and then
the files will be simpler, as they'll only have to contain the actual
patterns, with no 'd' command.

>
>
> >>
> > $(_LINT_man_mandoc): $(_MANDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/
> > $(info LINT (mandoc) $@)
> > ! ($(MANDOC) -man $(MANDOCFLAGS) $< 2>&1 \
> >    | $(GREP) -v 'STYLE: lower case character in document title:' \
> >    | $(GREP) -v 'UNSUPP: ignoring macro in table:' \
> >    | $(GREP) -v 'WARNING: cannot parse date, using it verbatim: TH (date)' \
> >    | $(GREP) -v 'WARNING: empty block: UR' \
> >    | $(GREP) -v 'WARNING: missing date, using "": TH' \
> >    | $(GREP) -v 'WARNING: undefined escape, printing literally: \\\\' \
> >    ||:; \
> > ) \
> > | $(GREP) '.' >&2
> > touch $@
> >
> > $(_LINT_mdoc_mandoc): $(_MANDIR)/%.lint-mdoc.mandoc.touch: $(MANDIR)/% | $$(@D)/
> > $(info LINT (mandoc) $@)
> > ! ($(MANDOC) -mdoc $(MANDOCFLAGS) $< 2>&1 \
> >    | $(GREP) -v 'STYLE: operating system explicitly specified: Os ' \
> >    | $(GREP) -v 'WARNING: cross reference to self: Xr ' \
> >    ||:; \
> > ) \
> > | $(GREP) '.' >&2
> > touch $@
> >
> >
> > grep(1) is your tool for disabling unwanted diagnostics.  ;)
>
>   sed(1) is a better tool.


No, grep is even better ;)
But thanks for the idea!

Alejandro Colomar <alx>
Mon 01 May 2023 05:56:09 PM UTC, comment #12: 


comment #10:
[...]

N.B.  Your text lines are unnecessary long.

  When deleting lines from output, (one) "sed -e '/<regex>/d' [...]" is
simpler than (many) "grep -v ... || : ...

  Or (simpler) use a file for the regular expressions, "sed -f <file>",
where the file contains lines with

/<regex/d


>>
> $(_LINT_man_mandoc): $(_MANDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/
> $(info LINT (mandoc) $@)
> ! ($(MANDOC) -man $(MANDOCFLAGS) $< 2>&1 \
>    | $(GREP) -v 'STYLE: lower case character in document title:' \
>    | $(GREP) -v 'UNSUPP: ignoring macro in table:' \
>    | $(GREP) -v 'WARNING: cannot parse date, using it verbatim: TH (date)' \
>    | $(GREP) -v 'WARNING: empty block: UR' \
>    | $(GREP) -v 'WARNING: missing date, using "": TH' \
>    | $(GREP) -v 'WARNING: undefined escape, printing literally: \\\\' \
>    ||:; \
> ) \
> | $(GREP) '.' >&2
> touch $@
>
> $(_LINT_mdoc_mandoc): $(_MANDIR)/%.lint-mdoc.mandoc.touch: $(MANDIR)/% | $$(@D)/
> $(info LINT (mandoc) $@)
> ! ($(MANDOC) -mdoc $(MANDOCFLAGS) $< 2>&1 \
>    | $(GREP) -v 'STYLE: operating system explicitly specified: Os ' \
>    | $(GREP) -v 'WARNING: cross reference to self: Xr ' \
>    ||:; \
> ) \
> | $(GREP) '.' >&2
> touch $@
>
>
> grep(1) is your tool for disabling unwanted diagnostics.  ;)


  sed(1) is a better tool.

Bjarni Ingi Gislason <bjarniig>
Wed 26 Apr 2023 01:31:35 PM UTC, comment #11: 

That exceeds the bounds of what I would deem "easy." :)

A groff warning category can be disabled from within the document source with the .warn request.  Your method not only takes control away from the document itself, it also realistically requires groff now be run from a script or a makefile: you might easily type a simple "groff myfile" on the command line every time you want to process it, but you certainly won't do that if a lengthy pipeline is involved, even a conceptually simple one.

Dave <barx>
Group Member
Wed 26 Apr 2023 01:17:06 PM UTC, comment #10: 


comment #9:

> comment #8 comment #8:]
> > Disabling a warning if you don't care is easy.
>
> I would love for that to be true of groff warnings.  Unfortunately, you can only enable and disable warnings by category, the existing categories range in scope from ridiculously narrow to overly broad, and there's not a scalable mechanism for adding more.  Realistically this proposed warning will end up in the same category as others you might care about even if you intentionally use ' when not strictly necessary.


It's actually true of warnings in any software (especially in software that produces a single line for each diagnostic).  As I said recently in the mailing list, here's what I do with mandoc(1), which doesn't have any native mechanisms for disabling warnings:



$(_LINT_man_mandoc): $(_MANDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/
$(info LINT (mandoc) $@)
! ($(MANDOC) -man $(MANDOCFLAGS) $< 2>&1 \
   | $(GREP) -v 'STYLE: lower case character in document title:' \
   | $(GREP) -v 'UNSUPP: ignoring macro in table:' \
   | $(GREP) -v 'WARNING: cannot parse date, using it verbatim: TH (date)' \
   | $(GREP) -v 'WARNING: empty block: UR' \
   | $(GREP) -v 'WARNING: missing date, using "": TH' \
   | $(GREP) -v 'WARNING: undefined escape, printing literally: \\\\' \
   ||:; \
) \
| $(GREP) '.' >&2
touch $@

$(_LINT_mdoc_mandoc): $(_MANDIR)/%.lint-mdoc.mandoc.touch: $(MANDIR)/% | $$(@D)/
$(info LINT (mandoc) $@)
! ($(MANDOC) -mdoc $(MANDOCFLAGS) $< 2>&1 \
   | $(GREP) -v 'STYLE: operating system explicitly specified: Os ' \
   | $(GREP) -v 'WARNING: cross reference to self: Xr ' \
   ||:; \
) \
| $(GREP) '.' >&2
touch $@


grep(1) is your tool for disabling unwanted diagnostics.  ;)

Alejandro Colomar <alx>
Wed 26 Apr 2023 01:11:25 PM UTC, comment #9: 

comment #8 comment #8:]

> Disabling a warning if you don't care is easy.


I would love for that to be true of groff warnings.  Unfortunately, you can only enable and disable warnings by category, the existing categories range in scope from ridiculously narrow to overly broad, and there's not a scalable mechanism for adding more.  Realistically this proposed warning will end up in the same category as others you might care about even if you intentionally use ' when not strictly necessary.

Dave <barx>
Group Member
Wed 26 Apr 2023 09:56:48 AM UTC, comment #8: 


comment #6:

> Also I think the technique you describe, which I have not seen in the wild, might be too clever by half.  Better to say in English in a comment, "This macro is intended to cause no breaks.", no?


I tend to prefer actual code than comments.  Comments can get out of date.  Code not (or well, it does, but you would notice more easily).

About the warning, I think I prefer having it.  Disabling a warning if you don't care is easy.  But if the warning is not there, you can't add it :)

Alejandro Colomar <alx>
Wed 26 Apr 2023 09:31:05 AM UTC, comment #7: 

comment #6:

> the technique you describe, which I have not seen in the wild,


I haven't either, just thinking out loud (or the virtual version thereof).

Dave <barx>
Group Member
Wed 26 Apr 2023 08:43:08 AM UTC, comment #6: 

comment #5:

> One may not be saying something meaningful to groff, but may be to a human reader.  For example, if a user-defined macro should not cause a break, and that macro invokes five requests, two of which cause a break, then as far as roff is concerned, only those two need the ' at the start of the line.  But as a courtesy to readers (including the future version of oneself), one might well write them all with a leading ' so that it's clear at a glance that the macro does not cause a break, without the reader having to have memorized, or having to look up, which requests break.


But if you enable the warning, you'll be told as much.  (Well, it'll be an inverse oracle.)

Also I think the technique you describe, which I have not seen in the wild, might be too clever by half.  Better to say in English in a comment, "This macro is intended to cause no breaks.", no?
 

> Semi-relatedly, a more warning-worthy situation is one that came up recently on the list, the construction:
>
> 'br
>
> It's not even clear what this should do (I think it turned out to be a no-op, at least in groff), so that's probably worth warning about.


Agreed.

G. Branden Robinson <gbranden>
Group administrator
Wed 26 Apr 2023 08:22:02 AM UTC, comment #5: 

comment #4:

> 2. Invoking a non-breaking request with the no-break control
> character; the user will think they're saying something
> meaningful, but they are not.


One may not be saying something meaningful to groff, but may be to a human reader.  For example, if a user-defined macro should not cause a break, and that macro invokes five requests, two of which cause a break, then as far as roff is concerned, only those two need the ' at the start of the line.  But as a courtesy to readers (including the future version of oneself), one might well write them all with a leading ' so that it's clear at a glance that the macro does not cause a break, without the reader having to have memorized, or having to look up, which requests break.

Semi-relatedly, a more warning-worthy situation is one that came up recently on the list, the construction:

'br

It's not even clear what this should do (I think it turned out to be a no-op, at least in groff), so that's probably worth warning about.

Dave <barx>
Group Member
Tue 25 Apr 2023 02:29:34 PM UTC, comment #4: 

I can't remember if I scribbled this memo elsewhere, but a "style" warning category could scoop this up along with two other weirdo input practices:

1. Leaving the last quoted argument in a macro call without a trailing quote; this is easily misread as an unquoted argument, and so appending a further quoted argument in maintenance will have unexpected consequences.

2. Invoking a non-breaking request with the no-break control character; the user will think they're saying something meaningful, but they are not.

G. Branden Robinson <gbranden>
Group administrator
Mon 01 Aug 2022 10:26:34 PM UTC, comment #3: 

Not an objection, just for your consideration.

We already have a method for enabling, disabling, and selecting diagnostic messages.
Please remember that organizing diagnostic messages is among those areas most prone to code sprawl and over-engineering,
hence a clean and uniform design is even more beneficial in that area than elsewhere.

Consequently, my gut feeling isn't enthusiastic about adding a second mechanism for controlling diagnostic messages.

Ingo Schwarze <schwarze>
Group Member
Tue 19 Jul 2022 02:49:00 PM UTC, comment #2: 


original submission:

> The question is how to expose it.  I'm leaning toward a user-writable register at this point.


That would work for me.  Just add a small sample demonstrating use in this ticket, and I'll take it.

Alejandro Colomar <alx>
Sat 16 Jul 2022 03:57:30 PM UTC, comment #1: 

Alex, I've added you to the CC list for this ticket.

G. Branden Robinson <gbranden>
Group administrator
Sat 16 Jul 2022 03:56:33 PM UTC, original submission:  

Alejandro Colomar requested this, and the fundamental thing is easily done.

https://lists.gnu.org/archive/html/groff/2022-06/msg00052.html

The question is how to expose it.  I'm leaning toward a user-writable register at this point.

G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Carbon-Copy List
  • -email is unavailable- added by bjarniig (Posted a comment)
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by alx (Posted a comment)
  • -email is unavailable- added by barx
  • -email is unavailable- added by gbranden (original requester)
  • -email is unavailable- added by gbranden (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-01-01 gbranden Dependencies- bugs #65099 is dependent
    2023-08-10 gbranden Dependencies- bugs #64275 is dependent
    2023-04-25 gbranden Summary[troff] add optional diagnostic for sentences ending mid-input line [troff] want "style" warning category for self-deceptive inputs
    2022-07-17 barx Carbon-Copy- Added barx
    2022-07-16 gbranden Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-b921.
    Corresponding source code