Bug 173559 – lint-test-expectations should be run during style checking

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Talking to a few bot-watchers, no one was aware of the stand-alone lint-test-expectations script and it is not regularly being checked (Note that our bots actually run a variant of lint-test-expectations every time they run the layout tests).  To prevent errors in the test-expectations from finding their way into the tree, we should run lint-test-expectations on during style checking, only printing out errors in the changed expectation files.

[Radar WebKit Bug Importer]

<rdar://problem/32854941>

[Jonathan Bedard]

Created attachment 313324 [details]
Patch

[Daniel Bates]

Comment on attachment 313324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313324&action=review

> Tools/Scripts/webkitpy/style/main.py:168
> +        linter_path = host.filesystem.join(host.filesystem.dirname(__file__), '..', '..', 'lint-test-expectations')
> +        lint_output = host.executive.run_command(['python', linter_path], error_handler=lambda error: 0)
> +        for line in lint_output.splitlines():
> +            if line:
> +                for file in file_reader.files:
> +                    if file in line:
> +                        _log.info(line)
> +                        error_count += 1
> +                        break

We should put this logic in a helper function.

Notice that the script lint-test-expectations is implemented using the webkitpy code in Tools/Scripts/webkitpy/layout_tests/lint_test_expectations.py and ultimately uses functionality on test_expectations.TestExpectations() to parse each test expectation file. Can we do this more directly by implementing similar logic and only listing a file if it is in the patch and is a port expectation file?

[Jonathan Bedard]

Created attachment 313412 [details]
Patch

[Jonathan Bedard]

Attachment 313412 [details] only lints expectation files which have been changed.  Causes quite a bit of code duplication, though, which is the reason I had originally just called lint-test-expectations.

This patch will be a bit fast than the first, since it doesn't lint every test expectation file.

[Jonathan Bedard]

Comment on attachment 313412 [details]
Patch

I talked in person with Dan Bates about this change yesterday (6/22).  He would like the linter inside check-webkit-style to only report errors caused by new changes and for the resulting to output to be more inline with what check-webkit-style uses.

[Jonathan Bedard]

Created attachment 313855 [details]
Patch

[Jonathan Bedard]

Attachment 313855 [details] still needs tests, but I would like to make sure that this is the correct approach before constructing tests for this change.

[Jonathan Bedard]

Created attachment 314760 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 314760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314760&action=review

r=me, although this patch is complex enough it wouldn't hurt to get a second set of eyes.

> Tools/ChangeLog:11
> +        Running the test expectation linter requires reading both files and lines not in the
> +        patch because, for example, deletion of a test can cause a lint failure even though
> +        not test expectations where modified.

I feel there's more to be said here.  What is the implication of this statement?  Maybe something like this?

This means that the linter may occasionally warn about lines that weren't changed in the immediate patch, but which were triggered by those changes.  Care is taken to reduce the number of warnings outside the changes made within a patch, though.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:401
> +        self.related_files = {}  # Dictionary of files to lines in that file. Storing associated warnings

Nit: Comment should end with period.

[Jonathan Bedard]

Created attachment 315854 [details]
Patch

[Dean Johnson]

Comment on attachment 315854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315854&action=review

Looks good overall, and thanks for adding tests! Few stylistic / pythonic idioms comments, but functionally nothing seems wrong/off to me. Unofficial r=me w/ my comments.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:64
> +class Warning(object):

I feel like this should be called TestExpectationWarning instead of just 'Warning' as to provide greater clarity as to what this object is representing.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:103
> +        self._shorten_filename = shorten_filename or (lambda x: x)

You could default to 'str' (the function) instead of 'None' and avoid doing the 'or (lambda x: x)'.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:401
> +        self.related_files = {}  # Dictionary of files to lines in that file associated with the warnings of this line.

Can you update this comment to be a bit more clear? Specifically the latter half.

> Tools/Scripts/webkitpy/style/checker.py:815
> +    def do_association_check(self, files, cwd, host=Host()):

Nice job with the docstring here!

> Tools/Scripts/webkitpy/style/filereader.py:67
> +        self._files = {}

Nit: Use collections.defaultdict here instead, like so:
import collections
self._files = collections.defaultdict(list)

