facebook / buck2

Build system, successor to Buck
https://buck2.build/
Apache License 2.0
3.49k stars 214 forks source link

feature: "top-level-types required" mode for `.bzl` files #651

Open thoughtpolice opened 3 months ago

thoughtpolice commented 3 months ago

I'm working on my own Prelude and occasionally I do this thing where I splat out some function, then run it with the wrong arguments later in the file. This is a time-honored tradition for those working in Python-derived languages.

Buck2 Starlark already has MyPy-style types, but what I'd like is to make sure types always have to be provided for all top-level .bzl code in my repository. That is, every single def in a .bzl should have all arguments typed, and a return type. Note that I consider typing.Any to be accetable, BTW — but only because it's much easier to spot and audit, and because having something for gradual migration is occasionally necessary. (This is coincidentally considered to be good practice or mandatory in most languages with inferred types, anyway.)

Does this kind of feature make sense? I'm sure there's something I can't anticipate; I know the typing support has undergone a lot of work in the past year, so it would be great if this were possible.

I would also accept a lint for this, I suppose.

thoughtpolice commented 3 months ago

An example I can't quite anticipate OTTOMH: what **kwargs typing typically looks like. There's probably some prior art with MyPy, I suppose.

stepancheg commented 3 months ago

Such enforcements probably belong to linters, rather than buck2 core.

Internally we generally don't use types in bzl files outside of buck2 prelude.

Could be a source-level hint recognized by buck2/starlark though, like this:

# my.bzl
# @starlark-rust require-types-in-top-level-defs

def foo(x): pass # fails
JakobDegen commented 3 months ago

Such enforcements probably belong to linters, rather than buck2 core.

How do you feel about adding this as an option to buck2 starlark lint?

thoughtpolice commented 3 months ago

Nit: it might also be nice if this enforced fields with types on providers and records, too?

stepancheg commented 3 months ago

@JakobDegen I'm not sure about linters. If this is supposed to be an error, then it should always be an error, not lint.

However, I have another possible idea to consider:

But I'm doubt about that, because:

Given that, an option for lint maybe not the worst option.

thoughtpolice commented 3 months ago

For a lint, I think I could just make it a hard error among all the other things the CI system checks. It's mostly a matter of whether you get an error in buck2 build or in CI, I suppose. An editor integration could smooth that out (show lints inline just like eval/parse errors.) So it may be the lesser of the various evils.

JakobDegen commented 3 months ago

@thoughtpolice feasibly, this is also the kind of thing that someone might prefer to not have block their local iteration, which would mean that warn-in-IDE and fail-in-CI would actually be the right balance