Fix visibility check at definition for aspects #19442

fmeum · 2023-09-07T11:43:23Z

With --incompatible_visibility_private_attributes_at_definition, the visibility of aspects (and aspects on aspects) into their implicit dependencies is now checked relative to the aspect's, not the rule's definition.

This requires ensuring the precedence of aspect on aspect attributes over those of the underlying aspect in DependencyResolutionHelper, which was hinted at by a comment but not realized.

fmeum · 2023-09-07T14:31:30Z

@mai93 @comius was right in #19141 (comment), the aspect case of --incompatible_visibility_private_attributes_at_definition wasn't implemented correctly.

I think that visibility for an implicit attribute of an aspect should be checked relative to that aspect's definition. I have implemented this in this PR and added test cases.

The new tests uncovered the interesting edge case of which attribute wins if two aspects define attributes with the same name. In the spirit of @mai93's commit 93ad589, I would argue that the outer aspect should see its own attribute or it might risk being broken by being applied to an aspect with an attribute of an incompatible type. aspectWithExtraAttributeDependsOnNotApplicable_usesAttributeFromDependentAspect codifies the opposite behavior though.

Is flipping that behavior around something we could do? If not, could we do that for implicit attributes only? Picking up an implicit attribute from a lower aspect instead of the own one seems just wrong.

mai93 · 2023-09-07T20:57:25Z

Thanks @fmeum! I also think the main aspect should get its dependencies from its own attributes not from its underlying aspects. I need to run internal tests for this part to verify that there is nothing depending on the old behaviour that needs to be updated first.

src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

fmeum · 2023-09-08T12:41:26Z

@mai93 Thanks for the feedback. I will separate out the change to aspect ordering.

I identified another place in which this ordering would probably need to be changed, but it doesn't seem to be covered by tests: #19449
In a previous version of that test PR, I replaced putIfAbsent with put, but all tests passed (except one that seemed unrelated). Do you happen to have an idea why this doesn't have an effect on the existing aspect order tests?

fmeum · 2023-09-10T09:24:18Z

@mai93 I extracted the aspect-on-aspect fixes into #19449 and stacked the visibility fixes onto that PR.

fmeum · 2023-09-13T06:45:59Z

@mai93 I set the test blocked on #19449 to ignored as you suggested in #19449 (comment).

mai93 · 2023-09-14T22:07:49Z

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

      if (!RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
        handleVisibilityConflict(context, prerequisite, implicitDefinition);
      }
    }
  }

