[clang] better error message for while loops outside of control flow

[inclyc]

Summary

report an error when encountering 'while' token parsing declarator

clang/test/Parser/while-loop-outside-function.c:3:1: error: while loop outside of a function
while // expected-error {{while loop outside of a function}}
^
clang/test/Parser/while-loop-outside-function.c:7:1: error: while loop outside of a function
while // expected-error {{while loop outside of a function}}
^

Fixes https://github.com/llvm/llvm-project/issues/34462

[mizvekov]

clang/test/Parser/while-loop-outside-function.c

4–5

How about just making the error be the same thing the note would say?

clang/test/Parser/while-loop-outside-function.cpp

28

Missing newline

[inclyc]

clang/test/Parser/while-loop-outside-function.c

4–5

OK, so just create a new error directly, and then report the error when encountering the while token?

[mizvekov]

clang/test/Parser/while-loop-outside-function.c

4–5

Well the end result should be that there is only one error, while outside of function, because that explains everything the user needs to know.

It would be strange to report the expected identifier or ... error in addition to this one, it would be just noise.

How to go about implementing that I leave it up to you :P
Unless you need some specific help of course.

[inclyc]

Now it only generates 1 error encountering token "while"

./local/while_loop_outside_func.cpp:1:1: error: while loop outside of function
while (true) {
^
1 error generated.

[mizvekov]

In D129573#3659404, @inclyc wrote:

Now it only generates 1 error encountering token "while"

Yes thank you, that is what I meant!

But now we just gotta make sure this stays true for more complex test cases.

clang/include/clang/Basic/DiagnosticParseKinds.td

542

Looking at other diagnostics which contain the word outside, there are many different ways to spell this.

Other possibilities include:

• while loop outside function
• while loop outside of a function
• while loop outside of a function body

All of these alternates could be justified with some prior art here.
I have no strong opinions either way, this seems fine, just making a note.

clang/test/Parser/while-loop-outside-function.c

4

You could break this down further into more lines to test exactly where the error is placed. Id expect it to be placed at the while keyword.

9

Can you add a few more test cases showing how error recovery is performing here?

Have we parsed this function declaration at all, or were we skipping until the next ';'?

What happens if there are (multiple) statements in the loop's block?

How do we handle a do ... while() loop instead?

Does it make a difference if the loop contains a block or a (possibly empty) single statement?

[inclyc]

clang/test/Parser/while-loop-outside-function.c

9

For this occasion,

// RUN: %clang_cc1 -fsyntax-only -verify %s

while(true) {}; // expected-error {{while loop outside of function}}

// without semicolon
while(true) {} // expected-error {{while loop outside of function}}


while(true) {
    // some statements
    int some_var = 3;
    some_var += 2;
}

while(true) 
{
    // some statements
    int some_var = 3;
    some_var += 2;
}

do {
    int some_var = 1;
    some_var += 3;
} 
while(true);

void someFunction() {
    while(true) {};
}

class SomeClass {
public:
    while(true) {}
    void some_fn() {
        while(true) {}
    }
};

./clang/test/Parser/while-loop-outside-function.cpp:3:1: error: while loop outside of function
while(true) {}; // expected-error {{while loop outside of function}}
^
./clang/test/Parser/while-loop-outside-function.cpp:6:1: error: while loop outside of function
while(true) {} // expected-error {{while loop outside of function}}
^
./clang/test/Parser/while-loop-outside-function.cpp:9:1: error: while loop outside of function
while(true) {
^
./clang/test/Parser/while-loop-outside-function.cpp:15:1: error: while loop outside of function
while(true) 
^
./clang/test/Parser/while-loop-outside-function.cpp:22:1: error: expected unqualified-id
do {
^
./clang/test/Parser/while-loop-outside-function.cpp:26:1: error: while loop outside of function
while(true);
^
./clang/test/Parser/while-loop-outside-function.cpp:34:5: error: expected member name or ';' after declaration specifiers
    while(true) {}
    ^
7 errors generated.

The parser generates the newly added error for every "top-level" declarator. For do ... while(...) loops it generates two errors. I think ideally only one error should be reported, only generating an error at the latter while token, without generating a "noise" at the preceding do. And, this patch doesn't seem to produce good results where classes, structs, etc. are also not part of the function body.

At first, when I solved this issue, I thought he only needed to report the appearance of the top-level while loop, without considering do-while, in a class, or in a structure. So the current code is only in Parser, this error is reported when the while token is encountered parsing declarators.

[inclyc]

clang/test/Parser/while-loop-outside-function.c

4

Passes this test case, I will submit this in the next patch.

// RUN: %clang_cc1 -fsyntax-only -verify %s

while(1) {}; // expected-error {{while loop outside of function}}


while(1) // expected-error {{while loop outside of function}}
{

}

while(1) { // expected-error {{while loop outside of function}}

}

while(1) { // expected-error {{while loop outside of function}}

};

while(1) // expected-error {{while loop outside of function}}
{

}

while(1) { // expected-error {{while loop outside of function}}

};

// without semicolon
while(1) {} // expected-error {{while loop outside of function}}

void some_fn();

void some_fn() {
    while(1) {};
}

[mizvekov]

clang/test/Parser/while-loop-outside-function.c

9

I see, thanks for the explanations.

I think this patch is a very simple change that brings some improvement, and apparently it does not regress anything or cause any unfortunate error recovery.

You don't really need to improve these other cases in the same patch, I am just checking that this was all considered :-)

But please do add these test cases to the patch!

[inclyc]

clang/test/Parser/while-loop-outside-function.c

9

Do you think we need to change something more to accurately report while / do-while / for loops, and only report errors outside of a reasonable control flow?

[inclyc]

clang/test/Parser/while-loop-outside-function.c

9

Right now this patch is far from perfect, ideally it should report various types of loops (as mentioned above) and also in a non-control flow context, such as class members, typedefs, etc. (I don't know how to do this, sorry)

[mizvekov]

clang/test/Parser/while-loop-outside-function.c

9

Don't worry about it! I think this patch is pretty good bang for the buck, for the cost of basically two lines of code you get much better diagnostic for the bulk of the interesting cases, without regressing anything.

11–20

I think you don't really need to test these two cases separately. They differ by just white space.

The one thing that I did suggest is that, for at least one of your test cases, put the while keyword alone by itself on a line,
so that you can test that the diagnostic points to the location of the keyword.

[inclyc]

Is the build error related to this patch?

[mizvekov]

In D129573#3664905, @inclyc wrote:

Is the build error related to this patch?

I don't think so, it's not even close to what you are changing.

[inclyc]

Could you please review the latest patch ? I have removed some redundant test cases mentioned above

[mizvekov]

clang/test/Parser/while-loop-outside-function.c

5

What I meant is something like this, we test that the error caret would be placed on the while keyword.
You have to break it down like this as you can't express in the verifier where an error would occur inside a line.

23

To demonstrate recovery, I think it would make sense to try to use this declaration later, and test that it would work.
Ie you can try to 'overload' on the return type and see that you get the expected error.

[inclyc]

clang/test/Parser/while-loop-outside-function.c

4

Thanks a lot for your suggestion, I replaced this statement with two lines to test if the diagnostics are correctly given at the token while.

The C++ test cases is also changed like this.

11

This test case checks whether the program can correctly diagnose other errors after reporting our newly added error

25

Should not give diagnostic information at these 2 lines of correct statements

[mizvekov]

Thanks! The patch itself LGTM.

Please update / fix up the commit message before merging. It still mentions note and has some minor English spelling issues.

clang/include/clang/Basic/DiagnosticParseKinds.td

def err_expected_member_name_or_semi_objcxx_keyword : Error<
   "expected member name or ';' after declaration specifiers; "
   "%0 is a keyword in Objective-C++">;
 
 /// C++ parser diagnostics
 def err_invalid_operator_on_type : Error<
   "cannot use %select{dot|arrow}0 operator on a type">;
 def err_expected_unqualified_id : Error<
   "expected %select{identifier|unqualified-id}0">;
+def err_while_loop_outside_of_a_function : Error<
+  "while loop outside of a function">; 
 def err_brackets_go_after_unqualified_id : Error<
   "brackets are not allowed here; to declare an array, "
   "place the brackets after the %select{identifier|name}0">;
 def err_unexpected_unqualified_id : Error<"type-id cannot have a name">;
 def err_func_def_no_params : Error<
   "function definition does not declare parameters">;
 def err_expected_lparen_after_type : Error<
   "expected '(' for function-style cast or type construction">;

clang/lib/Parse/ParseDecl.cpp

if (D.getContext() == DeclaratorContext::Member) {
         D.SetRangeEnd(Tok.getLocation());
         ConsumeToken();
         goto PastIdentifier;
       }
       Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
            diag::err_expected_member_name_or_semi)
           << (D.getDeclSpec().isEmpty() ? SourceRange()
                                         : D.getDeclSpec().getSourceRange());
+    } else {
+      if (Tok.getKind() == tok::TokenKind::kw_while) {
+        Diag(Tok, diag::err_while_loop_outside_of_a_function);
       } else if (getLangOpts().CPlusPlus) {
         if (Tok.isOneOf(tok::period, tok::arrow))
           Diag(Tok, diag::err_invalid_operator_on_type) << Tok.is(tok::arrow);
         else {
           SourceLocation Loc = D.getCXXScopeSpec().getEndLoc();
           if (Tok.isAtStartOfLine() && Loc.isValid())
             Diag(PP.getLocForEndOfToken(Loc), diag::err_expected_unqualified_id)
                 << getLangOpts().CPlusPlus;
           else
             Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
                  diag::err_expected_unqualified_id)
                 << getLangOpts().CPlusPlus;
         }
       } else {
         Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
              diag::err_expected_either)
             << tok::identifier << tok::l_paren;
       }
+    }
     D.SetIdentifier(nullptr, Tok.getLocation());
     D.setInvalidType(true);
   }
 
  PastIdentifier:
   assert(D.isPastIdentifier() &&
          "Haven't past the location of the identifier yet?");
 

clang/test/Parser/while-loop-outside-function.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+while // expected-error {{while loop outside of a function}}
+(1) {};
+
+// without semicolon
+while // expected-error {{while loop outside of a function}}
+(1) {}
+
+int overload_return(); // expected-note {{previous declaration is here}}
+
+void overload_return() // expected-error {{conflicting types for 'overload_return'}}
+{ 
+    while(1) {};
+    while(1);
+}
+
+while // expected-error {{while loop outside of a function}}
+(1); 
+
+void correct();
+
+void correct() {
+    while(1) {};
+    while(1);
+}

clang/test/Parser/while-loop-outside-function.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+while // expected-error {{while loop outside of a function}}
+(true) {};
+
+// without semicolon
+while // expected-error {{while loop outside of a function}}
+(true) {}
+
+do { // expected-error {{expected unqualified-id}}
+    int some_var = 1;
+    some_var += 3;
+} 
+while // expected-error {{while loop outside of a function}}
+(true); 
+
+void someFunction() {
+    while(true) {};
+}
+
+class SomeClass {
+public:
+    while(true) {} // expected-error {{expected member name or ';' after declaration specifiers}}
+    void some_fn() {
+        while(true) {}
+    }
+};