Add a command to explicitly remove a load

[creachadair]

As part of working around https://github.com/bazelbuild/buildtools/issues/192, I wrote a macro to silence the deprecation warning from go_test when the library= attribute is set. That allows us to keep the syntax of go_test for compatibility with our imports, without triggering several pages of deprecation warnings every time we build.

Unfortunately, that results in a different tricky problem: Now each file with a go_test has a load statement like

load("//tools:build_rules/testing.bzl, "go_test")

That file also defines some other things, so I don't want to delete the load unconditionally. But fix unusedLoads will not remove it, which means we have to basically do something like

1. Unconditionally delete all the loads of that file.
2. Unconditionally add loads of everything in the file except our go_test macro.
3. Fix unused loads.

This turns out to be tricky to get right, not to mention brittle.

In hindsight, I might have to go back and move the macro into its own file, but it would be helpful if buildozer had a way to explicitly remove a loaded symbol for cases like this, e.g.,

remove_load //tools:build_rules/testing.bzl go_test 

For files that do not include this bizzle file, or which do not import go_test, it would be a no-op.

[laurentlb]

Fix unused loads.

This command removes any load where the symbol is unused in the file. If you have any reference go_test in the file, it will keep the load.

The command remove_load seems useful only in the case where: you load a symbol with the same name of a predefined symbol in order to shadow it. Maybe it will be clearer if you use another name? If you use a name other than go_test, Buildozer can do the renaming.

[creachadair]

We specifically did not want to use a different name for the macro, since there does not seem to be any way to change the type of an existing rule invocation. What we really wanted was #192.

I wound up separating the macro into a separate file, deleting the load entirely (with a regexp, since fix unusedLoads won't remove it for the reason you described), and dropping the macro definition from the import.

[chsigg]

We would like to have a remove_load command as well so I can combine it with new_load to change load statements in TensorFlow's BUILD files.

[laurentlb]

There are a few options for the implementation:

• Update new_load to remove the previous conflicting definitions.
• Add a new command remove_load (to be combined with new_load).
• Add a new command replace_load.

[chsigg]

Update new_load to remove the previous conflicting definitions.

I don't think that would work in our case. We want to do things like: Before: load(":foo.bzl", "x", "y") After: load(":foo.bzl, "a") load(":bar.bzl", "x")

[laurentlb]

buildozer "new_load bar.bzl x" could remove the previous definition of x.

In your example, "y" was replaced with "a", is it intentional? If so, do you want to rename all the call sites in the fiel?

[chsigg]

buildozer "new_load bar.bzl x" could remove the previous definition of x.

Ah, I see. But we do sometimes simply want to drop loads completely.

is it intentional?

Yes, but the example was probably a bit too random. No, I wouldn't expect that to update the call sites.

The background is that remove_load would allow us to simplify TensorFlow's copybara script with automatic reverse commands (new_load being the opposite of remove_load). Currently we manually specify 'fix unusedLoads' as the reverse of a new_load. Or we textually replace loads with a placeholder comment. Both seem clunky.

[ericastor]

This is also impacted by the fact that fix unusedLoads will not remove loads with a comment attached to them... so if a load is first in the block after a file-level comment, or if there's a comment between loads, there's currently no way to remove it.

As of right now, we either need a way to suppress that behavior, or add an explicit remove_load command. @laurentlb, @vladmos - any thoughts?

[vladmos]

There's a pending fix (#896) for removing load statements with comments, I'll try to get it submitted soon. There was a question which whether comments should also be removed together with the load statement, I think the most reasonable behavior is to remove comments on the same line but keep before- and after-comments.