Helper to make linters by lilatomic · Pull Request #16412 · pantsbuild/pants

lilatomic · 2022-08-04T04:09:43Z

I was looking at one of my old projects. It was from before I discovered Pants, so it uses Tox. I was reminded that it is incredibly easy to just run a python package in tox:

[testenv:radon]
deps = radon
commands = 
    radon cc -s --total-average --no-assert -nb src/

I thought it would be great if it were that easy to create a linter in Pants. So I hacked together this prototype!
now it's as simple as

def rules():
    class VultureTool(MetalintTool):
        options_scope = "vulture"
        name = "Vulture"
        help = """Vulture finds unused code in Python programs"""

        default_version = "vulture==2.5"
        default_main = ConsoleScript("vulture")

        args = ArgsListOption(example="--min-confidence 95")

    def vulture_args(tool: VultureTool, files: Tuple[str, ...]):
        return tool.args + files

    vulture = make_linter(VultureTool, "vulture", vulture_args)

    return [
        *vulture.rules(),
    ]

This MR follows (reasonably closely) the Add a linter common task walkthrough.

Initial discussion questions:

Do we want this type of helper in Pants?
Is this type of helper (a function that generates the rules) the type of helper we want?
What features would we need before helpers like this are mergeable?

Understanding this MR

register.py contains 3 example linters. 2 of them share the same tool, which produces some surprising results but actually works.
MetalintTool extends PythonToolBase with the fields that make the expected actions (in particular, export and generate-lockfiles) go. This class is 1 part of the basic interface of Metalint. The other component is make_linter, which creates all the classes and the rules and shoves them all together into the Metalint DTO.

Some of the helpers could also be useful on their own. For example, make_export_rules can dynamically generate the boilerplate for the export task.

What is even happening

MetalintTool adds all the fields to make things go.
Metalint is a DTO, convenient for gathering all the rules
make_linter is the real function here. It accepts a MetalintTool class and an argv_maker (which enables the caller to format the invocation arguments as required. This allows passing the right flags and formatting all the filenames, if needed). It optionally accepts the FieldSet and LintTargetsRequest, as escape hatches. IDK, until Mypy ruined the fun it was an easy include.
make_export_rules and make_lockfile_rules use a cache to ensure that a tool used in multiple linters only has 1 set of rules generated for it. (It looks like Pants deduplicates rules if they are equal by identity.)
Otherwise it's fairly standard dynamic class generation. The classes themselves are not equal by identity because they are created as variables inside the function, so each invocation of the function creates a new class. It uses a function instead of a class because the classes have dependencies on each other; but all the statements in a class have to be mostly valid on their own, so they can't depend on each other like this (at least AFAIK).

Known limitations and future work

tests
lockfile generation is a lot of a mess, at least internally. The problem is that the lockfile code assumes that we're shipping a lockfile. Which we aren't, since someone is dynamically generating the idea of this lockfile. It still works, although you have to generate lockfiles first.
does not allow post-processing the result of running the tool. Some tools won't fail your code, but you might want to induce a failure depending on the output. For example, Radon's Maintainability Index: you might want to fail if this is too low. I include this here because inserting a postprocess step might be really easy.
only works with Linters. I think iformatters would be necessary. Ideally we'd also have run targets, although that's maybe less necessary with macros and the experimental_shell_command
only works with python tools. At a minimum it should work with a binary that the user has on their system. Ideally we'd also build helpers to fetch default_known_versions from popular places, like github releases.
typing is a bit of a mess, if anyone knows how to get mypy to let you use an arg of type Type[T] as a valid type annotation LMK.

thejcannon · 2022-08-04T13:57:17Z

Without diving into the details too much, I like the idea of "if your tool fits this description, you can use this turnkey API". Not all tools will fit, but certainly some would (even ones we have internally). Permit me some time to stew on it more, but I think the overall is a ➕ from me.

sureshjoshi · 2022-08-15T12:43:13Z

