Implement SassScript Maps

[xzyfer]

This PR implements SassScript Maps and the API as of 3.3.14. There were changes to some map APIs in 3.4 none of which are implemented here.

I think this is about ready to start the feedback process. I'm not a c/c++ developer by trade so there's likely plenty of room for improvement.

TODO

• map parsing
• length
• map-get
• map-merge
• map-remove (3.3 syntax)
• map-keys
• map-values
• map-has-key
• keywords

Known issues

• @debug support - @debug support #223

Possible issues

• have I sourcemaped correctly?

list and map variable keyword arguments in same definition

I think this can be implemented as a separate feature. It's probably best suited to being shipped with call.

Spec

Specs for SassScript Maps have been added in sass/sass-spec#44. Theses specs admittedly focus primarily on the map-* functions and not the parsing of Maps.

/cc @akhleung

[xzyfer]

This error checking block is repeated a few times. Is there a better way?

      Expression* m = ARG("$map", Expression);
      Expression* v = ARG("$key", Expression);
      if (!(m->concrete_type() == Expression::MAP || m->concrete_type() == Expression::LIST)) {
        error("$map: is not a map for `" + string(sig) + "`", path, position);
      }
      Map* map = dynamic_cast<Map*>(env["$map"]);

[akhleung]

Hmm ... I assume that this function is supposed to accept lists and treat them as maps that have integer keys? If you need to handle both cases, I can't think of a good way to do the type checking that wouldn't involve some boilerplate ... I suppose you could write variants of ARG and get_arg that can take 2 or 3 different types, and check that the argument matches one of them....

Also, even though you check for both Maps and Lists, you seem to cast only to a Map afterward -- is this an omission?

[xzyfer]

Apologies, I'll clarify. The pattern with these function is they take a map, or an empty map () which is indistinguishable from a list, and is actually treated a list by the parser.

So in essence I'm just trying to determine an $map is infact a map with zero or more keys.

[xzyfer]

The caveat with this approach is that I'm not sure what'll happen with a list in fact passed in and coerced with Map* map = dynamic_cast<Map*>(env["$map"]);. Sass would return an error

$map: ("foo" "bar" "baz") is not a map for `map-get'

http://sassmeister.com/gist/ecfdc8072ab078cbdbc3

[akhleung]

Actually, now that I've consulted the Ruby Sass docs and code snippets, I'm not sure why you're checking if the $map argument might also be a list -- can't you just require it to be a map? Then you could just use

Map* m = ARG("$map", Map);

on the first line.

[xzyfer]

No worries, I appreciate the feedback. Generally speaking I think I'm doing mostly the right the things.

I'm just not sure whether it's the correct way to do them.

[xzyfer]

So the issue I ran into with

Map* m = ARG("$map", Map);

is that this

foo: map-get((), foo);

would resulting in

error: argument $mapofmap-get($map, $key) must be a map

because ARG thinks () is a list. I suppose the correct thing to do would be to update ARG to understand that () is both an empty list and map.

[akhleung]

It might be worth seeing if you can add a template specialization of get_arg (specialized for Map) that does some extra checking to see if the arg is an empty list, and if it is, then replace the arg with a newly-constructed empty map and return a pointer to that map. (If you try this, you'll need to modify get_arg to take a Context object so that the memory allocations can be tracked.)

[xzyfer]

Yep I agree that's the sane thing to do. I'll give it a go today.

[xzyfer]

Fixed in 57dada0

[HamptonMakes]

EEEEE!!!!

[xzyfer]

It'd be ideal @akhleung could get his feedback in before Sunday US time.
I have company allotted hack time to work on this coming up this Monday AU time (Sunday US time).

[akhleung]

Hey sorry for the delay -- I've been ill for the past few days. I'll review this and comment on it today!

[xzyfer]

This equality check is naive, and could potentially be a performance issue with the right inputs. Maybe this is ok for the first iteration?

An alternative approach would be to maintain a has for each map or key value pair for equality comparisons.

[akhleung]

That seems fine for a first pass; we can optimize later if it becomes a problem.

[am11]

@akhleung, it's good to have you back! 👍
Hope you are doing well. :)

[xzyfer]

I've updated the Map casting boilerplate with a get_arg specialised for maps as discussed. It's much nicer now 💯

I think this is pretty much good to go.

[HamptonMakes]

@akhleung can we have a special "Maps in LibSass" announcement for SassConf this week?

[akhleung]

Sure, I'll get this merged in time for SassConf.

[lunelson]

I'm getting syntax failure on the following.

$nest: (
  alpha: (
    beta: (gamma: delta),
    epsilon: 2 3 4,
    zeta: 'hi'
  )
);

.test {
  out: map-get(map-get(map-get($nest, alpha), beta), gamma);
  out: map-get(map-get($nest, alpha), epsilon);
  out: map-get(map-get($nest, alpha), zeta);
}

output with ruby sass 3.4.4 is the following:

.test {
  out: delta;
  out: 2 3 4;
  out: "hi";
}

Running sassc on the command line with this however, I get unclosed parenthesis error for line 4.