Upgrade cpplint to external upstream target

[Aiden2244]

See this comment for context. New upstream of cpplint causes existing codebase to trigger 1666 cpplint errors.

---

This change is 

[jwnimmer-tri]

(1) For the indent_namespace, I think it's fine to turn it off. It is not very useful, and seems to have lots of false positives:

--- a/CPPLINT.cfg
+++ b/CPPLINT.cfg
@@ -39,6 +39,10 @@ filter=-build/include
 # by the readability/todo rule, which we leave enabled.)
 filter=-whitespace/todo
 
+# TODO(jwnimmer-tri) This has hundreds of false positives. Seems completely
+# broken. With our growing use of clang-format, we probably don't care.
+filter=-whitespace/indent_namespace
+
 # TODO(#1805) Fix this.
 filter=-legal/copyright
 

(2) For errors such as these ...

==================== Test output for //systems/lcm:py/lcm_buses_cpplint:
systems/lcm/lcm_buses.cc:39:  Add #include <vector> for vector<>  [build/include_what_you_use] [4]
systems/lcm/lcm_buses.cc:47:  Add #include <string> for string  [build/include_what_you_use] [4]

... the problem seems to be that the tool is failing to identify the "associated header" of lcm_buses.h for the file lcm_buses.cc. If the include already appears in the associated header, it's not supposed to be required in the cc file.

In the past, I've seen this when file paths or cpplint configuration are incorrect. Possibly the switch to the new tool broke the file grouping. The FilesBelongToSameModule function is supposed to match up the cc file with its header. If it's not working, a lot of the lint rules will not operate correctly, so I would start by fixing that first.

[jwnimmer-tri]

Also FYI, we might be able to remove the unittest from our build (and thus, no venv changes required). The reason we used to be running the test in our CI was so that our fork at https://github.com/RobotLocomotion/styleguide had test coverage. If we end up using an unmodified upstream cpplint, then we should remove the unittest from our CI. The upstream CI on its own will be fine.

[tyler-yankee]

Reviewed 3 of 22 files at r1, 3 of 6 files at r2, 1 of 3 files at r4, 17 of 24 files at r6, all commit messages.
Reviewable status: 11 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

        name = name,
        repository = "cpplint/cpplint",
        commit = "80da3c1ef37af017715dc1366fbda8263179bdeb",

Could you add a comment here documenting what version of cpplint this is? (Tag, date?)

---

CPPLINT.cfg line 51 at r6 (raw file):

# TODO(#1805) Fix this.
filter=-legal/copyright

nit Remove this commented-out block before merging. Or is it worth keeping around for history, @jwnimmer-tri?

---

setup/python/pdm.lock line 0 at r6 (raw file):
Are all of these package version changes in the lockfile necessitated by the linter change? It seems like a lot ...

[Aiden2244]

Reviewable status: 11 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

setup/python/pdm.lock line at r6 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Are all of these package version changes in the lockfile necessitated by the linter change? It seems like a lot ...

I just pushed some changes that restored this file to the version currently on the master branch, this should resolve the issue.

[Aiden2244]

Dismissed @tyler-yankee from a discussion.
Reviewable status: 11 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Could you add a comment here documenting what version of cpplint this is? (Tag, date?)

Done. I added this line: # cpplint version targeted: v2.0.0-4-g80da3c1 03-06-2025. Lmk if this is what you had in mind!

[tyler-yankee]

Reviewed 1 of 2 files at r8, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

Previously, Aiden2244 (Aiden McCormack) wrote…

Done. I added this line: # cpplint version targeted: v2.0.0-4-g80da3c1 03-06-2025. Lmk if this is what you had in mind!

Yes, this works. I am wondering @jwnimmer-tri's thoughts on how we should keep this up-to-date moving forward.

[Aiden2244]

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Yes, this works. I am wondering @jwnimmer-tri's thoughts on how we should keep this up-to-date moving forward.

That's a good question. Future upgrades of cpplint will also require regenerating the patchfile, which unfortunately makes this process fairly nontrivial.

[jwnimmer-tri]

Reviewed 19 of 24 files at r6, 1 of 2 files at r8, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

a discussion (no related file):

Previously, Aiden2244 (Aiden McCormack) wrote…

I noticed a similar parsing issue here as well: // NOLINTNEXTLINE (runtime/references) Per hash\_append convention. does not trigger any errors but // NOLINTNEXTLINE(runtime/references) Per hash\_append convention. does not get picked up by the new cpplint target...

Ahh, I finally understood this. Saying // NOLINT (thing) or // NOLINTNEXTLINE (thing) with the space there is the same as saying // NOLINT or // NOLINENEXTLINE with no category. It's disabling all linting of the next line; the parenthetical is being silently ignored. This was all a red herring.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This warning has always been enabled, but it seems like the upgraded tool does a better job of finding everything.

We should adjust the code to pass the linter.

This one might be done now? WDYT?

---

CPPLINT.cfg line 51 at r6 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

nit Remove this commented-out block before merging. Or is it worth keeping around for history, @jwnimmer-tri?

We have a policy of never committing commented-out code, so yes this commented-out code will need to be removed before this can land.