> Tools/Scripts/webkitpy/style/filereader.py:112
> +        abs_file_path = self.filesystem.abspath(file_path)  # Unifies patch and cherry-picking code paths.

This comment for this line specifically adds confusion for me... it seems like it'd be better placed in the ChangeLog, or you should describe why it's implemented this way in the docstring.

> Tools/Scripts/webkitpy/style/filereader.py:113
> +        if not abs_file_path in self._files:

Nit: not abs_file_path in self._files: => abs_file_path not in self._files:

> Tools/Scripts/webkitpy/style/filereader.py:119
>  

You can remove the first if block, and the inner if block of `if 'line_numbers' in kwargs` by using collections.defaultdict. It will essentially auto-create a list for your key when instantiating a new key on the dict. This allows you to easily do:
self._files.extend(kwargs['line_numbers'])

I would rewrite the entire block as follows:
abs_file_path = self.filesystem.abspath(file_path)
line_numbers = kwargs.get('line_numbers)
if line_numbers is not None:
  self._files[abs_file_path].extend(line_numbers)

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:127
> +        files_linted = set()

Super-nit: Swap the order of these for readability. Also remove the excessive new-line above.

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:129
> +            expectations_dict = port.expectations_dict()

Nit: expectations_dict is only used here and doesn't provide much value. Let's just in-line the port.expectations_dict().keys() call.

[Kocsen Chung]

Comment on attachment 315854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315854&action=review

Overall seems solid, I have a few nits listed here and there as well as some questions.

> Tools/ChangeLog:3
> +        lint-test-expectations should be run during style checking

Nit: period at the end of this line?

> Tools/ChangeLog:11
> +        not test expectations where modified. This means that the linter will occasionally warn

not => no

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:335
> +    def open_text_file_for_reading(self, path, errors='strict'):

Where is this new parameter 'errors' used?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:64
> +class Warning(object):

"Warning" is too generic of a name IMO. What about "TextExpectationWarning"? If that's too long maybe "TestWarning"?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:103
> +        self._shorten_filename = shorten_filename or (lambda x: x)

Why are we passing `shorten_filename` as a parameter? Why not have the private function within TestExpectationParser?

> Tools/Scripts/webkitpy/port/win.py:52
> +    _log.debug("Not running on native Windows.")  # Importing win.py on non-windows systems is a common use case.

Nit: The in-line comment is good to explain why you made this change when reviewing the patch, but I don't think we should check it in.

[Jonathan Bedard]

Comment on attachment 315854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315854&action=review

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:335
>> +    def open_text_file_for_reading(self, path, errors='strict'):
> 
> Where is this new parameter 'errors' used?

This is a mock object.  It should have had this the entire time since the actual object has this.  The mock object doesn't use it.

>>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:103
>>> +        self._shorten_filename = shorten_filename or (lambda x: x)
>> 
>> You could default to 'str' (the function) instead of 'None' and avoid doing the 'or (lambda x: x)'.
> 
> Why are we passing `shorten_filename` as a parameter? Why not have the private function within TestExpectationParser?

To Kocsen's question: It's the same idiom used in TestExpectationsModel.

>> Tools/Scripts/webkitpy/style/filereader.py:67
>> +        self._files = {}
> 
> Nit: Use collections.defaultdict here instead, like so:
> import collections
> self._files = collections.defaultdict(list)

In this case, using a defaultdict isn't the correct approach since 'None' vs 'not in the dictionary' are not the same thing.  Take a look at the file deletion code on line 153.  Generally, None is used to indicate that the entire file has been changed (with a move or a delete) while a list of line numbers tracks changes in specific lines in a file.  This results in a bit more code in TextFileReader.process_file(...), but much less code in TestExpectationChecker.lint_test_expectations(...)

>> Tools/Scripts/webkitpy/style/filereader.py:119
>>  
> 
> You can remove the first if block, and the inner if block of `if 'line_numbers' in kwargs` by using collections.defaultdict. It will essentially auto-create a list for your key when instantiating a new key on the dict. This allows you to easily do:
> self._files.extend(kwargs['line_numbers'])
> 
> I would rewrite the entire block as follows:
> abs_file_path = self.filesystem.abspath(file_path)
> line_numbers = kwargs.get('line_numbers)
> if line_numbers is not None:
>   self._files[abs_file_path].extend(line_numbers)

See my above comment.  Because of how deleted files are handled, this isn't the correct approach.

[Jonathan Bedard]

Created attachment 315922 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315922 [details]
Patch

Clearing flags on attachment: 315922

Committed r219657: <http://trac.webkit.org/changeset/219657>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 315922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315922&action=review

> Tools/Scripts/webkitpy/style/filereader.py:159
>          """Count up files that contains only deleted lines.
>  
>          Files which has no modified or newly-added lines don't need
>          to check style, but should be treated as checked. For that
>          purpose, we just count up the number of such files.
>          """

This comment is out-of-date.

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:123
> +    def lint_test_expectations(files, configuration, cwd, increment_error_count=lambda: 0, line_numbers=None, host=Host()):

Is it necessary to have default values for increment_error_count and host?

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
> +                    expectations_file,
> +                    configuration,
> +                    increment_error_count,
> +                    line_numbers)

See my remark above about exploding function arguments.

[Daniel Bates]

Comment on attachment 315922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315922&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:95
> +    def __init__(self, port, full_test_list, allow_rebaseline_modifier, shorten_filename=str):

How did you come to the decision to use str() as the default value for shorten_filename? Does this mean that we can never have Unicode characters in the names of layout tests? This default value does not match the default value of the same argument in the TestExpectationsModel constructor.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:745
> +        if expectation_line.related_files.get(self._shorten_filename(prev_expectation_line.filename), None) is None:
> +            expectation_line.related_files[self._shorten_filename(prev_expectation_line.filename)] = []
> +        expectation_line.related_files[self._shorten_filename(prev_expectation_line.filename)].append(prev_expectation_line.line_number)

This code repeatedly computes the shortened filename and underutilizes the default value for .get(). I would have written this as:

previous_expectation_filename = self._shorten_filename(prev_expectation_line.filename)
expectation_line.related_files[previous_expectation_filename] = expectation_line.related_files.get(previous_expectation_filename, [])
expectation_line.related_files[previous_expectation_filename].append(prev_expectation_line.line_number)

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:748
> +        if prev_expectation_line.related_files.get(self._shorten_filename(expectation_line.filename), None) is None:
> +            prev_expectation_line.related_files[self._shorten_filename(expectation_line.filename)] = []

I would take a similar approach as above.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:749
> +        prev_expectation_line.related_files[self._shorten_filename(expectation_line.filename)].append(prev_expectation_line.line_number)

Did you mean to append prev_expectation_line.line_number?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:971
> +                    self._shorten_filename(expectation.filename),
> +                    expectation.line_number,
> +                    expectation.original_string,
> +                    warning,
> +                    expectation.name if expectation.expectations else None)

