fix: overwrite attribute values when not mergeable

[jbedard]

Fix #1962

What type of PR is this?

Bug fix

What package or component does this PR mostly affect?

all

What does this PR do? Why is it needed?

Allow updating non-mergeable/resolveable attributes.

Which issues(s) does this PR fix?

Fixes #1962

Other notes for review

[jbedard]

I actually don't think visibility should be here but it causes too many changes in this repo atm. I think those changes are actually correct but they could be breaking such as reducing the visibility of //internal/....

[fmeum]

@tyler-french Could you test this at Uber?

[fmeum]

Should this be

Suggested change

	// If src has an attribute not present in 'mergeable' and not marked with a
	// If dst has an attribute not present in 'mergeable' and not marked with a

[jbedard]

I think this comment is correct as-is, although maybe confusing.

This is pointing out that attributes in src will overwrite those in dst if they are unknown and the value in dst is not marked as # keep.

[jbedard]

I've updated the comment a bit. See the fixup commits.

[tyler-french]

@tyler-french Could you test this at Uber?

I'm seeing 1000s of build files behaving differently, need to dig a bit deeper. Please hold off on merging for now

[jbedard]

@tyler-french what types of changes are you seeing? Are effected attributes in the KindInfo anywhere?

[jbedard]

@tyler-french have you had a chance to look at this? I don't understand why you'd ever want SetAttr to not update the attribute?

[stoiky]

@tyler-french any news on this? We've been waiting for this fix at Adobe.

Thank you @jbedard and @fmeum!

[tyler-french]

The main issues we are seeing are:

• Things being removed from data attributes of go_test rules (specifically targets added are removed) and replaced with whatever gazelle applies.
• Non-deterministic changes to the ordering of fields, (specifically for ordered, non-sorted string_list attributes
• Changes to the behavior of the self_import attribute in the gomock rule (paths changing)
• Random new additions to certain fields

I will try to make reproductions with public code sometime soon

[jbedard]

Are those changes all in rules_go targets or in custom languages you have?

[jbedard]

@tyler-french any updates here? Even just ideas for tests I could write to reproduce the issues you're finding...

[jbedard]

@tyler-french I'm trying to understand what's going on here... it really sounds like all the issues you're seeing would be gazelle respecting SetAttr calls, which is the point of this PR.

When should a SetAttr call not overwrite an attribute?

[tyler-french]

The behavior that we depend on (for non-mergable attributes) is:
If an attribute doesn't exist, SetAttr sets the value of the attribute
If a user manually populates that attribute with their own value, then SetAttr cannot overwrite it (i.e. it is not mergeable), so we keep the previous value.

The issue here is that we have many attributes that are supplied by a user, and the understanding is that gazelle is used to generate these attributes if they don't exist, but once they do exist, gazelle doesn't have the "permission" to merge/overwrite them, since they are marked as not mergeable.

This is particularly needed/required when the gazelle plugin is not necessarily deterministic (e.g. an unsorted list), that way, we don't have to worry about gazelle overwriting an attribute when the value already exists, if the attribute is not mergeable.

I think the solution here is that we should have a separate OverwriteAttrs in rule.KindInfo that allows users to specify attributes the new value should always overwrite, thereby preserving historic behavior.

@linzhp do you have any thoughts?

[tyler-french]

So, I guess another option is we address the non-determinism in these extensions and also add # keep statements where appropriate.

So it's more of a question of either: is changing the accepted behavior of non-mergable attributes risky? and/or is there a way we can alleviate issues that arise from people who depend on this behavior.

[linzhp]

There are 3 types of attributes:

1. unmanaged: Gazelle doesn't manage them at all. All edits in build files are preserved.
2. default values: if the attribute is not set (e.g., in newly generated rules), Gazelle would populate the default value. However, if people customize them in the build files, the edits are preserved. This is currently achieved by setting the attributes in the extensions without declaring them in KindInfo. This is a frequent use case for visibility and go_test's data.
3. full control. Gazelle and its extensions are the only authoritative source. All user edits are overridden unless # keep is present. This is achieved by adding them in the KindInfo.

This PR removes the support of Type 2. As you found out, you have to make a special case for visibility, because it legitimately needs to be Type 2. Since what you need is Type 3 attributes, why don't you make them MergeableAttrs or ResolveAttrs?

[jbedard]

My intention was to expand the "default values" scenario (point 2) to allow languages to decide the default vs override behaviour, where today gazelle forces the default behaviour.

Since what you need is Type 3 attributes, why don't you make them MergeableAttrs or ResolveAttrs?

The primary scenario I had was gazelle only managing attributes if a user opts-in, and otherwise letting the user do it manually (without needing # keep 100% of the time). The main one being all the tsconfig related ts_project attributes. There's a lot of them so sometimes it's nice for gazelle to automate it, but other times users prefer just doing it within a macro to avoid unnecessary clutter in BUILD files (especially when these attributes are the same across an entire repo). These attributes are all trivial strings or booleans, no "merging" or "resolving" is required so I feel like they don't belong in either of those categories.

The second use case I had was a feature where the attributes aren't known at compile time, maybe they come from directives or a config file of some type etc. If a user changes those directives or config files the attributes might need to change.

[jbedard]

Regarding letting languages decide on default vs overwriting attributes... This PR allows languages to make that decision, but we could also add something such as SetAttrDefault to make that easier.

Or to avoid changing existing SetAttr behaviour we could instead add OverwriteAttr. IMO SetAttr setting the attribute and SetAttrDefault setting only a default is significantly clearer, but OverwriteAttr would be a new feature and not change any existing behaviour...

Note that # keep should always have priority in all these scenarios, like it does today.

[linzhp]

According to the doc here:

bazel-gazelle/rule/merge.go

Lines 35 to 38 in 6699394

	// If src and dst have the same attribute and the attribute is mergeable and the
	// attribute in dst is not marked with a "# keep" comment, values in the dst
	// attribute not marked with a "# keep" comment will be dropped, and values from
	// src will be copied in.

and the implementation here:

bazel-gazelle/rule/merge.go

Lines 108 to 110 in 6699394

	if srcAttr != nil && isScalar(srcAttr.expr.RHS) {
	return srcAttr.expr.RHS, nil
	}

string and boolean (and other "scalar" values) attributes are overridden by Gazelle if you declare them as MergeableAttrs. Yeah, the word "merge" is a bit confusing in this case, because there is no way to merge, say "True" into "False". We can interpret it as the attributes from "src" that Gazelle uses when "merging two rules".

Can you try declaring those attributes are MergeableAttrs as see if it works?

[jbedard]

Yeah, I understand the gazelle "mergeable" doesn't necessarily mean it will be merged. I think of it more as being "managed by gazelle" (which includes merge logic for non-primitives, # keep etc).

Can you try declaring those attributes are MergeableAttrs as see if it works?

That's what I have done to workaround the issue this PR is "fixing". It just means I need a workaround to maintain the existing values when I don't want gazelle to be managing those attributes.

[linzhp]

That's what I have done to workaround the issue this PR is "fixing". It just means I need a workaround to maintain the existing values when I don't want gazelle to be managing those attributes.

I don't think removing the support of Type 2 attributes is fixing an issue. You just need to make them Type 3, which is already supported. Like you noted above, you will have to find another workaround if you remove the Type 2 support in order to maintain existing values.

[jbedard]

I don't think removing the support of Type 2 attributes is fixing an issue.

This PR simply adds the ability to set an attribute without marking it as mergeable. This does not remove any functionality.

IMO if you want "default values" (type 2) then you should only set that default value if the attribute does not already have a value. Isn't that the definition of a default value?

[jbedard]

I have a workaround to accomplish what I'm looking for, but this use case seemed standard enough I thought a workaround shouldn't be required.

If we don't want to change existing behaviour we could add this functionality another way.

If we don't want to add this functionality we can close this PR.