---

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

Previously, Aiden2244 (Aiden McCormack) wrote…

That's a good question. Future upgrades of cpplint will also require regenerating the patchfile, which unfortunately makes this process fairly nontrivial.

For externals where we are pointing to a git sha instead of a release tag, the following applies:

(1) We only allow/use git shas that refer to the project's default branch (in this case, "develop"), which this commit does.

(2) The commit sha is the only fact we write down about the pin we're using. The hand-written "cpplint version" comment does not add new information, yet is a pain to maintain, so should be removed.

(3) The tools/workspace/new_release program will remind us to bump the sha every month when we run it, and make the edits here for us (commit & sha256).

(4) If our patch(es) have conflict(s), then the person running new_release either fixes the conflict by adjusting the patch, or else can ask for help.

---

common/hwy_dynamic_impl.h line 12 at r8 (raw file):

// and we always want its code to be private to that file, so we'll use an
// anonymous namespace in a header file, for simplicity.
namespace {  // NOLINT (build/namespaces)

nit Don't add whitespace. Fix the category name instead.

---

examples/pendulum/pendulum_parameters_derivatives.cc line 50 at r8 (raw file):

      PendulumPlant<AutoDiffXd>::get_state(*derivatives);

  // NOLINTNEXTLINE (build/namespaces)  Usage documented by fmt library.

nit Don't add whitespace. Fix the category name instead.

---

bindings/pydrake/common/text_logging_pybind.cc line 67 at r8 (raw file):

    }

    // NOLINTNEXTLINE (build/namespaces) This is how pybind11 wants it.

nit Don't add whitespace. Fix the category name instead.

---

common/overloaded.h line 30 at r8 (raw file):