As far as I know exploding function arguments such that there is one argument per line is not the style we use.

[Daniel Bates]

Comment on attachment 315922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315922&action=review

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:102
> +    def _should_log_linter_warning(warning, files, cwd, host):

Can we write unit test for this?

>> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:123
>> +    def lint_test_expectations(files, configuration, cwd, increment_error_count=lambda: 0, line_numbers=None, host=Host()):
> 
> Is it necessary to have default values for increment_error_count and host?

Can we write unit test for this?

[Jonathan Bedard]

Comment on attachment 315922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315922&action=review

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:95
>> +    def __init__(self, port, full_test_list, allow_rebaseline_modifier, shorten_filename=str):
> 
> How did you come to the decision to use str() as the default value for shorten_filename? Does this mean that we can never have Unicode characters in the names of layout tests? This default value does not match the default value of the same argument in the TestExpectationsModel constructor.

This was an attempt to remove the (lambda x: x) in line 103.  But you are correct.  str is not right here.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:971
>> +                    expectation.name if expectation.expectations else None)
> 
> As far as I know exploding function arguments such that there is one argument per line is not the style we use.

I believe our style guide is ambitious on the matter.  In some cases we break these calls up in our python (especially when the line would be very long, as in this case).  Even the previous line was split, although not one argument on each line.  I will mimic the pattern used previously.

>> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:102
>> +    def _should_log_linter_warning(warning, files, cwd, host):
> 
> Can we write unit test for this?

I don't think such a unit test would be very meaningful.  In order to test these functions, we would need to construct fake warnings, a fake file-line-number map and a host which was aware of the files which both those warnings and file-line-number-map came from.  Given that the error-prone parts of this process is creating those file-line-number maps, I feel like unit tests would result in brittle test code.

The better way to test these functions is implemented in main_unittest.py, where we create TestExpectations in a mock file system with the types of errors we expect to catch with these functions.  Errors in these functions will be caught with that testing code.

>>> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:123
>>> +    def lint_test_expectations(files, configuration, cwd, increment_error_count=lambda: 0, line_numbers=None, host=Host()):
>> 
>> Is it necessary to have default values for increment_error_count and host?
> 
> Can we write unit test for this?

Default values: Not at the moment, it is not necessary to have default values.  But since we can do something sane without these arguments, I think its worth defining defaults for them.

Unit tests: See above comment on _should_log_linter_warning.

>> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
>> +                    line_numbers)
> 
> See my remark above about exploding function arguments.

Here, it should definitely be collapsed.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 315946 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #23)
> Created attachment 315946 [details]
> Patch

This patch addresses Dan's comments, except for the unit tests and default arguments.

Note that the change Dan was commenting on has already landed.  I would still like to make these changes since 2 of them are fixing bugs.

[Daniel Bates]

Comment on attachment 315946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315946&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:718
> +        shortened_expectation_file = self._shorten_filename(expectation_line.filename)
> +        shortened_previous_expectation_file = self._shorten_filename(prev_expectation_line.filename)

file => filename

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:734
> -                self._shorten_filename(expectation_line.filename), expectation_line.line_number,
> -                self._shorten_filename(prev_expectation_line.filename), prev_expectation_line.line_number))
> +                shortened_previous_expectation_file, expectation_line.line_number,
> +                shortened_expectation_file, prev_expectation_line.line_number))

This is a behavior change. Is this intentional?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747
> +        if expectation_line.related_files.get(shortened_previous_expectation_file, None) is None:
> +            expectation_line.related_files[shortened_previous_expectation_file] = []
> +        expectation_line.related_files[shortened_previous_expectation_file].append(prev_expectation_line.line_number)

You did not make the change I suggested in comment #19. Did you disagree with it? If so, can you elaborate?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:751
> +        if prev_expectation_line.related_files.get(shortened_expectation_file, None) is None:
> +            prev_expectation_line.related_files[shortened_expectation_file] = []
> +        prev_expectation_line.related_files[shortened_expectation_file].append(expectation_line.line_number)

You did not make the change I suggested in comment #19. Did you disagree with it? If so, can you elaborate?

[Jonathan Bedard]

Comment on attachment 315946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315946&action=review

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:734
>> +                shortened_expectation_file, prev_expectation_line.line_number))
> 
> This is a behavior change. Is this intentional?

Good catch.  No, it is not intentional.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747
>> +        expectation_line.related_files[shortened_previous_expectation_file].append(prev_expectation_line.line_number)
> 
> You did not make the change I suggested in comment #19. Did you disagree with it? If so, can you elaborate?

I'm sorry, I didn't read comment #19 closely enough.

The catch with this code is that an entry in related_files can be none if the file in question was deleted. (see line 205 in Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py)  I did this so that this code would be robust and continue working even if you somehow find yourself in a case where you both have a line number for a file and you think the file does not exist.  In practice, this case should be impossible since a) you shouldn't be able to attribute a file line to a missing file and, b) as the code is currently structured, files will only be classified as missing if they are test files and will only have line numbers associated with errors if they are test expectations.

Since this is a question of style over robustness, I tend to err on the side of more robust code.  In this case, I don't feel strongly either way.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:751
>> +        prev_expectation_line.related_files[shortened_expectation_file].append(expectation_line.line_number)
> 
> You did not make the change I suggested in comment #19. Did you disagree with it? If so, can you elaborate?

See above comment.

[Jonathan Bedard]

Created attachment 315953 [details]
Patch

[Daniel Bates]

