uber-go / gopatch

Refactoring and code transformation tool for Go.
MIT License
841 stars 33 forks source link

Patching ranged slice of struct literals #130

Closed mway closed 11 months ago

mway commented 11 months ago

I'm trying to write a patch to transform a ranged slice of (anonymous) struct literals. For example, I want to turn this:

for _, foo := range []struct {
  bar string
  baz string
} {
  {
    bar: "bar",
    baz: "baz",
  },
  // ...
} {
  // do something with foo
}

into this:

foos := []struct {
  bar string
  baz string
} {
  {
    bar: "bar",
    baz: "baz",
  },
  // ...
}

for _, foo := range foos {
  // do something with foo
}

I've tried minor variants of the following patch:

@@
var x identifier
@@
- for _, x := range []struct{ ... } {
  ...
- }
+ foos := []struct { ... }
+ for _, x := range foos {
  ...
+ }

which returns the following error:

0.minus.augmented:4:6: missing ',' before newline in composite literal (and 2 more errors)

@abhinav suggested adding a trailing ; to the elision, but that resulted in the same error.

I'm not sure if this is technically the same thing as #4, but I think for this case it doesn't seem practical to do it in two stages (e.g., a second match on the trailing } { would be extremely ambiguous).

Is this type of patch possible currently?

mway commented 11 months ago

cc @abhinav @sywhang

abhinav commented 11 months ago

@mway I misread the patch. The issue was that the left and right sides of the patch did not independently make valid syntax. The []struct{ ... } is just the type, it was missing the []struct{ ... }{ a, b, c } bit.

Try this:

@@
var x identifier
@@
- for _, x := range []struct{ ... }{
+ foos := []struct { ... }{
    ...,
- } {
+ }
+ for _, x := range foos {
  ...
  }

It's a bit unreadable, but it appears to work on my local sample.

For a more readable version, the []struct{ ... }{ ... } would have to be all on one line, but right now it can't be because the ... matching is janky. I think there was an issue somewhere to give them names so that you could do something like:

@@
var x identifier
var fields, items dots // TODO: better name
@@
+foos := []struct{ fields }{ items }
-for _, x := range []struct{ fields }{ items } {
+for _, x := range foos {
  ...
  }

Which would eliminate all the bugginess with ... matching.

abhinav commented 11 months ago

CC @lverma14

mway commented 11 months ago

Thanks @abhinav, that worked. :)

As a minor thing, it does end up with the following:

foos := []struct{
  // ...
}{
  // ...
  { // last entry
    // ...
  }}

instead of

foos := []struct{
  // ...
}{
  // ...
  { // last entry
    // ...
  },
}

(note the }} instead of },\n}). This seems to be regardless of any attempt at "forcing" the trailing comma of the list initialization to be included (maybe an artifact of capturing from the ...,?). It wouldn't be a problem if this was something that gofmt or gofumpt corrected, but unfortunately they don't.

abhinav commented 11 months ago

Yeah, my local experiment also showed the }}. For now, I would just sed the affected files to fix that. 😆

It's like that partly because ... is greedy about eating up comments and whitespace, so only what's necessary is left behind afterwards. There's definitely some room for improvement there.

mway commented 11 months ago

Yeah, that's a reasonable workaround. Thanks for the help!