incompatible_disable_target_provider_fields: disallow access of providers on "target" objects by field syntax

[c-parsons]

Related: #7347 (--incompatible_disallow_struct_provider_syntax)

Flag: --incompatible_disable_target_provider_fields

Motivation

The motivation for this change is described in #6241 and #7347. The affected Starlark syntax has been deprecated for over a year, and this aids #7347 in migrating off of the deprecated syntax.
The deprecated syntax is explicitly disabled with the flag --incompatible_disallow_struct_provider_syntax (see #7347). Namely, one is not allowed to return a struct from a rule implementation function. Instead, the returned object must be a provider-instance object or a list of such objects.

That is,

  foo_info = FooProvider()
  return struct(foo_info = [foo_info])

is disallowed. Instead, use the following:

  foo_info = FooProvider()
  return [foo_info]

However, this is difficult to do in isolation. Namely, if my rule foo_rule uses the above deprecated syntax, and I have a rule bar_rule which depends on a target of foo_rule, then bar_rule's implementation might contain:

  ctx.attr.dep.foo_info

So, when foo_rule changes, bar_rule will break.
This flag, --incompatible_disallow_struct_provider_syntax, explicitly disables the bar_rule's syntax, which facilitates a multi-phase migration. See the proposed migration ordering below.

Migration

Here we outline the migration plan for --incompatible_disable_target_provider_fields and --incompatible_disallow_struct_provider_syntax:

STEP 1: Rule implementation functions which return providers use the old struct syntax must simultaneously support the new syntax and the old syntax. For example:

  foo_info = FooProvider()
  return struct(foo_info = [foo_info])

becomes:

  foo_info = FooProvider()
  return struct(foo_info = [foo_info], providers = [foo_info])

STEP 2: Rules which depend on rules using the deprecated syntax can, after step 1, access the provider using either ctx.attr.dep.foo_info or ctx.attr.dep[FooProvider]. They should migrate to using the ctx.attr.dep[FooProvider] syntax. This can and should be verified by using the --incompatible_disable_target_provider_fields flag.

STEP 3: Rules which needed to be migrated in step 1 can completely migrate to the new syntax. The syntax from step 1 becomes:

  foo_info = FooProvider()
  return [foo_info]

This becomes a safe migration if we're guaranteed that all users of the rule have --incompatible_disable_target_provider_fields enabled from step 2.
We can completely deprecate the old syntax using the flag --incompatible_disallow_struct_provider_syntax (#7347).

[dslomov]

Is this realistic for 1.0? Many rules will need to be migrated, and migration is non-trivial and non-automatic.

[Globegitter]

Yeah I do agree, especially given this flag has just been introduced unflipped for the upcoming 0.29 as far as I am aware. Maybe it is worth considering to postpone 1.0 a month to allow for smoother migration?

[laurentlb]

It's a much simpler version of #7347 (basically, just the first step of it). I'm interested to hear if there's any migration difficulty with this new flag.

[ittaiz]

There's a good chance #10170 is a blocker for this effort.

[ittaiz]

I'm mistaken, there's a workaround

[limdor]

@philwo this was marked as breaking change for 5.0 but looking at the release notes and the PRs I think it was not flipped. Could it be that this has not been flipped in Bazel 5.0? If that is the case could you update the label?
I found it going over the list in https://github.com/bazelbuild/bazel/labels/breaking-change-5.0

[martis42]

Can confirm this was not flipped:
https://github.com/bazelbuild/bazel/blob/5.0.0/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java#L313

[limdor]

@philwo @c-parsons how should we proceed then? Can you please remove the label breaking-change-5.0 with this way bazelisk --migrate behave as it should be?
As a follow up, was this intentional to not flip this in Bazel 5 or was forgotten? If it was the second I could create a PR doing the flip in master.