rule: unset attribute if src is nil and no keep annotation in dst when merge #1993
Conversation
rule/merge.go
Outdated
| return true | ||
| } | ||
| default: | ||
| fmt.Printf("unexpected expression type %T\n", e) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
oops, this was for debugging. Let me get rid of it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense, thanks for the explanation!
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?
This change will allow merging a non-existing attribute to any dst attribute not annotated with
# keep.The current merge logic requires the dst attribute to be a scaler to unset, which prevents Gazelle from removing an attribute using
glob().Which issues(s) does this PR fix?
Fixes #1989