existing_rule and rule.outputs use different Starlark representations for attributes

[dslomov]

(this issue originally reported by @alandonovan)

Observe the output of the program below:

$ cat t/BUILD
load(":inc.bzl", "r")

r(
    name = "myrule",
    src = "a",
)

print(type(native.existing_rule("myrule")["src"]))  # "string"

$ cat t/inc.bzl

def _implicit_outputs(src):
  return {"": "%s" % type(src)} # "Label"

def _implementation():
  pass

r = rule(
    outputs = _implicit_outputs,
    implementation = _implementation,
    attrs = {
        "src": attr.label(),
    },
)

$ blaze query t:*
WARNING: /google/src/cloud/adonovan/gbuild/google3/t/BUILD:8:1: string.
//t:Label
//t:myrule
//t:a
//t:BUILD

The existing_rule function converts a rule's attribute dictionary back into Skylark form. Similarly, the rule.outputs function converts some rule attributes into Skylark form. However, they use different conversion logic. For example, existing_rule converts labels to strings whereas rule.outputs preserves labels as labels.

This is confusing to users, since it requires twice as much documentation to specify (or would do, if either conversion were actually documented), and confusing to implementors, since it requires twice as much logic to implement.

They should use the same logic.

[meisterT]

closing in favor of #2884

[alandonovan]

There are two separate problems here: this one is a problem with the design; the other one (#2884) is a problem with the documentation.

Please re-open this issue.

[meisterT]

Thanks for paying attention, re-opened.

[brandjon]

Ideally existing_rule's attr types should be closely aligned with what you actually pass when instantiating a target. But to avoid ambiguity with respect to labels, we might have to use the Label type in existing_rule (and ensure that Label is valid as input to a target -- I forget offhand if that's the case).

If implicit outputs were to be redesigned, I'd want them to agree with existing_rules and the rest of the loading phase on the representation of attributes. But since we want to ditch implicit outputs, I'm less worried about that case. Regardless, the current design of implicit outputs doesn't inform what we should do for existing_rules very much.

[sgowroji]

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen (or ping me to reopen) if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.