[bug #59956] Recipes inside conditionals can break the parser

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
URL:
  <https://savannah.gnu.org/bugs/?59956>

                 Summary: Recipes inside conditionals can break the parser
                 Project: make
            Submitted by: None
            Submitted on: Wed 27 Jan 2021 08:29:00 PM UTC
                Severity: 3 - Normal
              Item Group: Bug
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
       Component Version: 4.3
        Operating System: Any
           Fixed Release: None
           Triage Status: None

    _______________________________________________________

Details:

I stumbled across a strange behavior and it's scary how trivial it appears. I
tested this with make 4.1, 4.2 and 4.3 on native Ubuntu 20.04, WSL-Ubuntu
18.04 and MSYS2.
Consider the following example:


ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif


The *else* recipe at line 5 is treated as part of the *ifeq (1,0)* conditional
thus making the next recipe line fail because there is no target before it:


Makefile:6: *** recipe commences before first target.  Stop.
 

_I know the recipe does not make any sense without .ONESHELL, but it should
not matter. Maybe the user has a shell with a valid "else" command._
 
 
Adding *.ONESHELL:* above _(below does not work)_ fixes the problem. However,
if there is any other statement between *.ONESHELL:* and the conditional, this
causes the same problem.
This can be fixed by adding an additional .ONESHELL: just before the
conditional:


.ONESHELL: # Adding this line fixes the parser
aaa = 1    # Adding this line breaks the parser again
.ONESHELL: # Adding this line fixes the parser again

ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

 
If you can add a character immediately after the *else*, this does not happen
anymore:


SHELL = python3
.ONESHELL:

aaa = 1

ifeq (1,0)

test:
        @if 'a' == 'b':
          print('true')
        else: # No space between "else" and ":"
          print('false')

endif


Leaving a whitespace between *else* and *:* _(which is valid in python)_
brings the error back.

I found that changing *.RECIPEPREFIX:* also fixes the problem.

Is this really a bug or just some hidden intended behavior?




    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #1, bug #59956 (project make):

There is one real bug here, but everything else is just an understandable
consequence of that same bug.

The real bug is that the "else" that is part of a recipe, is being interpreted
by make as an "else" match for the ifeq.

This is because while make is parsing the "not-taken" side of the
if-statement, it is not interpreting the content it sees.  Because it's not
interpreting that content, it doesn't realize that the "else" here is indented
by a TAB in the context of a rule and it assumes that the "else" is supposed
to be attached to the "ifeq".

Fixing this without some heuristic is basically impossible.  In order to allow
this to work, make would need to be tracking enough of the "not-taken" side of
the if-statement to know whether the "else" is in the context of a recipe or
not.  But, people often use "ifeq" to disable entire sections of a makefile,
some of which might not even be valid make syntax.  Also consider if the
.RECIPEPREFIX was modified _inside_ of the ifeq statement, to something
else...!!

We can "fix" it by adding a simplifying heuristic; perhaps something like:
"conditional statements are only recognized if they are not prefixed by the
recipe prefix in effect when the if-statement started".  That is not perfect
but it's probably good enough.

All the other items mentioned here are not really bugs; if you remove the
lines starting with *ifeq* through *else* (and the *endif* of course) and just
look at the resulting makefile, which is what make is seeing, it's pretty
obvious why you get the behavior you do in all cases.

Adding *.ONESHELL:* makes it "work" because the part of the if-statement after
the else is a interpreted as a recipe attached to *.ONESHELL:* which is not an
error although it is a no-op.  In other words this:


.ONESHELL:
ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif


is equivalent to writing this:


.ONESHELL:
                echo "false"
        fi


which is a valid makefile, because we allow any target including a special
target like *.ONESHELL:* to have a recipe; obviously the recipe is invalid
shell syntax but that doesn't matter since the recipe is never run.

When you add a variable assignment after the *.ONESHELL:* and before the
*ifeq*, now you see this:


.ONESHELL:
aaa = 1
                echo "false"
        fi


which is now clearly an invalid makefile.  And if you add another *.ONESHELL:*
you'll get this:


.ONESHELL:
aaa = 1
.ONESHELL:
                echo "false"
        fi


which is, again, just fine.

If you add a character after *else*, then it's no longer a valid *else*
command for make's *ifeq*, so it's ignored.  In make conditionals are grouped
by _word_ (whitespace separated) so *else:* is considered a single token which
does not equal *else*, it's not considered two tokens, an *else* and a *:*.

If you add a space after the *else* before more text, then it IS a valid
*else* command, and you'll get an error that there's extraneous text after the
*else*.

Finally, adding *.RECIPEPREFIX* doesn't really fix anything.  It only changes
the error you get.  If you write:


.ONESHELL:
.RECIPEPREFIX = >
ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif


then the makefile make sees is:


.ONESHELL:
.RECIPEPREFIX = >
                echo "false"
        fi


You won't get an error about recipe commencing before first target because the
*echo* line is _not a recipe_; it doesn't begin with the *.RECIPEPREFIX*
character.  Instead you'll get a _missing separator_ error because these lines
are not valid make directives.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #2, bug #59956 (project make):

It may seem trivial but in reality it's pretty hard to hit.  There is
essentially only one way it can happen (in valid make recipes), and that's by
using *.ONESHELL:*.

If you don't use *.ONESHELL:* then the only way it can happen is if you have a
program named *else* that you want to run, which no sane person would ever do,
but even if you did want to do that the shell will consider the *else* to be a
keyword; so you'd have to write *./else* or something and then make wouldn't
match it either.

If you do use *.ONESHELL:* then you need to have the entire recipe in a make
*if* block with *else* as its own token on the recipe line.  Using
*.ONESHELL:* is still not that common and putting entire rules inside an *if*
block is also not that common.  *ifeq* etc. are more commonly used around
variable assignments or maybe include files.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #3, bug #59956 (project make):

Thanks for the even more detailed explanation than on Stack Overflow :)
I didn't realize special targets like *.ONESHELL:* can also have a recipe.
 