+  private static boolean forStarlarkRuleOrAspect(RuleContext.Builder context) {
+    Aspect currentAspect = context.getMainAspect();
+    if (currentAspect != null && currentAspect.getAspectClass() instanceof StarlarkAspectClass) {


I missed this in the first review but if the aspect is native, this needs to return False and not fallback to the rule type.

Good catch, fixed. Also renamed the local variable to mainAspect.

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

comius · 2023-09-19T04:46:59Z

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

+          // Only verify visibility of implicit dependencies of the current aspect. Implicit
+          // dependencies of other aspects as well as the rule itself are checked when they are
+          // evaluated.
+          return;


Readability: The return here breaks the encapsulation of the else block. You can't add any code after the block and assume it's correct.

Possible solution is to invert if (!mainAspect.getDefinition().getAttributes().containsKey(attrName)) {
and move the block if (!RuleContext.isVisible... inside after setting implicitDefinition.

I wasn't entirely sure what you meant with "move the block ... inside". I left it outside without duplicating it, please let me know what you think.

src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java

With `--incompatible_visibility_private_attributes_at_definition`, the visibility of aspects (and aspects on aspects) into their implicit dependencies is now checked relative to the aspect's, not the rule's definition.

src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java

comius · 2023-09-20T16:33:21Z

This is ready for reimport. @iancha1992

With `--incompatible_visibility_private_attributes_at_definition`, the visibility of aspects (and aspects on aspects) into their implicit dependencies is now checked relative to the aspect's, not the rule's definition. This requires ensuring the precedence of aspect on aspect attributes over those of the underlying aspect in `DependencyResolutionHelper`, which was hinted at by a comment but not realized. Closes #19442. PiperOrigin-RevId: 567333681 Change-Id: I167c45fea8535e0f35c0298ca1cb9f845c9fe2d2 (cherry picked from commit 7a537ca)

fmeum force-pushed the aspect-check-at-definition branch 2 times, most recently from 0e2def4 to c7289ce Compare September 7, 2023 14:25

fmeum marked this pull request as ready for review September 7, 2023 14:31

fmeum requested a review from a team as a code owner September 7, 2023 14:31

fmeum requested review from katre, a team and sdtwigg and removed request for a team and katre September 7, 2023 14:31

fmeum assigned comius and mai93 Sep 7, 2023

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Sep 7, 2023

fmeum unassigned comius and mai93 Sep 7, 2023

fmeum requested review from comius and mai93 and removed request for sdtwigg September 7, 2023 14:32

mai93 reviewed Sep 7, 2023

View reviewed changes

src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java Outdated Show resolved Hide resolved

fmeum force-pushed the aspect-check-at-definition branch 2 times, most recently from 4f9f0c6 to f5f0838 Compare September 10, 2023 09:23

fmeum requested a review from mai93 September 10, 2023 09:24

fmeum force-pushed the aspect-check-at-definition branch 2 times, most recently from 4126a9a to 46f5235 Compare September 10, 2023 09:42

mai93 mentioned this pull request Sep 11, 2023

Fix type checking for colliding aspect-on-aspect attributes #19449

Closed

fmeum force-pushed the aspect-check-at-definition branch from 46f5235 to 22c8921 Compare September 13, 2023 06:45

mai93 reviewed Sep 14, 2023

View reviewed changes

fmeum force-pushed the aspect-check-at-definition branch 2 times, most recently from e3d7e1a to 81ee3b3 Compare September 15, 2023 06:22

fmeum requested a review from mai93 September 15, 2023 06:22

mai93 approved these changes Sep 15, 2023

View reviewed changes

mai93 added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 15, 2023

comius requested changes Sep 19, 2023

View reviewed changes

comius added awaiting-user-response Awaiting a response from the author and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Sep 19, 2023

Fix visibility check at definition for aspects

2ed46a7

With `--incompatible_visibility_private_attributes_at_definition`, the visibility of aspects (and aspects on aspects) into their implicit dependencies is now checked relative to the aspect's, not the rule's definition.

fmeum force-pushed the aspect-check-at-definition branch from 81ee3b3 to 2ed46a7 Compare September 19, 2023 06:18

fmeum requested a review from comius September 19, 2023 06:20

comius approved these changes Sep 20, 2023

View reviewed changes

src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java Show resolved Hide resolved

comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Sep 20, 2023

fmeum mentioned this pull request Sep 21, 2023

incompatible_visibility_private_attributes_at_definition #19330

Closed

copybara-service bot closed this in 7a537ca Sep 21, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 21, 2023

fmeum deleted the aspect-check-at-definition branch September 22, 2023 06:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix visibility check at definition for aspects #19442

Fix visibility check at definition for aspects #19442

fmeum commented Sep 7, 2023 •

edited

Loading

fmeum commented Sep 7, 2023

mai93 commented Sep 7, 2023

fmeum commented Sep 8, 2023

fmeum commented Sep 10, 2023

fmeum commented Sep 13, 2023

mai93 Sep 14, 2023

fmeum Sep 15, 2023

comius Sep 19, 2023

fmeum Sep 19, 2023

comius commented Sep 20, 2023

Fix visibility check at definition for aspects #19442

Fix visibility check at definition for aspects #19442

Conversation

fmeum commented Sep 7, 2023 • edited Loading

fmeum commented Sep 7, 2023

mai93 commented Sep 7, 2023

fmeum commented Sep 8, 2023

fmeum commented Sep 10, 2023

fmeum commented Sep 13, 2023

mai93 Sep 14, 2023

Choose a reason for hiding this comment

fmeum Sep 15, 2023

Choose a reason for hiding this comment

comius Sep 19, 2023

Choose a reason for hiding this comment

fmeum Sep 19, 2023

Choose a reason for hiding this comment

comius commented Sep 20, 2023

fmeum commented Sep 7, 2023 •

edited

Loading