BLD: add more static analysis by tacaswell · Pull Request #31661 · matplotlib/matplotlib

tacaswell · 2026-05-12T03:10:16Z

PR summary

This adds more static analysis to our CI / prek config.

run clang-tidy on c/c++/objectiveC code (and fix or ignore issues found). This found a couple of real issues and will only run on CI (+ a helper script to run it locally). The dependencies are too heavy to try and launder through prek's virtual environments.
run zizmor in prek (and hence automatically on CI). Had to explictily allow the 4 places we use pull_request_target and a couple of places we allow caching. Also enabled cooldown windows on our dependabot config. This also flagged we had a redundant checkout step in
enable shellcheck in prek to hit the handful of bash scripts we ship and applied the hardening it found.
~~run ruff on our checked in notebooks as well~~ Our exsiting ruff config hits this already
added static validation of svg files to prek (I got in my head that this could be used to get information out of developer's machines which is probably not possible (beyond what you give up by going to any website)), but now it will be harder for someone to sneak an active SVG into our baseline images.

Not reflected in the commits here, but I did look at https://github.com/PyCQA/bandit but every single thing it flagged was a false positive (either flagging things like exec in the plot directive or being too simplistic to notice that we had already mitigated the thing it was flagging in the code).

AI Disclosure

I started from the prompt

Analyze this repository and identify any programming languages, markup languages, document formats, build artifacts, generated files, embedded assets, or other non-core technologies that could be overlooked by traditional SAST or standard code-scanning tools.

Focus especially on:

languages or file types that are not part of the repository's primary tech stack
obscure, uncommon, legacy, or less frequently scanned formats
executable or scriptable content hidden in non-obvious places
document or graphics formats that may contain logic, macros, scripts, links, or embedded content
Include examples such as LaTeX, PostScript, SVG, and similar formats, but do not limit the analysis to those.

For each item you identify, provide:

the language, format, or artifact type
why it may be missed by classic SAST or scanning tools
the security relevance or potential risk
typical file extensions or repository locations where it may appear
whether it is likely source, generated output, embedded content, or supporting artifact

with opus 4.6 and then pushing on addressing the coverage with a mix of opus 4.6 and sonnet 4.6. I reviewed everything as I committed it and took a very heavy editing pass at the initial version of the documentation.

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

github-advanced-security · 2026-05-12T03:10:41Z

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

QuLogic · 2026-05-12T05:23:59Z

@@ -0,0 +1,246 @@
+#!/usr/bin/env python3
+"""
+Run clang-tidy on Matplotlib's C/C++ and Objective-C sources.


I'm not sure this script is fully necessary; meson will create a clang-tidy target for you (but only if you have .clang-tidy in the root), so you just need to move it, make an editable install, and run ninja -C build/cp313 clang-tidy or similar

I think it is still useful for filtering out warnings from the Agg source, but this can indeed be greatly simplified.

I simplified this, still think it is useful to have to automate creating a build directory just for clang-tidy and for filtering the output.

These were false positives, but add code to give the analyzer a hint + asserts in a debugging build rather than NOLINT to be defensive. Aided by 🤖

When for no-change to code with the qhull wrapper rather than adjusting the code. Aided by 🤖

Fixes an issue found by clang-tidy Aided by 🤖

Simplified at Elliott's suggestions Aided by 🤖

Aided by 🤖

Added exceptions for things it thinks are questionable but we are OK with doing. Aided by 🤖

Aided by 🤖

tacaswell · 2026-05-13T14:36:24Z

This runs zizimor twice (once in its own job, once in the prek job). I think that is OK, particularly because its own job also has a cron attached to it, but dropping the action in favor of just the prek invocation would also be fine.

QuLogic · 2026-05-14T19:12:12Z

This runs zizimor twice (once in its own job, once in the prek job). I think that is OK, particularly because its own job also has a cron attached to it, but dropping the action in favor of just the prek invocation would also be fine.

The separate job also has a token and should be able to run more checks, I think?

QuLogic · 2026-05-16T07:23:54Z

+            if current:
+                groups.append(current)
+            current = [line]
+        else:
+            current.append(line)
+
+    if current:
+        groups.append(current)


You can just append when you create it.

Suggested change

if current:

groups.append(current)

current = [line]

else:

current.append(line)

if current:

groups.append(current)

current = [line]

groups.append(current)

else:

current.append(line)

QuLogic · 2026-05-16T07:27:24Z

+    hooks:
+      - id: shellcheck
+  - repo: https://github.com/zizmorcore/zizmor-pre-commit
+    rev: a4727cbbcd26d7098e96b9cb738169b59711ae51  # frozen: v1.24.1


Suggested change

rev: a4727cbbcd26d7098e96b9cb738169b59711ae51 # frozen: v1.24.1

rev: 04a94ec45c8e92de281e3cc55754eb6d9db526a1 # frozen: v1.25.1

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

greglucas

Honestly not very many issues, most very minor ignores, so that is good to see!

Overall looks good to me and definitely something good to have in CI checks as well. I'm not sure if there are any comments @QuLogic is still waiting on so I'll hold off merging, but feel free to self-merge if you think you've already addressed everything.

greglucas · 2026-06-03T02:45:40Z

+      # These workflows use pull_request_target solely to obtain write access
+      # for API operations (labeling, commenting) on fork PRs. None of them
+      # check out or execute any PR-supplied code.


I wonder if it is possible for zizmor to identify that we don't use actions/checkout within the workflows?

It would be nice to prove that no one changes these or we aren't doing this on accident. There are just so many vulnerabilities with the pull_request_target issues these days...

…661-on-v3.11.x Backport PR #31661 on branch v3.11.x (BLD: add more static analysis)

github-actions Bot added GUI: MacOSX topic: path handling topic: text/fonts topic: contour Documentation: devdocs files in doc/devel labels May 12, 2026

QuLogic reviewed May 12, 2026

View reviewed changes

tacaswell added 11 commits May 12, 2026 16:18

STY: add shellcheck

d053d95

MNT: fix warnings identified by clang-tidy

76e84f7

These were false positives, but add code to give the analyzer a hint + asserts in a debugging build rather than NOLINT to be defensive. Aided by 🤖

BLD: add NOLINT directives to _qhull_wrapper

b8fb1b4

When for no-change to code with the qhull wrapper rather than adjusting the code. Aided by 🤖

MNT: ensure that variables are always initialized

99ccdca

Fixes an issue found by clang-tidy Aided by 🤖

BLD: skip two more clang-tidy warnings in _tri.cpp

419087b

BLD: add tooling to run clang-tidy on c/c++ code

4b3ce4d

Simplified at Elliott's suggestions Aided by 🤖

CI: add cool-down periods on dependabot

5449bea

CI: remove redundancy and leaked credentials in workflow

edef1d8

Aided by 🤖

BLD: add zizmor to prek config

628a5f5

Added exceptions for things it thinks are questionable but we are OK with doing. Aided by 🤖

CI: add zizmor action to keep us in line

75b3192

Aided by 🤖

CI: add static validation of svg

050c2e8

Aided by 🤖

tacaswell force-pushed the bld/static_analysis branch from 1c966c2 to 050c2e8 Compare May 12, 2026 20:19

github-actions Bot added the topic: tri label May 12, 2026

tacaswell added 2 commits May 12, 2026 20:16

CI: ensure clang-tidy on PATH

0420bc4

CI: try to use the right compilers

dc46af5

tacaswell added this to the v3.12.0 milestone May 13, 2026

ksunden approved these changes May 14, 2026

View reviewed changes

DOC: clearify comment as to why only cygwin needs this config

6ff08e9

QuLogic reviewed May 15, 2026

View reviewed changes

Comment thread src/tri/_tri.cpp

QuLogic reviewed May 16, 2026

View reviewed changes

CI: bump and simplify zizmor config

f385c10

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

DOC: fix section heading markup

b691a33

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

tacaswell commented May 22, 2026

View reviewed changes

Comment thread doc/devel/coding_guide.rst Outdated

DOC: add missing dep to run clang-tidy

98a8893

QuLogic requested a review from greglucas May 26, 2026 06:06

QuLogic mentioned this pull request May 27, 2026

[Bug]: macOS backend leaks NSTrackingArea #31754

Closed

QuLogic linked an issue May 27, 2026 that may be closed by this pull request

[Bug]: macOS backend leaks NSTrackingArea #31754

Closed

iccir mentioned this pull request May 28, 2026

Fix several MacOS memory management bugs #31769

Merged

1 task

greglucas approved these changes Jun 3, 2026

View reviewed changes

greglucas reviewed Jun 3, 2026

View reviewed changes

QuLogic modified the milestones: v3.12.0, v3.11.0 Jun 4, 2026

QuLogic merged commit b825c6d into matplotlib:main Jun 4, 2026
42 of 44 checks passed

meeseeksmachine mentioned this pull request Jun 4, 2026

Backport PR #31661 on branch v3.11.x (BLD: add more static analysis) #31816

Merged

tacaswell deleted the bld/static_analysis branch June 4, 2026 14:12

QuLogic added a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 4, 2026

Backport PR matplotlib#31661: BLD: add more static analysis

cfeef53

QuLogic added a commit that referenced this pull request Jun 4, 2026

Merge pull request #31816 from meeseeksmachine/auto-backport-of-pr-31…

70a0d27

…661-on-v3.11.x Backport PR #31661 on branch v3.11.x (BLD: add more static analysis)

	rev: a4727cbbcd26d7098e96b9cb738169b59711ae51 # frozen: v1.24.1
	rev: 04a94ec45c8e92de281e3cc55754eb6d9db526a1 # frozen: v1.25.1

Uh oh!

Conversation

tacaswell commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

AI Disclosure

PR checklist

Uh oh!

github-advanced-security AI commented May 12, 2026

What Enabling Code Scanning Means:

Uh oh!

QuLogic May 12, 2026

Choose a reason for hiding this comment

Uh oh!

tacaswell May 12, 2026

Choose a reason for hiding this comment

Uh oh!

tacaswell May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tacaswell commented May 13, 2026

Uh oh!

QuLogic commented May 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QuLogic May 16, 2026

Choose a reason for hiding this comment

Uh oh!

QuLogic May 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

greglucas Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tacaswell commented May 12, 2026 •

edited

Loading