ReconfigureIO / goblin

convert from a Go AST to JSON
Apache License 2.0
42 stars 15 forks source link

`const` definitions with multiple assignments are parsed as separate declarations #38

Closed patrickt closed 7 years ago

patrickt commented 7 years ago

The top-level expression

const ( 
    Foo = 1
    Bar = 2
    Baz = 3
)

is parsed as three separate declarations, identical to

const Foo = 1
const Bar = 2
const Baz = 3

This normally doesn't cause any problems, except for the case of iota propagation within a single const definition. This will require some pretty heavy-duty rejiggering in DumpDecl.

bohde commented 7 years ago

replaceIota is not being called

lanzafame commented 7 years ago

Not sure if I am understanding the problem correctly, but using goblin version v0.1.4-5-ga72e012 I am getting slightly different output for the above two snippets and rather different output for an iota propagation statement.

Snippet 1:

package iota

const (
    Foo = 1
    Bar = 2
    Baz = 3
)

Snippet 2:

package iota

const Foo = 1
const Bar = 2
const Baz = 3

Snippet 3:

package iota

const (
    Foo = iota
    Bar
    Baz
)

Snippet 1 & 2 Diff:

diff snippet1 snippet 2
24c24,26
<       },
---
>       }
>     ],
>     [
43c45,47
<       },
---
>       }
>     ],
>     [

Snippet 1 & 3 Diff:

diff snippet1 snippet 3
19,21c19,24
<             "kind": "literal",
<             "type": "INT",
<             "value": "1"
---
>             "kind": "expression",
>             "type": "identifier",
>             "value": {
>               "kind": "literal",
>               "type": "IOTA"
>             }
36,42c39
<         "values": [
<           {
<             "kind": "literal",
<             "type": "INT",
<             "value": "2"
<           }
<         ]
---
>         "values": []
55,61c52
<         "values": [
<           {
<             "kind": "literal",
<             "type": "INT",
<             "value": "3"
<           }
<         ]
---
>         "values": []

So two things I am unsure of 1) wherein the golang source code the incrementing of iota consts is occurring, as I can't figure out whether the JSON output of Snippet 3 should have the values populated or not, 2) whether goblin plans to fully support the Go spec?

patrickt commented 7 years ago

@Lanzafame iota incrementing is done not in the goblin code, but in our downstream proprietary code. (I should have been more clear about this in the bug report; my apologies. We may open-source this code someday, too, though that's at least a year off.)

We do this because go/ast preserves iota as a literal, and we wanted goblin both to map very closely onto Go's declared AST, and to do only a few things (and do them well) rather than have feature creep—there are a bunch of other small things that goblin could do with additional traversals, but we'd rather that people do them downstream in whatever consumes goblin output. (Besides, Go does not shine in terms of high-level traversal of data structures.)

Secondly, yeah, I can definitely see that the output between your first two examples differ slightly. However, in goblin 0.2, they will differ drastically. Here's an example (please don't hold me to any details of the JSON output, as this will probably change):

const Foo = 1
const Bar = 2
const Baz = 3

will stay mostly the same, having three separate "const" entries (I've omitted a lot of the JSON here for readability's sake):

// entry 1
        "names": [
          {
            "kind": "ident",
            "value": "Foo"
          }
        ],
        "type": "const",
        "values": [
          {
            "kind": "literal",
            "type": "INT",
            "value": "1"
          }
        ]
      }
    ],
    [
// entry 2
      {
        "names": [
          {
            "kind": "ident",
            "value": "Bar"
          }
        ],
        "type": "const",
        "values": [
          {
            "kind": "literal",
            "type": "INT",
            "value": "2"
          }
        ]
      }
    ],
    [
// entry 3
      {
        "names": [
          {
            "kind": "ident",
            "value": "Baz"
          }
        ],
        "type": "const",
        "values": [
          {
            "kind": "literal",
            "type": "INT",
            "value": "3"
          }
        ]
      }
    ]
  ],
}

Whereas, by contrast, a const declaration with three sub-entries will be parsed not as three top-level entries, but as one top-level entry with three children. Something like

{
    "type": "const",
    "entries": [
         {"name": "Foo",
          "value": 1},
         {"name": "Bar",
          "value": 2},
        {"name": "Baz",
         "value": 3 }
    ]
}

That way, in our downstream code, we can successfully propagate iota values throughout individual const declarations by iterating through the structures yielded from the entries array.

And yes, we do want to support the Go spec as much as is possible. We don't, however, want to do too much in goblin—there are certain aspects of Go, such as disambiguating casts/function calls, or qualified functions/method invocations, that are simply impossible to do without full package-level analysis. We at Reconfigure would rather goblin be something small that doesn't do everything rather than something big that does everything poorly.

Hope this helps, and thanks very much for your attention on this issue! I hope to start fixing this over the holidays: it's not hard, it's just tedious.

lanzafame commented 7 years ago

@patrickt Thanks for the apology, I understand the balancing act you are performing and appreciate that you have open sourced goblin.

Regarding goblin 0.2, has the JSON output that you want to achieve been defined? And if it has, are you in a position to elaborate on it?

patrickt commented 7 years ago

@lanzafame I appreciate the kind words. I haven't decided on an output format for goblin 0.2 yet—that should come this week—but I can guarantee that it will be regular enough to be specified by a JSON Schema. (Regularity in output is very important to us, as we use Haskell downstream to parse goblin output, and our Haskell code yields a lot of power by expecting a very regular, well-typed AST.)

achudnov commented 7 years ago

@patrickt, is the gobling-JSON to Haskell parser open-source? This is what I'm actually interested in, and what I was planning to write a JSON schema for (see issue #32).

patrickt commented 7 years ago

@achudnov Not at the moment, but it's definitely something that has crossed our mind. The fact that you're interested will probably inspire us to polish it and get it out the door.

patrickt commented 7 years ago

This is done; just have to fix the tests.