// We put this in the anonymous namespace to ensure that it will not generate
// linkable symbols for every specialization in every library that uses it.
namespace {  // NOLINT (build/namespaces)

nit Don't add whitespace. Fix the category name instead.

[jwnimmer-tri]

Reviewed 1 of 24 files at r6, 1 of 2 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(Not sure, I'll need to look more carefully, later.)

The bottom line is that cpplint/cpplint#277 broke cpplint by withdrawing support for the stanza of the Looping and Branching part of the guide near the text "For historical reasons, we allow one exception to the above rules:". Instead, they just added a TODO and ran away. You can see it in the change to the test expectations where valid code is now being flagged: https://github.com/cpplint/cpplint/pull/277/files#diff-6dc7f5dae41f93b0141520d6aa8b1e66e53984788a6d1c5b4e5c305db88b7c92.

For now, let's add a Drake patch to give that particular case a distinct name and disable it in our CPPLINT.cfg, something like this:

--- a/cpplint.py
+++ b/cpplint.py
@@ -355,6 +355,7 @@ _ERROR_CATEGORIES = [
     'whitespace/indent_namespace',
     'whitespace/line_length',
     'whitespace/newline',
+    'whitespace/newline_controlled_statement',
     'whitespace/operators',
     'whitespace/parens',
     'whitespace/semicolon',
@@ -4390,13 +4391,13 @@ def CheckBraces(filename, clean_lines, linenum, error):
   if keyword := re.search(
       r'\b(else if|if|while|for|switch)'  # These have parens
       r'\s*\(.*\)\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\};]', line):
-    error(filename, linenum, 'whitespace/newline', 5,
+    error(filename, linenum, 'whitespace/newline_controlled_statement', 5,
           f'Controlled statements inside brackets of {keyword.group(1)} clause'
           ' should be on a separate line')
   elif keyword := re.search(
       r'\b(else|do|try)'  # These don't have parens
       r'\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\}]', line):
-    error(filename, linenum, 'whitespace/newline', 5,
+    error(filename, linenum, 'whitespace/newline_controlled_statement', 5,
           f'Controlled statements inside brackets of {keyword.group(1)} clause'
           ' should be on a separate line')
 

[jwnimmer-tri]

Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The bottom line is that cpplint/cpplint#277 broke cpplint by withdrawing support for the stanza of the Looping and Branching part of the guide near the text "For historical reasons, we allow one exception to the above rules:". Instead, they just added a TODO and ran away. You can see it in the change to the test expectations where valid code is now being flagged: https://github.com/cpplint/cpplint/pull/277/files#diff-6dc7f5dae41f93b0141520d6aa8b1e66e53984788a6d1c5b4e5c305db88b7c92.

For now, let's add a Drake patch to give that particular case a distinct name and disable it in our CPPLINT.cfg, something like this:

--- a/cpplint.py
+++ b/cpplint.py
@@ -355,6 +355,7 @@ _ERROR_CATEGORIES = [
     'whitespace/indent_namespace',
     'whitespace/line_length',
     'whitespace/newline',
+    'whitespace/newline_controlled_statement',
     'whitespace/operators',
     'whitespace/parens',
     'whitespace/semicolon',
@@ -4390,13 +4391,13 @@ def CheckBraces(filename, clean_lines, linenum, error):
   if keyword := re.search(
       r'\b(else if|if|while|for|switch)'  # These have parens
       r'\s*\(.*\)\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\};]', line):
-    error(filename, linenum, 'whitespace/newline', 5,
+    error(filename, linenum, 'whitespace/newline_controlled_statement', 5,
           f'Controlled statements inside brackets of {keyword.group(1)} clause'
           ' should be on a separate line')
   elif keyword := re.search(
       r'\b(else|do|try)'  # These don't have parens
       r'\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\}]', line):
-    error(filename, linenum, 'whitespace/newline', 5,
+    error(filename, linenum, 'whitespace/newline_controlled_statement', 5,
           f'Controlled statements inside brackets of {keyword.group(1)} clause'
           ' should be on a separate line')
 

And actually, since the new category is a made-up name, we should probably have our patch add it to _DEFAULT_FILTERS to disable it. We don't want to put made-up names into CPPLINT.cfg as that might be confusing / incompatible with our tools reading it.

[Aiden2244]

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ahh, I finally understood this. Saying // NOLINT (thing) or // NOLINTNEXTLINE (thing) with the space there is the same as saying // NOLINT or // NOLINENEXTLINE with no category. It's disabling all linting of the next line; the parenthetical is being silently ignored. This was all a red herring.

Good catch. That's really tricky but makes total sense.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This one might be done now? WDYT?

I believe so! At this point, the only errors I am getting locally are the ones pertaining to whitespace, so I think the style changes resolved this issue.

---

tools/workspace/cpplint_internal/repository.bzl line 9 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For externals where we are pointing to a git sha instead of a release tag, the following applies:

(1) We only allow/use git shas that refer to the project's default branch (in this case, "develop"), which this commit does.

(2) The commit sha is the only fact we write down about the pin we're using. The hand-written "cpplint version" comment does not add new information, yet is a pain to maintain, so should be removed.

(3) The tools/workspace/new_release program will remind us to bump the sha every month when we run it, and make the edits here for us (commit & sha256).

(4) If our patch(es) have conflict(s), then the person running new_release either fixes the conflict by adjusting the patch, or else can ask for help.

Copy that. I'll make that adjustment.

[tyler-yankee]

CPPLINT.cfg line 16 at r10 (raw file):

# Unlike Google, drake embraces <filesystem>.
filter=-build/c++17

I may have missed this during the process - what is this for?

[jwnimmer-tri]

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

CPPLINT.cfg line 16 at r10 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I may have missed this during the process - what is this for?

To match the pink strikethrough here:
https://drake.mit.edu/styleguide/cppguide.html#Other_Features

We could add that link to the comment paragraph, maybe?

---

tools/workspace/cpplint_internal/patches/whitespace_newline.patch line 9 at r10 (raw file):

The fix basically adds a "made-up" error category and diverts the
expressions that trigger the aforementioned cases out of the more 

nit Remove trailing whitespace at the end of this line.

Ditto on line 12 (the last one of the paragraph).

[tyler-yankee]

CPPLINT.cfg line 16 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To match the pink strikethrough here:
https://drake.mit.edu/styleguide/cppguide.html#Other_Features

We could add that link to the comment paragraph, maybe?

Gotcha - it might be useful to add that comment in, though it's also something I wouldn't be familiar with where Drake developers might already be.

[jwnimmer-tri]

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 9 at r11 (raw file):

       continue
 
-    _re_patterns = []

nit Why are we removing this part? If I remove this section of the patch, I don't see any failures.

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 27 at r11 (raw file):

     # The following function is just a speed up, no semantics are changed.
-    if '<' not in line:  # Reduces the cpu time usage by skipping lines.
+    if not '<' in line:  # Reduces the cpu time usage by skipping lines.

nit Zap this unrelated change. It has no upside, yet the downside of hazarding merge conflicts.

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 71 at r11 (raw file):

+  # not having the .h file means there isn't one.
+  if filename.endswith('.cc') and not header_found:
+    return

I'm pretty sure that this return-statement is disabling all IWYU linting on cc files. The find-related-header search seems broken, so header_found is always false (except for common/timer.cc, which has a relative-path typo I'll need to fix), so all cc files are skipped for IWYU entirely.

A good idea when playing with linters is to check for both true and false positives -- the CI will find false positives (what we've been dealing with so far), but locally you should introduce a mistake in a cc file (by deleting a line that's supposed to be there) and run the linter tests to make sure they catch it.

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 81 at r11 (raw file):

-                     and (header_stripped[1:] + '.h') in include_dict)):
-      error(filename, required[header][0],
+  for required_header_unstripped in required:

nit I'm not sure why we're changing this part (renaming header to required_header_unstripped)?

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 93 at r11 (raw file):

 
 
+

nit Zap unnecessary whitespace change from the patch file. It has no benefit and just cause unnecessary merge conflicts.

---

tools/workspace/cpplint_internal/patches/transitive_includes.patch line 96 at r11 (raw file):

 def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error):
   """Check that make_pair's template arguments are deduced.
 

nit Text files should always end with a newline. Here, the last character in the file is the period, not an newline. Note that we don't need a blank line (two newline characters), we just need one newline character.

[tyler-yankee]

pending outstanding comments.

Reviewed 2 of 3 files at r9, 4 of 4 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)