As a clarification, when I mentioned *.RECIPEPREFIX:* I was also implying
actually using it in the recipe:


.ONESHELL:
.RECIPEPREFIX = >
ifeq (1,0)
test:
>@if [ "asd" == "123" ]; then
> echo "true"
>else
> echo "false"
>fi
endif


I guess *>else* is not seen as a valid token and that's why it works.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #4, bug #59956 (project make):

Any target can have a recipe, even special targets like *.PHONY:*, *.POSIX:*,
*.ONESHELL:*, etc.  Those recipes are ignored but they are syntactically
legal.  I guess there would be value in mentioning them if some sort of
"warning mode" were enabled.

Re *.RECIPEPREFIX*: I see; yes I should have tried it myself then I would have
realized what you meant.  Right, this bug is a weird confluence of the
Original Sin of makefiles (using TAB, which is just whitespace, as a recipe
prefix) and the choice in GNU make to use simple *else* as the keyword; other
make implementations use special keywords like *.else* or similar which are
less likely to conflict with "real rules".

If the recipe prefix is anything other than whitespace this bug cannot happen.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #5, bug #59956 (project make):

> We can "fix" it by adding a simplifying heuristic; perhaps something like:
"conditional statements are only recognized if they are not prefixed by the
recipe prefix in effect when the if-statement started".  That is not perfect
but it's probably good enough.

I think this heuristic as stated would be disastrous at my $dayjob and
probably many others. Almost everyone who works on makefiles here is a C
programmer by trade and they're accustomed to indenting by tabs so I see a lot
of:

if ...
<tab>if ...
<tab>else
<tab>endif
else
endif

I try to discourage this and have written up a style guide which includes
"tabs should be used only as a recipe prefix" but there are still many such
instances.



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #6, bug #59956 (project make):

What about adding another keyword, e.g. .else?

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #7, bug #59956 (project make):

Adding a new keyword doesn't help: it's not that we're trying to use a make
*else* clause but the *else* is being interpreted as something else, where
replacing it with *.else* would help clarify what is meant; the problem is
that we're trying to use a shell (in this situation) *else* clause, so we
can't change it to *.else*, but make is interpreting it as a make *else*
clause.

The only way new keywords could help would be something like: allowing all
make conditional statements (*ifeq*, *ifneq*, *ifdef*, *else*, *endif*) to be
prefixed by ".", _and_ whichever form is used by the if-statement would have
to be used throughout.  So if you used *.ifeq* instead of *ifeq* then you must
also use *.else* and *.endif* and *else* and *endif* would not be recognized
for that if-statement.  This is a lot of churn; I'm not convinced.

I'm not sure what to say about people using TAB to indent conditionals.
That's SO dangerous and SO error-prone!  All that has to happen is that the
makefile is modified so these exist in a recipe context and boom!  Your entire
makefile is broken.  And how easy is it to create a recipe context?  As I show
in my replies below, *very* easy; here's a simple example:


ifeq (1,1)
        X = true
else
        X = false
