Fix parsing ast with array

[smacker]

Example of real AST (typescript):

      {
        "col": 1,
        "declarationList": {
          "col": 1,
          "declarations": [...],
          "end": 19,
          "flags": ["Const", "BlockScoped"],
          "kind": "VariableDeclarationList",
          "line": 1,
          "pos": 0
        },
        "end": 20,
        "flags": [],
        "kind": "VariableStatement",
        "line": 1,
        "modifierFlagsCache": 536870912,
        "pos": 0
      }

Now driver always return:

expected object of type map[string]interface{}, got: "Const"
	Writing fixtures/simple.ts.uast ...

Please, guide me how to fix it if current solution is wrong.

Thanks!

[juanjux]

Arrays with non-dicts are not processed by the normalizer thus the error. If the flags children where dictionaries you would have to add ot to the "PromotedPropertyList" setting in the ToNoder object in the driver so flags turns into a full children node of DeclarationLists. But I'm think that worked only for arrays of dicts (nodes). In this case, with the array values being just string, the right solution would be to do it using a ToNode callback in the ToNode object to convert them. Check: https://github.com/bblfsh/php-driver/blob/master/driver/normalizer/tonode.go

[smacker]

Something like this?

	Modifier: func(n map[string]interface{}) error {
		if flags, ok := n["flags"].([]interface{}); ok {
			var newFlags map[string]bool
			if len(flags) > 0 {
				newFlags = make(map[string]bool, len(flags))
			}
			for _, f := range flags {
				flagStr, ok := f.(string)
				if !ok {
					return fmt.Errorf("flag %+v isn't string", f)
				}
				newFlags[flagStr] = true
			}
			n["flags"] = newFlags
		}
		return nil
	},

What is your opinion about handling non-node arrays on sdk level? Maybe it should be presented in UAST the same as in original AST? Like:

.  .  .  .  .  Properties: {
.  .  .  .  .  .  flags: [Const, BlockScoped]
.  .  .  .  .  .  internalRole: declarationList
.  .  .  .  .  }

instead of

.  .  .  .  .  Properties: {
.  .  .  .  .  .  flags: map[Const:true BlockScoped:true]
.  .  .  .  .  .  internalRole: declarationList
.  .  .  .  .  }

[juanjux]

Could be done, but properties other than locations, internalType and roles are not part of the spec, so they could be lost to code analysis (at least not language specific ones). We could think of a way to incorporate them and do a proposal.

[smacker]

Hm. Actually from my point of view it looks more like a bug.

Consider AST to UAST:
{"langSpecific": "string"} -> {"Properties": {"langSpecific": "string"}}
{"langSpecific": 1} -> {"Properties": {"langSpecific": "1"}}
{"langSpecific": true} -> {"Properties": {"langSpecific": "true"}}
{"langSpecific": {"a": 1}} -> {"Properties": {"langSpecific": "map[a:1]"}}
{"langSpecific": ["a", "b"]} -> error

looks pretty inconsistent.

I updated PR to make it
{"langSpecific": ["a", "b"]} -> {"Properties": {"langSpecific": "[a b]"}}

[juanjux]

I only see now one problem with this PR: the Node.Properties field is of type map[string]string] (https://github.com/bblfsh/sdk/blob/master/uast/node.go#L32) so there isn't really a way to separate the way an array is serialized from a string starting with a [ since it exports both as the same string. Maybe Node.Properties should be a map[string]interface{} but I don't know if that will break JSON serialization (the ideal case would be that the JSON serialization exports a JSON array in that case, and a string in others).

If you've time, could you experiment with this? If not I'll play with this after I finish my current PR.

[smacker]

It shouldn't break anything. But I'll add some tests tomorrow to prove it. Thanks!

P.S. "map[..]" is weird in my opinion too

[smacker]

Oh. I see why it's map[string]string. You export it through protobuf as is.

I see 2 possible solutions:

1. Keep it as string and use function to convert value, something like:

func toPropValue(o interface{}) (string, error) {
	var v string
	t := reflect.TypeOf(o)
	switch t.Kind() {
	case reflect.Map, reflect.Slice, reflect.Array:
		b, err := json.Marshal(v)
		if err != nil {
			return "", err
		}
		return string(b), nil
	default:
		return fmt.Sprint(o), nil
	}

}

2. Change type to map[string]PropValue something like:

type PropValue struct {
	Value string
	Array []PropValue
	Map map[string]PropValue
}

And provide helpers to receive value. But it will require changing of proto uast consumers.

Please share your opinion! (better in separate issue #214)

---

But this PR actually isn't really about how you represent values internally or how do you pass them further.

This PR makes consistent input for sdk.
All types are valid except array and this PR makes array valid too because there is no good reason to make it special.

Please consider merging it.

P.S. I will be happy to send separate PR with change of serialization but it definitely requires more discussion.

[juanjux]

I'm merging, even trough the stringuized format is not very helpful as the linked issue show.

[smacker]

Thank you!

Value of this PR is driver developers don't need to make ugly hack like this: https://github.com/bblfsh/typescript-driver/blob/master/driver/normalizer/tonode.go#L27 just to be compatible with sdk

[juanjux]

Yes, but with regard to creating a driver, remember that something semantically important shouldn't be only represented as a property since then it can't have roles applied and such (I don't know enough typescript to know if that's the case there).