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.

Sign up for GitHub

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

rule: unset attribute if src is nil and no keep annotation in dst when merge #1993

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

rule: unset attribute if src is nil and no keep annotation in dst when merge #1993

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9456ce
rule: unset attribute if src is nil and no keep annotation in dst whe…
ashi009 Dec 9, 2024
9e0d2b7
remove debug message
ashi009 Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 44 additions & 7 deletions rule/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ func MergeRules(src, dst *Rule, mergeable map[string]bool, filename string) {
//
// The following kinds of expressions are recognized.
//
// * nil
// * strings (can only be merged with strings)
// * lists of strings
// * a call to select with a dict argument. The dict keys must be strings,
// - nil
// - strings (can only be merged with strings)
// - lists of strings
// - a call to select with a dict argument. The dict keys must be strings,
// and the values must be lists of strings.
// * a list of strings combined with a select call using +. The list must
// - a list of strings combined with a select call using +. The list must
// be the left operand.
// * an attr value that implements the Merger interface.
// - an attr value that implements the Merger interface.
//
// An error is returned if the expressions can't be merged, for example
// because they are not in one of the above formats.
Expand All @@ -102,7 +102,7 @@ func mergeAttrValues(srcAttr, dstAttr *attrValue) (bzl.Expr, error) {
return nil, nil
}
dst := dstAttr.expr.RHS
if srcAttr == nil && (dst == nil || isScalar(dst)) {
if srcAttr == nil && (dst == nil || !shouldKeepRecursively(dst)) {
return nil, nil
}
if srcAttr != nil && isScalar(srcAttr.expr.RHS) {
Expand Down Expand Up @@ -138,6 +138,43 @@ func mergeAttrValues(srcAttr, dstAttr *attrValue) (bzl.Expr, error) {
return makePlatformStringsExpr(mergedExprs), nil
}

func shouldKeepRecursively(e bzl.Expr) bool {
if ShouldKeep(e) {
return true
}
switch e := e.(type) {
case *bzl.AssignExpr:
if shouldKeepRecursively(e.RHS) {
return true
}
case *bzl.DictExpr:
for _, kv := range e.List {
if shouldKeepRecursively(kv.Value) {
return true
}
}
case *bzl.ListExpr:
for _, el := range e.List {
if shouldKeepRecursively(el) {
return true
}
}
case *bzl.CallExpr:
for _, arg := range e.List {
if shouldKeepRecursively(arg) {
return true
}
}
case *bzl.BinaryExpr:
if shouldKeepRecursively(e.X) || shouldKeepRecursively(e.Y) {
return true
}
default:
fmt.Printf("unexpected expression type %T\n", e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we walk expressions recursively, wouldn't we always hit this eventually if there is no keep comment? Can we avoid recursing so deeply and e.g. only do it once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, this was for debugging. Let me get rid of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I find it somewhat difficult to fix with a single keep comment check.

For example, when merging nil to a list with a single element marked as # keep. The merging logic should be handled by ListExpr.Merge. However, given the way the Merger interface is designed, it's src's responsibility to merge dst. A plain nil doesn't implement Merger, but relies on the heuristic of mergeAttrValues to figure that out.

And this won't work on complicated value types either, say some variant of PlatformList. As dst is always being parsed as CallExpr, it's src's responsibility to convert that to the actual type to merge.

My approach is basically to leave that attribute value alone if src wants to remove it and there is a # keep inside. Which will defer the merge issue to the user. (and it doesn't break established behavior based on the unit tests.)

Alternatively, or to say the ultimate solution, would be to rewrite rule API, to make it work like json/v2 that allows plugin writers to use actual go type in some rule structures, and let reflection do its magic while preserving relevant AST properties (spacing/comments etc.) So that when the merge happens, we can consult the typing info from the struct directly instead of guessing in a heuristic way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the explanation!

}
return false
}

func mergePlatformStringsExprs(src, dst platformStringsExprs) (platformStringsExprs, error) {
var ps platformStringsExprs
var err error
Expand Down