(In reply to Jonathan Bedard from comment #26)
> >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747
> >> +        expectation_line.related_files[shortened_previous_expectation_file].append(prev_expectation_line.line_number)
> > 
> > You did not make the change I suggested in comment #19. Did you disagree with it? If so, can you elaborate?
> 
> I'm sorry, I didn't read comment #19 closely enough.
> 
> The catch with this code is that an entry in related_files can be none if
> the file in question was deleted. (see line 205 in
> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py)  I did this
> so that this code would be robust and continue working even if you somehow
> find yourself in a case where you both have a line number for a file and you
> think the file does not exist.  In practice, this case should be impossible
> since a) you shouldn't be able to attribute a file line to a missing file
> and, b) as the code is currently structured, files will only be classified
> as missing if they are test files and will only have line numbers associated
> with errors if they are test expectations.
> 
> Since this is a question of style over robustness, I tend to err on the side
> of more robust code.  In this case, I don't feel strongly either way.
> 

Usually we use assert to ensure invariants. I am not very familiar with this code and hence did not check your patch for correctness. The fact that the related files dictionary can have keys that map to None does not seem intuitive and caught me and Kocsen off guard. When something is not obvious it is always better to make the code obvious and add asserts. If this is not straightforward then adding unit tests is a great way to capture such knowledge. Lastly code comments can help explain non-obvious or weird code contortions. I will leave the decision how to update this code (or not) to you.

[Jonathan Bedard]

Created attachment 315960 [details]
Patch for landing

[Jonathan Bedard]

(In reply to Daniel Bates from comment #28)
> (In reply to Jonathan Bedard from comment #26)
> > ...
> > 
> 
> Usually we use assert to ensure invariants. I am not very familiar with this
> code and hence did not check your patch for correctness. The fact that the
> related files dictionary can have keys that map to None does not seem
> intuitive and caught me and Kocsen off guard. When something is not obvious
> it is always better to make the code obvious and add asserts. If this is not
> straightforward then adding unit tests is a great way to capture such
> knowledge. Lastly code comments can help explain non-obvious or weird code
> contortions. I will leave the decision how to update this code (or not) to
> you.

Since it seems like my use of 'None' has caused confusion, I added a comment in both filereader.py and models/test_expectations.py. I also added an assert in models/test_expectations.py, to check that the case which I said should be impossible does not occur.

With these changes, your suggestion in comment #19 has also been integrated.

[WebKit Commit Bot]

Comment on attachment 315960 [details]
Patch for landing

Clearing flags on attachment: 315960

Committed r219669: <http://trac.webkit.org/changeset/219669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 315960 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=315960&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747
> +        expectation_line.related_files[shortened_previous_expectation_filename] = expectation_line.related_files.get(shortened_previous_expectation_filename, []).append(prev_expectation_line.line_number)

The way you wrote this is not faithful to the way I suggested writing his code in comment #19. That's fine as long as this code works. Does it? From the examples on <https://docs.python.org/2/tutorial/datastructures.html> it looks like list.append() returns None.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #33)
> Comment on attachment 315960 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315960&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:747
> > +        expectation_line.related_files[shortened_previous_expectation_filename] = expectation_line.related_files.get(shortened_previous_expectation_filename, []).append(prev_expectation_line.line_number)
> 
> The way you wrote this is not faithful to the way I suggested writing his
> code in comment #19. That's fine as long as this code works. Does it? From
> the examples on <https://docs.python.org/2/tutorial/datastructures.html> it
> looks like list.append() returns None.

You are correct, and this has revealed a weakness in the unit tests as well.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 315992 [details]
Patch

[Aakash Jain]

Comment on attachment 315992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315992&action=review

> Tools/Scripts/webkitpy/style/main_unittest.py:155
> +                '# TestExpectations\ncss1/test.html [ Failure ]\ncss1/test.html [ Failure ]\n'}

Nit: spacing seems off.

[Jonathan Bedard]

Committed r219689: <http://trac.webkit.org/changeset/219689>

[Manuel Rego Casasnovas]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 315922 [details]
> Patch
> 
> Clearing flags on attachment: 315922
> 
> Committed r219657: <http://trac.webkit.org/changeset/219657>

It seems this introduced an issue on the style checker.
See bug #174775 for details.