Bug 27462 – Add cpplint check for proper include order

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Treat]

cpplint should check to make sure that header files and implementation files follow the proper include order and style mandated here:

http://webkit.org/coding/coding-style.html

The only thing not checked here is whether includes surrounded by ifdefs are put at the end.

[Adam Treat]

Created attachment 33109 [details]
Checks include order and tests

[David Levin]

Comment on attachment 33109 [details]
Checks include order and tests

Here some initial comments to consider.  The checking of header states really took me some time to get through (and I need to look at it a little more).  Since I'm not done, I won't give this an r-.


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> [phone]  Adam Treat  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add a new check to cpplint to flag cases where the include section of a file
> +        does not match the mandated include order and style of the Webkit coding style
> +        guidelines.
> +
> +        Add associated tests.
> +        https://bugs.webkit.org/show_bug.cgi?id=27462

I think the style du jour for WebKit changelog's is to have it like this:

bug title
bug link

description

> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py

> @@ -273,31 +272,28 @@ class _IncludeState(dict):
>      # self._section will move monotonically through this set. If it ever
>      # needs to move backwards, check_next_include_order will raise an error.

Note the comment for the "class _IncludeState(dict):"

 "Calls in an illegal order will raise an _IncludeError with an appropriate error message."

is incorrect as it was before you started your change (but time to fix it).



> +        errorMessage = ''

You switchedToWebkitCStyleNames instead of python_style_names (note: beforeErrorMessage and afterErrorMessage as well).

> +        if (self._section != self._OTHER_H_SECTION):
> +            beforeErrorMessage = ('Found %s before %s' %
> +                                    (self._TYPE_NAMES[header_type],
> +                                     self._SECTION_NAMES[self._section + 1]));
1. The indenting appears to be off.
2. No need for ; in python.

> +        afterErrorMessage = ('Found %s after %s' %
> +                                (self._TYPE_NAMES[header_type],
> +                                 self._SECTION_NAMES[self._section]));
> +


> +        if header_type == _CONFIG_HEADER:
> +            if self._section > self._CONFIG_SECTION:
Perhaps:
            if self._section >= self._CONFIG_SECTION:

since there is only one item in the config section.


>          elif header_type == _LIKELY_MY_HEADER:
> +            if self._section == self._OTHER_H_SECTION:

Consider:
            if self._section > self._MY_H_SECTION:


>              assert header_type == _OTHER_HEADER
> +            if not file_is_header and self._section != self._MY_H_SECTION and self._section != self._OTHER_H_SECTION:
> +                errorMessage = beforeErrorMessage

I think this is to catch missing config.h files.  Maybe the error should just say that "This cpp files appears to not include config.h or config,h is not included first."


> +                 if (not is_blank_line(next_line)):

I've noticed that you did a lot of if statements with surrounding ().  There isn't a need for this (unless you want to continue the condition on to a new line).

[Adam Treat]

Created attachment 33175 [details]
New version incorporating comments

I've greatly simplified _classify_include following discussions on IRC and incorporated your other comments into this new version.

[David Levin]

Comment on attachment 33175 [details]
New version incorporating comments

This looks really good.  Just a few things to address.

> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +        if header_type == _CONFIG_HEADER and file_is_header:
> +            return 'Header file should not contain WebCore config.h.'
> +        if header_type == _PRIMARY_HEADER and file_is_header:
> +            return 'Header file should not contain itself.'
> +
> +        error_message = ''
> +        if self._section != self._OTHER_SECTION:
> +            before_error_message = ('Found %s before %s' %
> +                                    (self._TYPE_NAMES[header_type],
> +                                     self._SECTION_NAMES[self._section + 1]))
> +        after_error_message = ('Found %s after %s' %
> +                                (self._TYPE_NAMES[header_type],
> +                                 self._SECTION_NAMES[self._section]))
> +
> +        if header_type == _CONFIG_HEADER:
> +            if self._section >= self._CONFIG_SECTION:
> +                error_message = after_error_message
> +            self._section = self._CONFIG_SECTION
> +        elif header_type == _PRIMARY_HEADER:
> +            if self._section >= self._PRIMARY_SECTION:
> +                error_message = after_error_message
> +            elif self._section < self._CONFIG_SECTION:
> +                error_message = before_error_message

I think this is primarily here to note that the config.h header include may be missing.  

Maybe it is better just have the message say something about the missing include.
"It appears that config.h wasn't included (or it wasn't included first)."


> +            if not file_is_header and self._section < self._PRIMARY_SECTION:

This seems to be ensuring that we got to the primary header.  It is rare but sometimes there is no primary header.
I think the only thing to check here is that the missing config.h file.  The check becomes:
   if not file_is_header and self._section < self._CONFIG_SECTION:

Have the same message as before about missing config.h.  This gets rid of before_error_message by the way.