Without digging into the code too much (details are less important to me right now), this is a +1 from me.

I'm a fan of making plugins/APIs easy, even at the expense of technical overhead. Something of this ilk would definitely strip out a lot of code from the codebase, possibly at some extra config expense.

I like the idea that these linters/formatters are more about their subsystems, rather than a decent chunk of copied/pasted rule code. Maybe from a global meta-class, maybe from a per-backend meta-linter. And if the meta-class wouldn't work well - then you fall back to exactly how it is today anyways.

Alternatively, maybe some of this functionality could come in the form of additional composable rules/rule_helpers? Haven't thought this through yet.

As I see it, most of the linters/formatters that I use or have worked on come down to:

Optional Skip
Optionally partition
Get source files (if they didn't come in with the fmtrequest/field_set for some reason)
Optionally get config files
Optionally get other resources/files
Bundle digests
Run (maybe custom) process with custom args
Return Lint/Fmt results

Most of that feels boilerplate-y since the inputs and outputs are pretty well-defined... Being able to specify those in the Fmt/Lint subsystem seems perfectly reasonable.

Some considerations:

Some linters/formatters use a custom process (NpxProcess, TerraformProcess, etc)
Not all subsystem tools are pex files, so there might be a generalized approach to return a hydrated tool (or maybe that's specified in the Subsystem file, as it kinda is today)

In general, I'd love to see and use this in mainline!

@lilatomic In the existing repo, do we have non-anecdotal numbers on how much boilerplate something like this would save? Or some concept of which tools it would work out of the box for, against how many would have to go custom anyways?

Edit: Forgot to mention, there probably even is some world where the rules_integration_test could be made declarative, assuming a common set of pass/fail tests for each new subsystem - where for testing, you provide example pass/fail files. Rules for these are also VERY boilerplate-y.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

since the tool is most likely the most complicated aspect of this, it makes sense to have people fill in the options themselves. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

we'll want to be able to have users construct these themselves, so they can include their tool's arguments as configured # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

I'm not sure this is necessary # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

currently only works if the tool is created once # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

since the rules created are equal by identity, Pants deduplicates them # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

It's pretty gross, TBH. I couldn't find a good way to just turn it on without providing an actual lockfile. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

also not be copypaste # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

lilatomic · 2022-08-18T01:33:45Z

So, doing a bit of an overview of the applicability for boilerplate reduction:
generating lockfile (searched GeneratePythonLockfile):

19 are pure boilerplate (return GeneratePythonLockfile.from_tool(..., use_pex=python_setup.generate_lockfiles_with_pex))
6 are slightly advanced boilerplate (a conditional for using a custom lockfile, which maybe everything should support)
5 are complex (eg mypy)

generating export (searched ExportPythonTool):

6 are pure boilerplate
3 are fetch interpreter constraints. I'm not sure if this is a different kind of boilerplate
3 are more complicated (eg mypy)

assessing the applicability of the main generated lint rule (-> LintResults) is harder, I've found at least 3 which look mostly boilerplate. I haven't looked at which formatters would be suitable, since metalint doesn't do that yet.

lilatomic · 2022-08-18T01:34:11Z

Relating to integration testing, that's really interesting. I wonder if the real boilerplate is in the tests, rather than the code. Obviously, any tool defined through metalint has that boilerplate tested. But if it can't be defined by metalint, we could at least run boilerplate tests. The example that comes to mind is that the "skip" flag is honoured. (Although, perhaps that's an indication that we need more rule helpers or higher-order rules to handle that behaviour).

On the subject of non-python-tools, I think there are basically 4 types of tools that we could easily serve with this:

Python tools: I think this is a good direction to start with because that's Pant's focus and it helps with parity with existing tools like Tox
Tools that Just Exist on the system: the all-important access hatch. Maybe it's installed from a different package manager (like grep from the OS package manager). Or maybe they're porting things over and will upgrade eventually. Or maybe they're downloadable but require some sort of installation.
Tools we can easily download: ExternalTool or TemplatedExternalTool
docker tools? Lots of tools come with a docker container. I don't think Pants really has the facility for running containerised tools, but if it did, there would just be a lot of boilerplate since the tools is completely installed

thejcannon · 2022-09-17T00:23:39Z

src/python/pants/backend/python/helpers/metalint.py

+    @rule(level=LogLevel.DEBUG)
+    def metalint_export(
+        _: MetalintExportSentinel,
+        tool: python_tool,  # type: ignore[valid-type]  # python_tool: Type[MetalintTool]


Does this work? I'm trying something similar and the rule engine is mad at me

lilatomic · 2025-03-12T14:51:39Z

I think the existing boilerplate reductions are good enough to close this

lilatomic added 22 commits August 17, 2022 19:49

slap together "working" prototype

b1bc88a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

extract helper to make rules

470b20a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

extract argv as param

0cba56a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

extract linter_name as param

1c04e43

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fix binary name and add second tool

e3e3551

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

extract version_spec as param

5e7619a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

add tool which uses different package

c52afe9

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

less bad names

4c87f85

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

helpers for simple argv scenarios

5ca0759

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

extractable default args

047a188

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

make other classes injectable

6289d21

I'm not sure this is necessary # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

pull interpreter constraints up

d3c9322

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

add export support

7074e62

currently only works if the tool is created once # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

export support supports duplicate tools

17f01a7

since the rules created are equal by identity, Pants deduplicates them # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

add skip ability

2bfa92a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

add loading of config files

17af18a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

mypy shenanigans

7385ad4

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

add and register demos for vulture and radon

fd8ebaa

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

change default discovery to pull in pyproject by default

171b21c

also not be copypaste # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

lilatomic added 2 commits August 18, 2022 00:09

push UnionRule construction into helpers to make them more helpful

7ad2447

use metalint helpers in clangformat

603ce67

lilatomic force-pushed the feature/metalint branch from 73ceaf6 to 603ce67 Compare August 18, 2022 04:14

thejcannon reviewed Sep 17, 2022

View reviewed changes

thejcannon mentioned this pull request Sep 21, 2022

Support overriding param types for rule code #16929

Merged

lilatomic mentioned this pull request Nov 7, 2022

Helpers to generate python lockfile rules #17480

Merged

3 tasks

gauthamnair mentioned this pull request Oct 14, 2023

Allow using a shell command/adhoc tool as part of any goal (check, lint, fmt, fix, deploy, ...) #17729

Open

gauthamnair mentioned this pull request May 7, 2024

Tool boilerplate reduction #20857

Open

lilatomic closed this Mar 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Helper to make linters#16412

Helper to make linters#16412
lilatomic wants to merge 24 commits intopantsbuild:mainfrom
lilatomic:feature/metalint

lilatomic commented Aug 4, 2022 •

edited

Loading

Uh oh!

thejcannon commented Aug 4, 2022

Uh oh!

sureshjoshi commented Aug 15, 2022 •

edited

Loading

Uh oh!

lilatomic commented Aug 18, 2022

Uh oh!

lilatomic commented Aug 18, 2022 •

edited

Loading

Uh oh!

thejcannon Sep 17, 2022

Uh oh!

lilatomic commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lilatomic commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Understanding this MR

What is even happening

Known limitations and future work

Uh oh!

thejcannon commented Aug 4, 2022

Uh oh!

sureshjoshi commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilatomic commented Aug 18, 2022

Uh oh!

lilatomic commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thejcannon Sep 17, 2022

Choose a reason for hiding this comment

Uh oh!

lilatomic commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lilatomic commented Aug 4, 2022 •

edited

Loading

sureshjoshi commented Aug 15, 2022 •

edited

Loading

lilatomic commented Aug 18, 2022 •

edited

Loading