trim issues after upgrade from v0.8.2 to v0.9.0

[freeformz]

What version of CUE are you using (cue version)?

$ cue version
cue version v0.9.0

go version go1.22.4
      -buildmode exe
       -compiler gc
       -trimpath true
     CGO_ENABLED 0
          GOARCH arm64
            GOOS darwin
cue.lang.version v0.9.0

Does this issue reproduce with the latest stable release?

Yes - is new with v0.9.0 - not present in v0.8.2

What did you do?

$ cue trim ./elevation/...
imported and not used: "strings":
    ./elevation/apps/config/satebatcher.cue:4:2

But I am....

$ ack -A 1 -B 1 "strings\." --  ./elevation/apps/config/satebatcher.cue
                        for zone in #edgeAppZones {
                                let suffix = strings.SliceRunes(zone.name, strings.LastIndex(zone.name, "-")+2, len(zone.name))
                                #satebatcherComponent & {

So I added _what: strings.Split("what-the", "-")[0] to the file ... and re-ran the trim

$ cue trim ./elevation/...
< LARGE DIFF with a bunch of stuff removed >
Aborting trim, output differs after trimming. This is a bug! Use -i to force trim.
You can file a bug here: https://cuelang.org/issues/new?assignees=&labels=NeedsInvestigation&template=bug_report.md&title=
make: *** [cue/trim] Error 1

Most, but not all of the - in the diff are for comprehensions....

I am going to see if I can create a minimal re-producer, but don't have one yet.

What did you expect to see?

No change really ... this is an upgrade to v0.9.0 from a project that was using v0.8.2

What did you see instead?

See above.

[freeformz]

I've tried to put together a re-producer, but I haven't been able to and I don't think it's a good idea to publicly make available the private cue bits.

[freeformz]

Oddly enough I changed our cue fmt makefile target to run and it runs fine (the mindepth there is due to the layout of the repo and how we use cue export)

find ./elevation -type d -mindepth 6 | xargs -n 1 -I % sh -c "echo cue trim -s --strict %; cue trim -s --strict %"

[mvdan]

@freeformz if you're unable to reduce the reproducer to be shared publicly, can you send it to me privately, for example via Slack? I can then do the reduction work so that we can have a small reproducer to investigate and add as a regression test.

Without a reproducer it might be hard for us to investigate this bug properly, as you can imagine.

[freeformz]

@mvdan Thx. Sent via slack.

[freeformz]

@mvdan Can I help in any other way?

[mvdan]

Apologies for the slowness, @freeformz, I've been on holiday recently.

@cuematthew and I have reduced the "unused import" error to this testscript:

exec cue trim input.cue

-- input.cue --
package apps

import "strings"

#values: {
	list: _
}
#values & {
	list: [
		for never in []
		let trimmed = strings.TrimSpace(never)
		{ trimmed }
	]
}

Rather than succeeding as expected, by for example trimming the file or doing nothing at all, we get a confusing error like yours:

> exec cue trim input.cue
[stderr]
imported and not used: "strings":
    ./input.cue:3:8
[exit status 1]
FAIL: test.txtar:1: unexpected command failure

It's worth pointing out that CUE v0.8.2 failed in your code when running cue trim with a single package, but when running on all packages, it seemingly hid the error - which doesn't look right:

$ cue-v0.8.2 trim ./elevation/apps/config
imported and not used: "strings":
    ./elevation/apps/config/satebatcher.cue:4:2
$ cue-v0.8.2 trim ./...
$ echo $?
0

Something changed between 0.8 and 0.9 where CUE now consistently shows the error, rather than inconsistently hiding it:

$ cue-v0.9.0 trim ./elevation/apps/config
imported and not used: "strings":
    ./elevation/apps/config/satebatcher.cue:4:2
$ cue-v0.9.0 trim ./...
imported and not used: "strings":
    ./elevation/apps/config/satebatcher.cue:4:2

So it seems to us like the underlying "imported and not used" bug in trim already existed in v0.8, it was just being hidden from you when you ran cue trim ./.... That bug still seems like a problem and we will look into it - but now we have a much smaller reproducer, and it's interesting to see that it existed for a while.

[mvdan]

We found the cause :) In your config, you had a list comprehension that always resulted in zero elements (./elevation/apps/config._satebatcherValues.app.components, which also has conflicting list types between [...string] and [...{}]?), so cue trim was trying to delete the entire comprehension and leave an empty list. This comprehension is where strings.SliceRunes was used, so when the only use of the strings import was removed, we should remove the import as well.

cue trim uses https://pkg.go.dev/cuelang.org/go/cue/ast/astutil#Sanitize to remove such unused imports, but it didn't do that correctly, hence the error. I'll send a fix shortly for the astutil package.

It's worth noting that, after the fix, cue trim ./elevation/apps/config still results in an error, due to cue trim having other bugs:

Aborting trim, output differs after trimming. This is a bug! Use -i to force trim.
You can file a bug here: https://cuelang.org/issues/new?assignees=&labels=NeedsInvestigation&template=bug_report.md&title=

But you can use cue trim -i ./elevation/apps/config to skip past that sanity check error and trim the files on disk, which does result in the expected changes I explain above. We are aware of the "output differs after trimming" bugs, and @cuematthew is looking at a new trim algorithm to resolve those.

So, in short - CUE v0.8.2's cue trim ./... seemed to be a no-op. Not only did it fail to trim due to a bug, it didn't even show you any error message at all. CUE v0.9.0 and master fail due to the astutil bug above, which we will fix - and then trimming will make changes with the -i flag.

From a quick look, the changes that cue trim -i ./... does make after the astutil fix don't look right. However, as I showed above with the existing breadth of trim issues, that's not entirely surprising. We will leave this issue open after the astutil fix, so that we can try trimming your project with the new algorithm once it is ready.

[freeformz]

@mvdan TY for the explanation and detailed break down!

[freeformz]

@mvdan I'm not sure how ./elevation/apps/config._satebatcherValues.app.components could have conflicting types of [...string] and [...{}] - any thoughts on debugging that ?

[freeformz]

AFAICT I'm essentially doing this: https://cuelang.org/docs/tour/expressions/fieldcomp/

_satebatcherValues: xyz.#values & {
	app: {
		name: _meta.name
		components: [
			for zone in _something
			let suffix = strings.SliceRunes(zone.name, strings.LastIndex(zone.name, "-")+2, len(zone.name)) {
				_satebatcherComponent & {
                                     name: "\(suffix)"
                                     ...
				}
			},
		]
	}
}

[mvdan]

The astutil.Sanitize fix is now at https://review.gerrithub.io/c/cue-lang/cue/+/1198351.

Please ignore my mention of a list type mismatch - we spent a while aggressively reducing your code to reach the dozen lines of CUE I shared yesterday, and at some point during that process we made the list comprehension produce a list of strings rather than a list of structs. Your original code doesn't have this issue, as you correctly point out. The reproducer I shared ended up not having this issue either in the end, as we swapped list: [...{}] for list: _ and the bug still reproduced.