Fix label_flag and label_setting to not have a dependency on the default#13372
Fix label_flag and label_setting to not have a dependency on the default#13372katre wants to merge 2 commits intobazelbuild:masterfrom katre:i11291-dep-cycle
Conversation
value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes #11291.
lberki
left a comment
lberki
left a comment
There was a problem hiding this comment.
Feel free to submit; I don't want to hold up this change for another day, regardless of the answer to the single comment I had.
Maybe also add a test case?
| .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.
3 participants
value.
This prevents an extra analysis, since the dependency should only be on
the value being used.
Fixes #11291.