endif

all:;@echo $X


That prints *true* as you expect.  Now if I change this makefile just
slightly:


.PHONY: all

ifeq (1,1)
        X = true
else
        X = false
endif

all:;@echo $X


Now *X* is not set at all!!  How many hours of staring at this makefile will
it take to understand what the problem is here?!?!

It's very hard for me to justify preserving such dangerous behavior as a
"feature".

An alternative heuristic would be that the same indentation level as the
*ifeq* would be required for all the conditional statements but I suspect that
would lead to even more widespread backward-compatibility problems.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

Re: [bug #59956] Recipes inside conditionals can break the parser

Edward Welbourne-3
In reply to this post by Paul D. Smith
> Consider the following example:

> ifeq (1,0)
> test:
>         @if [ "asd" == "123" ]; then
>                 echo "true"
>         else
>                 echo "false"
>         fi
> endif

> The *else* recipe at line 5 is treated as part of the *ifeq (1,0)*
> conditional thus making the next recipe line fail because there is no
> target before it:

> Makefile:6: *** recipe commences before first target.  Stop.

... and this could all too easily go *unreported* if this conditional
target-and-rule appeared just after some other.

I expected it to lead to the resulting rule getting a syntax error, but
am now baffled to find, by experiment, that (with suitable blocks of
spaces replaced by tabs, of course, if my mailer has substituted spaces
for tabs, which it probably has)

.ONESHELL:

fluff:
        @echo fluff

ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

        echo puff

doesn't provoke an error; make fluff simply echoes fluff then puff,
without echoing false or complaining of a syntax error due to the fi.
Removing the tab before else does get me the expected syntax error, so I
think something more complicated is going on here than simply the else
being recognised as belonging to the ifeq.

Can anyone explain that ?

> _I know the recipe does not make any sense without .ONESHELL, but it
> should not matter. Maybe the user has a shell with a valid "else"
> command.

That has proven a fascinating topic, thanks for bringing it up.
I particularly liked Paul's example of how adding a .PHONY rule could
dramatically change the meaning of a Makefile that uses tabs to indent
things not intended as rules.  One more reason to *only* use tabs in
rules.

The error message isn't much help, even when you do get it, but once
you've worked out what the problem there is at least a straightforward
work-around for the case above; just (regardless of .ONESHELL being set)
backslash-escape relevant newlines (and add semicolons before them,
where needed),

ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then \
                echo "true"; \
        else \
                echo "false"; \
        fi
endif

so that make sees the rule all as one line and the else doesn't appear
(as far as make is concerned) as a line on its own.

        Eddy.

Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
In reply to this post by Paul D. Smith
Follow-up Comment #8, bug #59956 (project make):

i mean, the user would tell make through some option (a special target or even
presence of ".else" in the makefile) "this makefile uses .else, rather then
else". make then would not consider "else" a keyword.
The keyword does not have to be ".else". It could be e.g. "otherwise". With
"otherwise" there is no need to prefix all conditionals with a ".".


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Сообщение отправлено по Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #9, bug #59956 (project make):

What about mixing in the special *$* character to help with differentiation?
Or adding *[]* to denote *make* only syntax.


$[ifeq (1,0)]
    $[info Maybe this could work with leading tabs also]
$[else]
    $[info Or with whitespaces]
$[endif]


Being able to indent the conditionals would be a big plus.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #59956] Recipes inside conditionals can break the parser

Paul D. Smith
Follow-up Comment #10, bug #59956 (project make):

Using *otherwise* means that now if a recipe runs a program named *otherwise*,
it will start to fail whereas before it succeeded.

I'm really not jazzed about creating new esoteric syntax for this problem.
Anyway, adding new syntax doesn't really help IMO.  There are already ways to
work around this problem by modifying the makefile: we don't need to create
anything new for that.  The hope is to find a way to make the current syntax
work "as expected", at least in the large majority of cases, without requiring
people to rewrite makefiles.

> Being able to indent the conditionals would be a big plus.

You CAN indent conditionals.  You just CAN'T indent them with the
*.RECIPEPREFIX* (TAB by default).

My feeling is that this is entirely reasonable.  Any attempt to allow TAB for
generic indentation by making a distinction between a recipe context and not a
recipe context is doomed to failure and future pain, and this has been true
ever since *ifeq* was invented.

If you really need to use TAB for indentation then you should modify your
makefiles to use a different *.RECIPEPREFIX* value.  Anything else is madness,
or at least leads that way.

Anyway that's my $0.02.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?59956>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/