Fix label_flag and label_setting to not have a dependency on the default #13372
Conversation
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291.
| @@ -941,7 +942,7 @@ public RuleClass build(String name, String key) { | |||
| attr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, type) | |||
| .nonconfigurable(BUILD_SETTING_DEFAULT_NONCONFIGURABLE) | |||
| .mandatory(); | |||
| if (BuildType.isLabelType(type)) { | |||
| if (type.getLabelClass() == LabelClass.DEPENDENCY) { | |||
There was a problem hiding this comment.
When is this true? It doesn't seem to be true at any call site and AFAIU in order to fix the associated bug, the build_setting_default attribute must not create any dependencies, and allowedFilesTypes() / allowedRuleClasses() only make sense if a dependency is created. Assert that it's not true instead? (either here or in BuildSetting)
There was a problem hiding this comment.
I changed this from isLabelType() to == LabelClass.DEPENDENCY specifically because of the change to NODEP_LABEL: with this change, the calls to allowedFileTypes() and allowedRuleClasses() were failing because they only accept real labels, not nodep labels.
Sorry I didn't wait for your review, this is currently internal at cl/369288702.
There was a problem hiding this comment.
Let me rephrase this: when I look at the code, RuleClass.getBuildSetting() never returns a Type where getLabelClass() would return DEPENDENCY. But then why is this branch here?
In addition, if there was any case where that did return DEPENDENCY, that would trigger the bug you are fixing here, so why not assert the opposite?
There was a problem hiding this comment.
Ah, now I see your point. This entire check should be removed, since it's not possible to trigger it.
Should I add a check in BuildSetting.create() to ensure that only NODEP_LABEL and NODEP_LABEL_CLASS are used? I will make changes in the internal version and test this.
There was a problem hiding this comment.
I think a better approach is to assert buildSetting.getType().getLabelClass() != LabelClass.DEPENDENCY beacuse that says exactly what is needed: that the type shouldn't be one that creates a dependency.
There was a problem hiding this comment.
But allowedFileTypes can only validly be called when the type is LabelClass.DEPENDENCY (see the definitions). Since it is now not possible to have an attribute with that type, there's no valid way to call this.
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291. Closes bazelbuild#13372. PiperOrigin-RevId: 369445041
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291. Closes bazelbuild#13372. PiperOrigin-RevId: 369445041
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291. Closes bazelbuild#13372. PiperOrigin-RevId: 369445041
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291. Closes bazelbuild#13372. PiperOrigin-RevId: 369445041
value.
This prevents an extra analysis, since the dependency should only be on
the value being used.
Fixes #11291.