www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 4375] New: Require explicit braces when 'else' is ambiguous

reply d-bugmail puremagic.com writes:
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



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: -------
Jun 23 2010
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


Jonathan M Davis <jmdavisProg gmail.com> changed:

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



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: -------
Jun 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Jun 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Jun 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Sep 20 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Sep 20 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


Stewart Gordon <smjg iname.com> changed:

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




 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: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375





 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: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




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: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




PDT ---
Except that this case is a classic case in computer programming with a clear
and standard solution. The grammar is totally unambiguous. I can't see Walter
ever agreeing to make it an error. No other language that I'm aware of does. A
warning makes some sense. An error does not. Granted, Walter doesn't like
warnings, but that doesn't mean that there's no way that he's going to add it,
and there's always the possibility that another D compiler down the road would
flag it as a warning. Just because Walter wouldn't want to make it a warning in
dmd doesn't mean that it should become an error.

If it's a question of error or nothing, I'm strongly behind nothing, and my
guess is that Walter will be as well. I'm fine with it becoming a warning, but
an error is too strong. The language is quite clear on the matter.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




Jonathan M Davis:

The grammar is totally unambiguous.<
For a computer. But people aren't that precise. The syntax of a computer language is not designed for the computer, it's designed to be an interface for the very bug-prone apes that use the computer.
I can't see Walter ever agreeing to make it an error.<
In D there are several precedents, this is a bug in D only: for (i = 0; i < 10; i++); -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375





 In D there are several precedents, this is a bug in D only:
 for (i = 0; i < 10; i++);
It isn't a bug, it's illegal code. A bug is something different. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 23 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




Just for reference this is what Wikipedia says about the Ada language:

Conditional statements are closed with "end if", avoiding a dangling else that
could pair with the wrong nested if-expression in other languages.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Oct 11 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com



14:28:11 PST ---
Stewart's right in that this change can be effected by changing the grammar. 

You can get into all kinds of confusion by perversely indenting the source
code, this particular one isn't special and so doesn't warrant special
treatment.

What I think would be of significant value is to create a D source code
formatter that will properly indent the code based on the grammar. I'd love
such a tool, and would use it (for example) as a filter on all D source code
checkins.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 07 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375






 Stewart's right in that this change can be effected by changing the grammar. 
Good, the grammar may just require braces in this case. This is a good enough solution.
 You can get into all kinds of confusion by perversely indenting the source
 code, this particular one isn't special and so doesn't warrant special
 treatment.
From what I have seen this particular indentation is a common enough bug, while other indentations aren't a common enough source of bugs. But you are right, there are other wrong kinds of indentations that may be sources of bugs that will enjoy specific warnings (example: unexpected negative indents warning).
 What I think would be of significant value is to create a D source code
 formatter that will properly indent the code based on the grammar. I'd love
 such a tool, and would use it (for example) as a filter on all D source code
 checkins.
The purpose of a modern IDE is to contain few hundred functionalities like this one. Using a different tool for those functionalities is not practical nor is what most modern programmers look for. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375






 You can get into all kinds of confusion by perversely indenting the source
 code, this particular one isn't special and so doesn't warrant special
 treatment.
This particular problem caused by indenting has even a name, "dangling else problem". When a problem gains a name it means it's not something equal to other things, it's different enough. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




I've being hit by a "dangling else" bug in D, traslating this Python code:


def foo(sol):
    global best
    if is_solution(sol[-1]):
        if best or len(sol) < len(best):
            best = list(sol)
    else:
        for next in alternatives(sol[-1]):
            if next not in sol:
                foo(sol + [next])


to the wrong D code:


void foo(Pair[] sol) {
    if (isSolution(sol[$-1]))
        if (!best.length || sol.length < best.length)
            best = sol.dup;
    else
        foreach (next; alternatives(sol[$-1]))
            if (!canFind(sol, next))
                foo(sol ~ [next]);
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 03 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


Nick Sabalausky <cbkbbejeap mailinator.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cbkbbejeap mailinator.com



14:17:14 PDT ---
"You can get into all kinds of confusion by perversely indenting the source
code, this particular one isn't special and so doesn't warrant special
treatment."

I disagree with this assessment. The whitespace is not the core problem here.
The core problem is that the construct "if(cond) stmt; if(cond) stmt; else..."
(with no braces) is generally confusing for people. The whitespace merely makes
for a more illustrative example. To demonstrate, the following code is
perfectly valid and works as intended, but despite that, it would still make a
lot of programmers very nervous anyway:

if(x == 17)
    if(useFoo) foo(); else bar();

While it's true there's plenty of general confusion that can be caused by
misleading whitespace, that's really a separate class of issues. It's a class
of issues that can certainly aggravate the "if if else" matter, but that's just
because, like you said, they can aggravate any situation.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 21 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375




See also:
http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=142984

http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=142985

http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=142993

Implementation ideas:
http://code.google.com/p/copute/issues/detail?id=21

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 23 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


kennytm gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
                 CC|                            |kennytm gmail.com





https://github.com/D-Programming-Language/dmd/pull/336

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 26 2011
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4375


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED



23:07:21 PDT ---
https://github.com/D-Programming-Language/dmd/commit/31052bdfdf6ab96e05200eb7087fdda84a7f49bc

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