Page MenuHomePhabricator

[Diagnostic] add a warning which warns about misleading indentation
ClosedPublic

Authored by Tyker on Nov 23 2019, 3:29 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
xbolva00 added inline comments.Nov 24 2019, 1:47 AM
clang/include/clang/Basic/DiagnosticGroups.td
866

Still typo here

clang/lib/Parse/ParseStmt.cpp
1199

Save P.getPreprocessor().getSourceManager() to variable.

clang/test/SemaCXX/warn-misleanding-indentation.cpp
60 ↗(On Diff #230785)

Add newline

Tyker updated this revision to Diff 230788.Nov 24 2019, 2:35 AM
Tyker marked 5 inline comments as done.

fixed comments.

xbolva00 accepted this revision.Nov 24 2019, 2:46 AM

Thanks.

But please wait for Aaron/Nico.

clang/lib/Parse/ParseStmt.cpp
1198

SourceManager &SM

clang-format this diff?

This revision is now accepted and ready to land.Nov 24 2019, 2:46 AM
aaron.ballman requested changes to this revision.Nov 24 2019, 7:42 AM

Thank you for working on this, it's very useful functionality!

clang/include/clang/Basic/DiagnosticGroups.td
866

Why is this in Most? Shouldn't it be in All, like GCC?

clang/include/clang/Basic/DiagnosticParseKinds.td
65

How about: misleading indentation; %select{statement|declaration}0 is not part of the previous '%select{if|else|for|while}1'?

Right now, you only seem to be covering statements, but I presume you want to also handle cases like:

if (foo)
  bar();
  int baz;

This also fixes a typo with "misleading" and handles localization better by using a selection for the "previous" construct.

clang/test/SemaCXX/warn-misleanding-indentation.cpp
1 ↗(On Diff #230788)

The test case file name has a typo: misleanding -> misleading

You should also add a run line that tests this in C mode, because the parsing rules can be subtly different in C and C++.

8 ↗(On Diff #230788)

I'd like to see a test case where the misleading indentation is not harmful, because we shouldn't diagnose that. e.g.,

if (foo)
  bar();
  ;  // this is not really a misleading indentation (-Wextra-semi-stmt covers it for style)

I'd also like to see a test where the indentation isn't on a statement but is still misleading. e.g.,

if (foo)
  bar();
  int x; // This should diagnose

if (foo)
  bar();
  typedef int frobble; // This should diagnose

if (foo)
  bar();
  [[]]; // No idea how I feel about this diagnosing or not

We should have some test cases for degenerate indentation, e.g.,

if (foo)
bar(); // GCC doesn't think this is misleading, we shouldn't either
baz(); // This is not misleading indentation

And some tests for other forms of the constructs, like if constexpr (whatever) and for (foo : bar)

I won't ask for a test case involving zero-width space characters. ;-)

This revision now requires changes to proceed.Nov 24 2019, 7:42 AM
Ka-Ka added a subscriber: Ka-Ka.Nov 24 2019, 7:46 AM
Tyker updated this revision to Diff 230803.Nov 24 2019, 10:01 AM
Tyker marked 5 inline comments as done.

fixed comment.

Tyker updated this revision to Diff 230804.Nov 24 2019, 10:04 AM
Tyker updated this revision to Diff 230806.
Tyker updated this revision to Diff 230807.Nov 24 2019, 10:07 AM
int bar(void);

int* foo(int* dst)
{
  if (dst)
    return dst + \
    bar();

  return nullptr;
}

This code is kinda weird, but please add it as a test (be sure we do not warn here).

Another case (but I am fine if you decide to not implement it yet, not blocking request)

void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

User forgot to write ; behind return.

aaron.ballman added inline comments.Nov 24 2019, 10:45 AM
clang/test/SemaCXX/warn-misleading-indentation.cpp
18 ↗(On Diff #230807)

Please use the full diagnostic text unless the identical full text has already been verified on the same kind of construct previously (same goes for most of the diagnostics here). We only cut out repetitive bits of frequent diagnostics, generally speaking, but we want the full diagnostics to be checked whenever it's a unique diagnostic.

59–71 ↗(On Diff #230807)

A few more test cases:

// No diagnostics from GCC on this
void f0(int i) {
  if (1)
    i = 2;
  else
         if (i == 3)
    i = 4;
    i = 5;
}

// Or this
#define TEST i = 5
void f0(int i) {
  if (1)
    i = 2;
  else
    i = 5;
    TEST;
}

void f0(int i) {
  if (1)
    i = 2;
  else
    if (i == 3)
    i = 4;
    i = 5; // This one is diagnosed
}

void f0(int i) {
  if (1)
    i = 2;
  else
    if (i == 3)
    {i = 4;}
    i = 5; // As is this one
}
Tyker updated this revision to Diff 230817.Nov 24 2019, 11:27 AM

added tests.
all the tests were already passing.

Tyker marked 2 inline comments as done.Nov 24 2019, 11:27 AM
xbolva00 added a comment.EditedNov 24 2019, 11:45 AM
void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

Well, for this case (void function) we could warn. But okay to ignore it for now.

void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

Well, for this case (void function) we could warn. But okay to ignore it for now.

this can be done in an other patch

Yes, looks fine.

aaron.ballman accepted this revision.Nov 25 2019, 6:52 AM

LGTM, thank you!

This revision is now accepted and ready to land.Nov 25 2019, 6:52 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 12:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the review

xbolva00 reopened this revision.Nov 28 2019, 8:12 AM
xbolva00 added a subscriber: tstellar.

@tstellar reverted this change.

This revision is now accepted and ready to land.Nov 28 2019, 8:12 AM
Tyker updated this revision to Diff 231534.Nov 29 2019, 7:09 AM

yeah i saw. this version of the patch builds all llvm subproject(not tested on llgo) without warnings. there is only one case in llvm's code in which this warning is more agressive thant GCC's.

if (1)
  i = 0;

// comment
  i = 1; //  clang + this patch warns through the comment wereas gcc doesn't.

i don't believe a warning in this case is too bad.

Please land code fixes as NFC patch and then try to reland this patch.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2532 ↗(On Diff #231534)

Please land as separated NFCI commit.

Tyker updated this revision to Diff 231545.EditedNov 29 2019, 7:56 AM

I just found out that Parser::isCXXDeclarationStatement does more then just disambiguation it can emit diagnostics. which can cause error on correct code. so we can't use it in this context to disambiguate.

so it is not possible to easily disambiguate between statements and declaration. i changed the warning message to reflect that but the message isn't so good now. gcc just calls everything a statement with is perhaps a good solution.

xbolva00 added inline comments.Nov 29 2019, 8:57 AM
clang/include/clang/Parse/Parser.h
2274

Extra change? Your code does not use isCXXDeclarationStatement.

there is only one case in llvm's code in which this warning is more agressive thant GCC's.

I think that's okay. If after-commit feedback shows this is bad, we can always tune this diagnostic more and do not warn in that case.

Tyker updated this revision to Diff 231561.Nov 29 2019, 9:16 AM
Tyker updated this revision to Diff 231583.Nov 30 2019, 2:53 AM

Improve the warning for else if

xbolva00 added inline comments.Nov 30 2019, 3:12 AM
clang/lib/Parse/ParseStmt.cpp
1239

I dont think we need this.

Tests will fail anyway if someone removes check() calls.

1377

What is wrong with code you used some rev ago?

if (usable) check();

Now you uselessly instantiate MIChecker since if Usable = false, check is not called.

I like the older code more...

If (usable) checkForMisleadingIndention(...)

Was good and accepted.

Tyker updated this revision to Diff 231584.Nov 30 2019, 3:14 AM
Tyker marked an inline comment as done.Nov 30 2019, 3:17 AM
Tyker added inline comments.
clang/lib/Parse/ParseStmt.cpp
1377

previous patch gave up on else if because we can't know wether there are braces.

this revision can produce correct diagnostics on else if

Tyker updated this revision to Diff 231585.Nov 30 2019, 3:29 AM
Tyker marked an inline comment as done.
Tyker added inline comments.
clang/lib/Parse/ParseStmt.cpp
1377

the MisleadingIndentationChecker gathers information during its construction this allows having more context and removes many false positives.

but I can bring back
If (usable) MIChecker.Check(...)

Looks better, thanks.

Aaron ?

aaron.ballman added inline comments.Dec 1 2019, 8:36 AM
clang/lib/Parse/ParseStmt.cpp
1372–1376

This looks incorrect to me. Consider a case like:

if (0) {
} else [[gsl::suppress("foo")]] if (1) {
}

I'm a little uneasy calling anything but ParseStatement() here as that is what needs to be parsed at this stage.

Tyker updated this revision to Diff 231792.Dec 2 2019, 3:33 PM

improved based on aaron's comment.

aaron.ballman accepted this revision.Dec 3 2019, 6:51 AM

LGTM with some minor nits.

clang/lib/Parse/ParseStmt.cpp
1219

Spurious whitespace, but it would be useful to add a newline above the Check() declaration for visual separation.

1239

This can be hoisted (negated) into the preceding if statement.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2532 ↗(On Diff #231534)

Yeah, these changes seem unrelated to the patch.

Tyker updated this revision to Diff 231949.Dec 3 2019, 11:02 AM
xbolva00 accepted this revision.Dec 3 2019, 11:33 AM
This revision was automatically updated to reflect the committed changes.

As an FYI, this appears to cause several false positive warnings with the Linux kernel:

../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
        if (fb_logo.depth > 4 && depth > 4) {
        ^
../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
        else
        ^
../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
        return ret;
        ^
../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
        if (!ret)
        ^
2 warnings generated.

Corresponding to:

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665

and

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072

fbmem.c case reduced:

https://godbolt.org/z/ma4aFA

Looks like kernel uses tabs here

So we have BinaryOperator <line:5:3, col:9>

If we use spaces: BinaryOperator <line:5:9, col:15> 'int' '='

clang/test/Parser/warn-misleading-indentation.cpp
209

Please add newline.

Tyker marked 6 inline comments as done.EditedDec 4 2019, 9:50 AM

i did a few test on the linux kernel on prior version of this patchs and the mix of spaces and tabs caused some false positives.
for the mix of space and tabs. gcc has a -ftabstop=X to specify how large tabs should be counted as.
clang has it as well i am going to check that it is taken in account.

i will investigate

As an FYI, this appears to cause several false positive warnings with the Linux kernel:

../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
        if (fb_logo.depth > 4 && depth > 4) {
        ^
../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
        else
        ^
../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
        return ret;
        ^
../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
        if (!ret)
        ^
2 warnings generated.

Corresponding to:

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665

and

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072

IMO that is misleading indentation. A single space followed by a tab? WTF

Tyker added a comment.Dec 4 2019, 3:15 PM

the warning was not affected by -ftabstop. i made a patch that fix this.

https://reviews.llvm.org/D71037

and I agree that mixing tabs and space is a horrible idea for many reasons.

IMO that is misleading indentation. A single space followed by a tab? WTF

I guess I will take a look over all of the warnings that cropped up and see if this is the case for all of them. If it is, I will just send patches to fix those (with the justification of the warning and the fact that it would be a checkpatch warning), rather than adding -ftabstop to KBUILD_CFLAGS.

$ ./scripts/checkpatch.pl --terse --types LEADING_SPACE -f drivers/video/fbdev/core/fbmem.c
drivers/video/fbdev/core/fbmem.c:665: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:666: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:667: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:668: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:669: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:670: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:671: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:672: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:673: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:674: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:675: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:676: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:677: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:678: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1063: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1064: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1070: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1075: WARNING: please, no spaces at the start of a line

Hey, cool! I gave this a shot myself many years ago at http://llvm.org/bugs/show_bug.cgi?id=18938 That bug has a list of interesting test cases I had collected back then. Maybe you want to check and see how this does on those cases?

Also, did you check if this has a measurable impact on compilation speed?

Tyker added a comment.Dec 5 2019, 11:14 AM

i have not tested the performance impact. but i tried to do the heavier checks last to minimize the impact.
i added your tests file to the improvement commit.

As a follow up to my previous post, I have sent patches to clean up all of the warnings that I see in the Linux kernel. However, I found one that I do believe is a false positive:

../drivers/staging/uwb/allocator.c:353:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
  alloc_found:
  ^
../drivers/staging/uwb/allocator.c:350:2: note: previous statement is here
        else
        ^
1 warning generated.

Corresponding to https://github.com/torvalds/linux/blob/2187f215ebaac73ddbd814696d7c7fa34f0c3de0/drivers/staging/uwb/allocator.c#L346-L353.

Simplified:

$ cat test.c
int a(int b, int c) {
	if (b)
		goto label;

	if (c)
		return 0;

  label:
	return 1;
}

$ clang -Wmisleading-indentation -o /dev/null -c test.c
test.c:8:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
  label:
  ^
test.c:5:2: note: previous statement is here
        if (c)
        ^
1 warning generated.

goto labels are unaffected by indentation so there should be no warning here. While I think that the labels should be unindented for style, the driver is marked as obsolete and is scheduled to be deleted so I am not sure such a patch would be welcomed.

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

I agree -- @Tyker, are you planning to work on that fix?

Tyker added a comment.Jan 4 2020, 8:43 AM

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

I agree -- @Tyker, are you planning to work on that fix?

i made a patch. https://reviews.llvm.org/D72202

@Tyker Hi, I noticed a case that isn't caught by -Wmisleading-indentation. In code that uses two space indents, there's a corner case that isn't caught when the preceding if uses curly braces. I've noticed a couple instances of this in lldb.

For example:

if (cond) {
  then1();
  then2();
  // ...
} else
  else1();
  else2();

The else2(); statement doesn't trigger the warning.

It seems that the logic is to use the column of the else keyword, not the column of the right brace. When using a four space indent (any indent != 2), the warning is emitted:

} else
    else1();
    else2();