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 alias_label struct field to all targets #18100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 14, 2023

This allows rules to obtain the label of an alias target rather than only the label of the target an alias chain ultimately resolves to. The former is the label as specified by the user on the rule, which can be important to know for e.g. fixup messages and manual implementations of location expansion.

Fixes #11044

@fmeum fmeum marked this pull request as ready for review April 14, 2023 14:21
@fmeum fmeum requested review from a team and lberki as code owners April 14, 2023 14:21
@fmeum fmeum requested review from katre and removed request for a team April 14, 2023 14:21
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Apr 14, 2023
This allows rules to obtain the label of an `alias` target rather than
only the label of the target an alias chain ultimately resolves to. The
former is the label as specified by the user on the rule, which can be
important to know for e.g. fixup messages and manual implementations of
location expansion.
@lberki
Copy link
Contributor

lberki commented Apr 17, 2023

We already have AliasProvider, why not make that available to Starlark instead?

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 17, 2023

We already have AliasProvider, why not make that available to Starlark instead?

To me AliasProvider feels more like a Bazel implementation detail than an intuitive API object. I find ctx.attr.dep.alias_label in parallel to the existing ctx.attr.dep.label much more readable than ctx.attr.dep[AliasProvider].label (which would additionally need an in check for AliasProvider). Exposing AliasProvider would also allow users to restrict attributes to targets that provide it, which I don't think should be possible.

Unless there is additional alias-related logic that may need to be exposed to Starlark, a struct field on Target, I would thus prefer not to expose this information via AliasProvider.

@lberki
Copy link
Contributor

lberki commented Apr 24, 2023

FWIW, AliasProvider has the advantage that it allows one to access the full chain of aliases, not just the two labels at the end of it. I can relate to your argument, though, that it would allow the abuse you described above.

@comius do you have an opinion?

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 24, 2023

@lberki Do you have an example use case that relies on the intermediate labels in the chain? I have a hard time coming up with any and wonder whether exposing this chain would really be a desirable feature. Bazel appears to only contain a single reference and that is in aspect logic.

@lberki
Copy link
Contributor

lberki commented Apr 24, 2023

To be pedantic, I didn't say that it's useful, I said that it's possible :)

Bazel uses it internally for better error reporting ("Target :a reached through alias chain :b -> :c -> :a is illegal) . "fixup messages and manual implementations of location expansion" sounds similar, although I could be easily convinced that Starlark code doesn't need to know about it.

@comius
Copy link
Contributor

comius commented Apr 24, 2023

Could I ask for a design document here? I think there are several different ways how to implement this, several problems and APIs related to this. There's also a philosophical aspect (or call it a good design practice), that an alias should be indistinguishable from the target it points to.

I'm lacking bandwidth to do an ad hoc "design doc" in my mind, review all the surrounding code and then confirm all the implicit design decision a PR like this could be making.

@katre katre requested review from comius and removed request for katre May 10, 2023 14:52
@katre katre added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels May 10, 2023
@gregestren gregestren removed the team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts label May 10, 2023
@katre katre added the team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts label May 10, 2023
@comius comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to get the alias label in starlark
5 participants