Let Label#debugPrint emit label strings in display form

[fmeum]

This is a reland of 30b95e3 with a different approach to emitting display form labels that avoids adding a new to_display_form() method to Label:

• In action command lines, which are the most frequent use of labels in rule implementation functions, labels are automatically emitted in display form since 9d3a8b0.
• In module extensions and repository rules, if labels can be turned into display form, the inverse of the repository mapping would need to be tracked in lockfiles and marker files for correct incrementality. Furthermore, allowing implementation functions to access apparent names would allow them to "discriminate" against them, thus possibly breaking the user's capability to map repos at will via use_repo and repo_name. Similar to how providers on a target can't be enumerated, it is thus safer to not provide this information to the implementation functions directly.

This change thus implements StarlarkValue#debugPrint for Label to allow ruleset authors to emit labels in display form in warnings and error messages while ensuring that Starlark logic doesn't have access to this information. print("My message", label) degrades gracefully with older Bazel versions (it just prints a canonical label literal) and can thus be adopted immediately without a need for feature detection.

This requires changing the signature of StarlarkValue#debugPrint to receive the StarlarkThread instead of just the StarlarkSemantics. Since debugPrint is meant for emitting potentially non-deterministic information, it is safe to give it access to StarlarkThread.

Also improves the Bzlmod cycle reporter so that it prints helpful information on a cycle encountered in a previous iteration of this PR.

Fixes #20486

RELNOTES: Label instances passed to print or fail as positional arguments are now formatted with apparent repository names (optimized for human readability).

[fmeum]

@tetromino I have one more, but it's purely mechanical. :-)

[tetromino]

Could you please make the motivating change (for label printing) in the same PR?

[fmeum]

@Wyverald Friendly ping

[Wyverald]

sorry for the delay -- looks good to me! I'd still like @tetromino to take a look before we merge.

[tetromino]

Overall LGTM with the exception of a question about getMainRepositoryMapping

[tetromino]

I don't understand why return empty here; you can have a repo map in legacy WORKSPACE mode.

[fmeum]

This has always been broken in numerous ways, @Wyverald can probably say more about it. As far as I know you also couldn't define a non-trivial repo mapping for the main repo, only for other repos.

[tetromino]

One undesirable result of a null main repo map though is that in legacy WORKSPACE mode, all non-main-repo labels will get printed in @@ form.

[tetromino]

@fmeum - observe that the weird corner cases for legacy WORKSPACE, bazel_tools etc. are already well-handled by getRepositoryMapping. So how about the following implementation of getMainRepositoryMapping, which falls back to getRepositoryMapping for the edge case logic:

  @Nullable
  private static RepositoryMapping getMainRepositoryMapping(
      BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env)
      throws InterruptedException {
    boolean bzlmod = starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD);
    RepositoryMappingValue.Key repoMappingKey;
    if (key instanceof BzlLoadValue.KeyForBuild) {
      repoMappingKey = RepositoryMappingValue.key(RepositoryName.MAIN);
    } else if ((key instanceof BzlLoadValue.KeyForBzlmod
            && !(key instanceof BzlLoadValue.KeyForBzlmodBootstrap))
        || (bzlmod && key instanceof BzlLoadValue.KeyForWorkspace)) {
      // Since the main repo mapping requires evaluating WORKSPACE, but WORKSPACE can load from
      // extension repos, requesting the full main repo mapping would cause a cycle.
      repoMappingKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
    } else {
      // For builtins, @bazel_tools, and legacy WORKSPACE, the key's local repo mapping can be used
      // as the main repo mapping.
      return getRepositoryMapping(key, starlarkSemantics, env);
    }
    RepositoryMappingValue mainRepositoryMappingValue =
        (RepositoryMappingValue) env.getValue(repoMappingKey);
    if (mainRepositoryMappingValue == null) {
      return null;
    }
    return mainRepositoryMappingValue.getRepositoryMapping();
  }

Note that this will also let you get rid of the Optional wrapper as a side effect

[tetromino]

Also, @brandjon - do you have any comments on putting the main repo mapping in module context? Or maybe we can rather stick it into StarlarkThread somehow (I don't see any clean way to do it, but maybe you do)?

[fmeum]

Thanks for the suggestion, this looks much better. I also added a test for the --noenable_bzlmod case.

[tetromino]

I talked to @brandjon about this PR in our daily sync; he suggested we try to put the main repository mapping pointer in BazelStarlarkContext rather than in module context (mainly for memory usage reasons; there may be a vast number of module context objects in flight in memory at any one time, but only a small number of starlark threads).

[fmeum]

BazelStarlarkContext isn't available when a module extension is evaluated. Should I introduce a new Phase for that or reuse LOADING?

[fmeum]

I am now using BazelStarlarkContext and the existing WORKSPACE phase for module extension evaluation, PTAL.

@Wyverald Could you verify that adding BazelStarlarkContext to module extensions' Starlark threads doesn't give extensions access to functions they shouldn't have access to?

[iancha1992]

@tetromino friendly ping :)
Also 7.2.0 RC1 is scheduled for next Monday, 5/13/2024 FYI

[tetromino]

LGTM and I apologize for the wait!

Thank you for getting this complex PR to a polished state!