> +            # Check to make sure all headers besides config.h and the primary header are
> +            # alphabetically sorted.
> +            if not error_message and header_type == _OTHER_HEADER:
> +                 previous_line_number = line_number - 1;
> +                 previous_line = clean_lines.lines[previous_line_number]
> +                 previous_match = _RE_PATTERN_INCLUDE.search(previous_line)
> +                 while not previous_match and previous_line_number > 0:
> +                    previous_line_number -= 1;
> +                    previous_line = clean_lines.lines[previous_line_number]
> +                    previous_match = _RE_PATTERN_INCLUDE.search(previous_line)

I would try to match #ifdef/#else here and break out if you get that without a previous match (just to avoid flagging a sorting issue when there is an ifdef'ed group after the regular includes.



> diff --git a/WebKitTools/Scripts/modules/cpplint_unittest.py b/WebKitTools/Scripts/modules/cpplint_unittest.py

> +    def test_check_next_include_order__no_config(self):
> +        self.assertEqual('Header file should not contain WebCore config.h.',
>                           self.include_state.check_next_include_order(
> -                             cpplint._CPP_SYS_HEADER))
> +                            cpplint._CONFIG_HEADER, True))
You seem to be doing a two space indenting here.  Anyway, I just wouldn't wrap the arguments.


It would be nice to add a test for this case as well:

   #include "test.h"

   #ifdef MY_DEFINE
   #include "a.h"
   #endif
Just to verify that a.h ins't flagged as a sorting error.



There is a test that says this:
3097	    def test_include(self):
3098	        # FIXME: Implement this.
3099	        pass

Should this test be deleted (or just add a comment there saying where it is tested) or should the header tests move there? etc.

[Adam Treat]

(In reply to comment #4)
> (From update of attachment 33175 [details])
> This looks really good.  Just a few things to address.
...
> I think this is primarily here to note that the config.h header include may be
> missing.  
> 
> Maybe it is better just have the message say something about the missing
> include.
> "It appears that config.h wasn't included (or it wasn't included first)."

It is there to note the correct order, not just to see if the config.h is missing.  It does say something if there is a missing include whether it is a config header or the primary header.
 
The error messages look like this (from the test cases):

    "Found header this file implements before WebCore config.h."

and

    "Found other header before WebCore config.h."

and

    "Found other header before a header this file implements."

I think these are quite clear error messages and the test cases encompass the range of possible errors pretty well.  I don't see any need to simplify further.

> 
> > +            if not file_is_header and self._section < self._PRIMARY_SECTION:
> 
> This seems to be ensuring that we got to the primary header.  It is rare but
> sometimes there is no primary header.
> I think the only thing to check here is that the missing config.h file.  The
> check becomes:
>    if not file_is_header and self._section < self._CONFIG_SECTION:

I think this is incorrect.  As you say, it is rare that an implementation file does not have a primary header.  I see no reason to ignore the check for the vast majority of cases to stop a false positive on the rare corner case.  That is why the error message has a confidence of 4 and not 5.  We could bump this down to 3 if you like, but I think the check should stay.
 
> I would try to match #ifdef/#else here and break out if you get that without a
> previous match (just to avoid flagging a sorting issue when there is an
> ifdef'ed group after the regular includes.

Ok, will do.
 
> > +    def test_check_next_include_order__no_config(self):
> > +        self.assertEqual('Header file should not contain WebCore config.h.',
> >                           self.include_state.check_next_include_order(
> > -                             cpplint._CPP_SYS_HEADER))
> > +                            cpplint._CONFIG_HEADER, True))
> You seem to be doing a two space indenting here.  Anyway, I just wouldn't wrap
> the arguments.

Ok, will do.

> 
> It would be nice to add a test for this case as well:
> 
>    #include "test.h"
> 
>    #ifdef MY_DEFINE
>    #include "a.h"
>    #endif
> Just to verify that a.h ins't flagged as a sorting error.
> 

Ok, will do.
 
> 
> There is a test that says this:
> 3097        def test_include(self):
> 3098            # FIXME: Implement this.
> 3099            pass
> 
> Should this test be deleted (or just add a comment there saying where it is
> tested) or should the header tests move there? etc.

I'll remove it.

Thanks...

Patch to follow!

[Adam Treat]

Created attachment 33187 [details]
More changes reflecting comments

New version reflecting the state of the review conversation.

[Adam Treat]

Created attachment 33191 [details]
New version that special cases 'FooCustom.cpp'

New version that special cases 'FooCustom.cpp' as discussed on IRC.

[David Levin]

Comment on attachment 33191 [details]
New version that special cases 'FooCustom.cpp'

Would be nice to make s small adjustment on landing.

> +                if filename.endswith('.h'):
> +                    error(filename, line_number, 'build/include_order', 4,
> +                          '%s Should be: alphabetically sorted.' %
> +                          (error_message))
No need for parens on error_message (since it is a single element).

Btw, wierd python-ism: the parens do nothing here :) 
to make it a tuple with one element you'd have to do this: (error_message,)
 


> +                else:
> +                    error(filename, line_number, 'build/include_order', 4,
> +                          '%s Should be: config.h, primary header, blank line, and then alphabetically sorted.' %
> +                          (error_message))

Same comment about (error_message)

[Adam Treat]

Landed with last changes mentioned with r46181.