Starlark rules should allow selectable attribute defaults

[gregestren]

Given simple.bzl:

def simple_impl(ctx):
ctx.file_action(
output = ctx.outputs.out,
content = ctx.attr.foo)

simple = rule(
simple_impl,
attrs = {
'foo': select({ '//conditions:default': 'bard' }),
},
outputs = {'out': '%{name}.out' },
)

and BUILD:

simple(name = 'foo')

building ':foo' produces the error:
Illegal argument: expected <String, Builder> type for 'attrs' but got <string, SelectorList> instead.

This is an attempt at a "configurable attribute default", which should be able to work just fine. This failure may come from Skylark or it may come from Bazel's underlying rule machinery, which currently doesn't have any intelligent concept of selectable values not explicitly set in BUILD files.

Originally noticed in https://groups.google.com/d/msg/bazel-discuss/5VR2rVfak-8/dMxXDW1DnZ4J

[hsyed]

This one is quite important for relaying features to a persistent worker.

[gregestren]

Hassan - can you elaborate?

[hsyed]

I need to convey certain feature flags to the kotlin builder. Some should invalid a target others shouldn't.

Currently I am working on strict_deps and need to make it toggle-able as I want to release it as experimental. I can't use the java_strict_deps flag this applies globally. Strict deps setting should invalidate targets it could go on the as a hidden attribute on toolchain(do toolchains support hidden attributes ?) -- This is what I am doing at the moment.

In this instance I'd rather do the select in an implicit rule attribute using default in attr. Ideally select should be available as a function on ctx as well. If selected values go into the params file of a persistent builder it would end up invalidating the target when it changes.

[kastiglione]

Laurent had a good suggestion (#1698 (comment))

The workaround for now is to wrap the rule with a macro, and use the select in the macro.

[SrodriguezO]

Any chance of this being revisited? A drawback of the macro wrapper workaround is that you lose the rule signature docs if you're using Stardocs (higherkindness/rules_scala#258 (comment))

[gregestren]

I think the best way forward is to get implementation guidance from whoever knows the code best so we have a shovel-ready recipe for anyone willing to make a patch.

I poked into this a bit. Using:

def _impl(ctx):
    print("My name is " + ctx.attr.name)
    print("My str is  " + ctx.attr.mystr)

simple_rule = rule(
    implementation = _impl,
    attrs = {
        "mystr": attr.string(default =
            select({"//conditions:default": "foo"})
        ),
    },
)

I get the error

expected value of type 'string' for parameter 'default', for call to method
string(default = '', doc = '',  mandatory = False, values = []) of 'attr (a language
module)'

This comes from

bazel/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java

Lines 287 to 293 in 770606a

	if (!type.contains(value)) {
	throw argumentMismatchException(
	call,
	String.format(
	"expected value of type '%s' for parameter '%s'", type, param.getName()),
	method,
	objClass);

.

type is a SkylarkType$Simple over strings and value is a SelectorList over strings. contains fails because their immediate types aren't the same. But type.type and value.type are the same (java.lang.String). So we could compare those values instead when value is a SelectorList to bypass this error.

When I tried doing that Bazel crashed on an argument type mismatch in

bazel/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java

Line 154 in 770606a

result = method.invoke(obj, args);

.

I'm passing this onto the Starlark experts to evaluate how much more Starlark machinery we'd have to plumb through to successfully preserve a SelectorList, and to evaluate how challenging the full implementation would be.

Starlark folks: How invasive is it for attr to understand / propagate select?

[brandjon]

The type checking is enforced in StarlarkAttrModule via the method param types and the explicit type checks. It also rules out select()s by using type.convert() instead of type.selectableConvert(), but perhaps nothing would break if that were changed.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[github-actions]

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!