http://d.puremagic.com/issues/show_bug.cgi?id=4375

           Summary: Require explicit braces when 'else' is ambiguous
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: bearophile_hugs@eml.cc


--- Comment #0 from bearophile_hugs@eml.cc 2010-06-23 05:12:01 PDT ---
This program is valid code both in D2 and C:

void foo() {}
void bar() {}
int main() {
    int a = 1;
    int b = 1;
    if (a)
        if (b)
            foo();
    else bar();
    return 0;
}


The problem is that the indentations do not reflect the program semantics, this can cause bugs (this bug source is missing in Python). The problem can be avoided adding explicit braces to that code.

Compiled with GCC V.4.5.0 it prints:
...>gcc -Wall test.c -o test
test.c: In function 'main':
test.c:6:8: warning: suggest explicit braces to avoid ambiguous 'else'


The warning used here by -Wall is just -Wparentheses:
...>gcc -Wparentheses test.c -o test
test.c: In function 'main':
test.c:6:8: warning: suggest explicit braces to avoid ambiguous 'else'


So in D2 I propose to turn that code into an actual syntax error, similar to
using the warnings-as-errors option of GCC:
...>gcc -Wall -Werror test.c -o test
cc1.exe: warnings being treated as errors
test.c: In function 'main':
test.c:6:8: error: suggest explicit braces to avoid ambiguous 'else'


Note: this enhancement request doesn't mean that explicit braces are always required in "if". They are required only in situations where the code can be ambiguous for a human programmer. Well written D code does not raise this error.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375


Jonathan M Davis <jmdavisProg@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmail.com


--- Comment #1 from Jonathan M Davis <jmdavisProg@gmail.com> 2010-06-23 09:21:28 PDT ---
Except that then you have to make the compiler understand indentation. I don't believe that it cares at all about indentation at this point. Whitespace is whitespace. It doesn't matter how much of it that there is; it just matters if there is any.

Requiring it to be able to understand indentation well enough to alert the programmer as to where they used it incorrectly or to complain that the programmer has used it in a manner which could be ambiguous to a human reader would likely complicate things quite a bit for the compiler.

The current situation is totally unambiguous. It's just that it doesn't stop the programmer from being stupid with how much indentation they use and thereby throwing off other programmers who don't read their code carefully (or they themselves when they read it later).

Also, it could be rather nasty to code generators, since then they'd have to worry a lot more about making code human-readable - which while potentially nice probably isn't necessary in many cases.

I believe that the usual solution if a programmer is worried about this sort of thing is just to use a coding standard that always requires braces for the bodies of if-else statements, loops, etc.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #2 from bearophile_hugs@eml.cc 2010-06-23 10:23:48 PDT ---
Answer to Comment 1:

>I don't believe that it cares at all about indentation at this point.<

Right, with this code GCC generates the same warning/error:

void foo() {}
void bar() {}
int main() {
    int a = 1;
    int b = 1;
    if (a)
        if (b)
            foo();
        else
            bar();
    return 0;
}


In the original code I have uses a wrong indentation to better show a possible semantic bug that can happen.


> Requiring it to be able to understand indentation well enough to alert the programmer as to where they used it incorrectly or to complain that the programmer has used it in a manner which could be ambiguous to a human reader would likely complicate things quite a bit for the compiler.

I think some lint do this, but I was not asking for D to do this. Sorry if I have written misleading things.


>The current situation is totally unambiguous.<

The current situation is totally unambiguous for the compiler. But unfortunately in some situations it is not unambiguous for some programmers. The purpose of that GCC warning it to help programmers, it's not for the compiler.


> Also, it could be rather nasty to code generators, since then they'd have to worry a lot more about making code human-readable - which while potentially nice probably isn't necessary in many cases.

In the original Description I have expressed my desires in a bad way. My enhancement request doesn't regard indentation at all, so there are no problems with code generators (that probably always use braces).


> I believe that the usual solution if a programmer is worried about this sort of thing is just to use a coding standard that always requires braces for the bodies of if-else statements, loops, etc.

If you always use braces, then you will never see this error I am asking for, so you can ignore this enhancement request, it's not for you. A sufficiently careful programmer can avoid most bugs. But programmers are not always fully careful, so it's good if the compiler guards for _easy to catch_ possible sources of bugs. D syntax differs from C syntax in some small parts also to catch common bugs.

I don't know if D devs/programmers agree with GCC devs that this is an important enough possible source of bugs.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #3 from Jonathan M Davis <jmdavisProg@gmail.com> 2010-06-23 11:41:41 PDT ---
Ah, okay. I guess that that could be detected without regards to indentation. As such, it probably wouldn't require huge changes to implement, and I don't see a particular problem with it (though it does strike me as more of a warning than an error and Walter does seem to be rather against warnings in general - but that's a whole other debate).

Personally, I'd never run into the problem because while I often write single statement body if-else statements and loops without braces, I always use braces if either the condition or the body is more than one line long (even if it's a single statement).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #4 from bearophile_hugs@eml.cc 2010-09-20 17:41:37 PDT ---
It seems that not just GCC flags this a possible error, in the Java audit rules in some Google Software this problem is named "Dangling Else":

http://code.google.com/intl/en-EN/webtoolkit/tools/codepro/doc/features/audit/audit_rules_com.instantiations.assist.eclipse.auditGroup.possibleErrors.html#com.instantiations.assist.eclipse.audit.danglingElse


It says:
This audit rule finds places in the code where else clauses are not preceded by
a block because these can lead to dangling else errors.

Example
    if (a > 0)
        if (a > 100)
            b = a - 100;
    else
        b = -a;

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #5 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-09-20 18:26:31 PDT ---
"The dangling else problem" is essentially the official name of the problem of knowing which if to put the last else with if you have a series of if statements with an else statement at the end and no braces to indicate where it should go. The canonical way to solve it is to put the else with the last if. It's a classic problem in programming language grammars.

Of course, the problem here is that the spacing that the programmer used seems to indicate that he did not intend the else to go with the last if, but the grammar is quite unambiguous on the matter. Still, it is likely an error on the programmer's part.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375


Stewart Gordon <smjg@iname.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |smjg@iname.com


--- Comment #6 from Stewart Gordon <smjg@iname.com> 2010-09-23 05:59:44 PDT ---
(In reply to comment #1)
> Except that then you have to make the compiler understand indentation.

???  Defining the form

    if ( IfCondition ) if ( IfCondition ) ThenStatement else ElseStatement

to be an error, rather than to be parsed in one of the two possible ways, doesn't require the compiler to understand indentation at all.

> Requiring it to be able to understand indentation well enough to alert the programmer as to where they used it incorrectly or to complain that the programmer has used it in a manner which could be ambiguous to a human reader would likely complicate things quite a bit for the compiler.

But if you design the language to be whitespace- and indent-sensitive in the first place, then it's easy.  That's how Python has managed it.  You need a different type of parser to read the language, but it isn't difficult.

On the other hand, you can't add indent-sensitivity to a free-form language, unless you discard the free-form aspect and require line breaks in set places.  But then it would be silly to require curly brackets and semicolons, which is why Python doesn't.  This would be a huge breaking change for D were it to be implemented.

> Also, it could be rather nasty to code generators, since then they'd have to worry a lot more about making code human-readable - which while potentially nice probably isn't necessary in many cases.

ISTM code generators would likely put the {} in, because it's easier to program it to always put them in than to detect whether they're necessary.

> I believe that the usual solution if a programmer is worried about this sort of thing is just to use a coding standard that always requires braces for the bodies of if-else statements, loops, etc.

My personal style is to always use {} unless the whole IfStatement or similar fits on one line.  Though I do use "else if" rather than

else {
    if (...) {
        ...
    }
}

and I've been known to do things like

else switch (...) {
    ...
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #7 from Stewart Gordon <smjg@iname.com> 2010-09-23 06:03:28 PDT ---
(In reply to comment #0)
> So in D2 I propose to turn that code into an actual syntax error, similar to
> using the warnings-as-errors option of GCC:
> ...>gcc -Wall -Werror test.c -o test
> cc1.exe: warnings being treated as errors
> test.c: In function 'main':
> test.c:6:8: error: suggest explicit braces to avoid ambiguous 'else'

If it were made an error, it wouldn't be a mere suggestion, surely?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #8 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-09-23 09:53:57 PDT ---
I think what Bearophile wants is that in cases where the grammar (and thus the compiler) use the typical dangling else rule to determine that the last else goes with the last if that the compiler should flag it as a warning, on the theory that the programmer probably screwed up. Apparently there are other compilers - including gcc - which flag such as a warning. OF course, most competent programmers would likely have braces around multiple levels of if anyway, thus totally avoiding the issue, but if the programmer doesn't do that, then they can end up with the last else going with an if they didn't intend because they really meant to use braces or perhaps accidentally deleted the if that went with the else (since there could be several lines between the last if and the else and the grammar still sees them as part of the same if statement).

Since other compilers such as gcc flag this sort of thing as a warning, requiring braces to get rid of it, I guess that there's some value in having it be a warning in dmd. However, since I don't think that it's terribly responsible to even code in the way that you have to code to get the issue to show up, I don't think that it's particularly worth having. Still, it wouldn't hurt anyone who was coding more intelligently, since they'd never see the warning anyway.

There's enough value to the enhancement that it may get implemented at some point, but it would be low enough priority that it's probably going to sit in the queue for a very long time if it ever gets implemented. It's the kind of thing that you add to a compiler as you're trying to find incremental improvements to add to a compiler that is essentially already complete (like gcc).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4375



--- Comment #9 from bearophile_hugs@eml.cc 2010-09-23 12:27:44 PDT ---
Stewart Gordon:

> to be an error, rather than to be parsed in one of the two possible ways, doesn't require the compiler to understand indentation at all.

Right, for this enhancement request the indentation is irrelevant.

See bug 4924 for something about indentation.


> If it were made an error, it wouldn't be a mere suggestion, surely?

It is meant first of all as an error.



Jonathan M Davis:

> It's the kind of thing that you add to a compiler as you're trying to find incremental improvements to add to a compiler that is essentially already complete (like gcc).

That's true if you are talking about a warning. But Walter doesn't like warnings, and I think in this case a true error is acceptable. And if it's an error, then it's part of the language, so you can't perform this change too much late. In Bugzilla I have some other very small changes like this one that are hard to do later.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------