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

Add attr.struct_list() #10414

Closed
cvrebert opened this issue Dec 14, 2019 · 4 comments
Closed

Add attr.struct_list() #10414

cvrebert opened this issue Dec 14, 2019 · 4 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: feature request

Comments

@cvrebert
Copy link
Contributor

Description of the problem / feature request:

The attr module should have a struct_list() function to allow the definition of attributes whose values are lists of struct()s.

Feature requests: what underlying problem are you trying to solve with this feature?

I have Starlark macros/rules which take structured information as inputs. Due to lack of this feature, if I want to keep the information bundled together in my rule implementation, I'm forced to apply ugly hacks like encoding the struct() as a string. Even worse, I can't easily use JSON for this encoding due to #3732 , so I'm forced to either reimplement JSON parsing myself, or implement a custom encoding scheme myself.

Have you found anything relevant by searching the web?

Nope.

  • StackOverflow: No results.
  • GitHub issues: No relevant results.
  • bazel-discuss: No relevant results.

Any other information, logs, or outputs that you want to share?

Happy to point you to concrete, Google-internal use-cases for this.

@laurentlb
Copy link
Contributor

Allowing arbitrary values (inside the struct fields) might be problematic for Bazel, especially if they may contain labels.

It's hard to suggest alternatives without at the details, but you can sometimes use a list of labels. Each target in the list uses a rule that can wrap the information you need.

@Bocete
Copy link
Contributor

Bocete commented Dec 17, 2019

If all contents of the struct attribute are ignored apart from piping the struct through (i.e. this is not a dependency attribute no matter what's inside it), is it still problematic?

@laurentlb laurentlb added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: feature request and removed untriaged labels Mar 6, 2020
@brandjon
Copy link
Member

Please see this explanation of why we are loathe to extend the set of allowed attribute types.

@cvrebert
Copy link
Contributor Author

Adding a new type has implications for many different systems. The whole of the loading phase
needs to know about the type --

  • how to serialize it,

Sure. The advantage of generic structs is only having to do this once, versus once per "kind" of struct, or per-class (if Starlark hypothetically had classes).

  • how to format it for bazel query,

Straightforward enough. struct(x=y, ...) or struct(**{'x':y, ...}) depending on what restrictions struct() enforces.
Or if we're paranoid about struct() only being allowed in .bzl and not BUILD, then represent it as a dict.

  • how to traverse label dependencies embedded within it.

As Bocete said, we're explicitly OK with not allowing labels, or having them implicitly decay into vanilla strings.

Then you need to think about how to represent attribute values of that type in Starlark within a rule implementation function,

List-of-structs-of-primitives. Not a difficult decision.

and come up with a good name for that type in the Starlark attr module.

Easy. struct_list

All of the tooling for formatting, linting, and analyzing BUILD files may need to be updated.

This doesn't introduce any new syntax, so only analysis would potentially need updating. Users can already call functions in BUILD files, so only more semantic-based analysis would be affected. And since we're disallowing labels, the analysis impact should be reduced.

It's usually possible to accomplish the end goal without making the target attribute grammar more expressive.

This is certainly true, but the UX of the workarounds aren't great in some cases. E.g. Exploding the struct into N separate *_dict attrs. And e.g. string_int_dict doesn't exist, so some extra conversions to/from strings may have to be performed.

If it's not, that may be a sign that attributes are not the right mechanism to use, and perhaps instead you should use [...] labels to sub-targets with more structure

I grant that this often works. But again, the UX can be suboptimal. It can be easier to read one target declaration, with some nesting in it, than jumping back-and-forth around to various sub-targets which the declaration references. And the sub-targets don't always make a lot of sense as independent entities. "This (sub-)target is only used by this (super-)target and will never be referenced by anything else. And you need to read it in the context of the super-target for it to make sense. Why do we have to write it this way? I resent having to devise names for these things." And then there's the sheer quantity and layering of the sub-targets.

Basically, I get it; there are non-trivial implementation costs. But the alternatives have their own UX costs. Not to mention that FWICT these workarounds / suggested-alternatives aren't documented well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants