Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
thoughtpolice opened this issue May 15, 2024 · 7 comments
Open

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

thoughtpolice opened this issue May 15, 2024 · 7 comments

Comments

@thoughtpolice
Copy link
Contributor

thoughtpolice commented May 15, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

stepancheg commented May 15, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@stepancheg
Copy link
Contributor

@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:

  • make it starlark dialect option
  • configure starlark dialect options with buckconfig

But I'm doubt about that, because:

  • prelude may want different dialect options
  • generally other external cells may want different dialect options

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

@thoughtpolice
Copy link
Contributor Author

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
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants