Quotes in C-API Strings

[chriseppstein]

LibSass's SassScript string values include the quotes. This makes implementing Sass's string semantics in a consistent way very awkward.

Sass treats all strings as equal regardless of whether they have quotes or have used single or double quotes. But stringA.getValue() != stringB.getValue() if stringA is "foo" and stringB is foo. because the value of stringA includes the double quotes.

Instead the string's value should omit the quotes and there should be a string.getQuote() method that either returns a boolean or the quote character.

[mgreter]

I guess you're talking about sass_values (C-API)? Can you give a short code example, because I'm not sure I understand the problem correctly!?

[chriseppstein]

@mgreter Right. As you can see in the source code: https://github.com/sass/libsass/blob/master/sass_values.cpp#L39-L42

A string only has a value. So quoted strings include the quotes in the value. But since the semantics of Sass are that strings are equal regardless of whether they are using single, double, or no quotes. As such, the value object should not include the quotes in the value of the object. This will prevent native function code interacting with the sass values from having to remember to disregard the quotes in equality checks, hash values, etc.

[mgreter]

IMO it depends from where the string comes from. I.e. in perl-libsass I can put a perl-string into a sass_string value, and I don't want it to unquote the value everytime I do this. So IMO it's correct for sass_values to not be unquoted by default. But IMO this depends on your application logic. To be clear: if I have a programatically allocated string, I don't want it to be unquoted when used as a Sass_String value. The C-API exposes the quote/unquote functions, so you can use them if you need to unquote before creating a Sass_Value. IMO it doesn't matter if single or double quotes are used, as this will only be preserved on static values. Quote will automatically chose the best quotemark.

BTW. libsass internally always unquotes strings in the parsing state and only quotes them, when emitting previousely quoted string (String_Quoted vs String_Constant). I agree that the current C-API does not expose this information (if a string was previousely quoted or not).

[HamptonMakes]

@mgreter I don't understand why the perl value would come with quotes in the first place?

It seems like we definitely keep that quoted value in the actual strings... shouldn't be hard to expose that to the API, right?

libsass/ast.hpp

Line 1442 in c8885dc

ADD_PROPERTY(char, quote_mark);

Internally, we use that all over the place... https://github.com/sass/libsass/search?utf8=%E2%9C%93&q=quote_mark

[chriseppstein]

So IMO it's correct for sass_values to not be unquoted by default.

No, this is wrong. The value of Sass string should have the same equality semantics as the string has in sass itself. Without this, client code will consistently break expected semantics by accidentally considering the quotes as part of the value of the string instead of an aspect of how it is output into the css file.

and I don't want it to unquote the value everytime I do this

Agree. The client code should not include the quotes in the value set into the object.

It's not hard to do the unquote operation. It's hard to know you should. It's easy to forget and break sass's semantics.

[mgreter]

I'm pretty confused because I don't know if you are talking about strings comming from an AST_Node or by creating one via the C-API. IMO the ones from the parser (AST_Node) will be unquoted already, but the C-API does not unquote it by itself. IMO if we unquote strings in the C-API when Sass_Strings are created, we need to quote anything we want to be taken literally (we may have another quoted string inside a quoted string which will get further unquoted with every new Sass_String instance).

@hcatlin yep, we keep that one around, altough we (AFAIR) only use it in a boolean context. But yes, should be pretty easy to add and expose this flag at the C-API level.

@chriseppstein I hope I got your point now. You cannot set if the Sass_String you create should be quoted or not when emitted. That should be solved by adding the flag mentioned above (so you could set it to have libsass add quotes when emitting the string). I agree that this was overlooked and it starts to make sense what you have written above and your reference to the output seem to make it clear!

[HamptonMakes]

I'm assuming that he's trying to use the C-API to define a function like compare("a", a). In the wrapper code, the Sass_Value that's sent back those values is "\"a\"" and "a" respectively, meaning that comparing them as equivalent would require implementing unquote. Where, we could send back value as "a", and then each Sass_Value would also have a boolean property called quoted, so that you can include that in your logic if required... because maybe we do want to react to them differently. But, most of the time, I would be surprised if I were writing an integration library and I got back a string value as "\"a\"".

Obviously, if we are passing a Sass_Value back to the interpreter, then it doesn't matter and libsass is dealing with the details for us, so either way is the same.

I definitely agree with @chriseppstein on this one... since we really want it to be easy to take values from Sass->Lib.

[chriseppstein]

👍

[mgreter]

@hcatlin if you define such a compare function and call it via scss code, the parser will unquote "a" to a, so it should already do that. I still don't see the actual point, beside not beeing able to tell libsass that it should quote a string when emitting it in the resulting css (which I do consider a bug).

I just checked this is true, and the ast debug for compare("a", a) looks as this:

Function_Call 0x268dc80 (0@[5:9]-[5:16]) [compare]
 args: Arguments 0x2bf9b40 (0@[5:9]-[5:16])
 args:  Argument 0x268db60 (0@[5:17]-[5:20]) [0x293cbc0] [name: ]  [rest: 0]  [keyword: 0]
 args:   value: String_Quoted : 0x293cbc0 (0@[5:17]-[5:20]) [a] {qm:*} <>
 args:  Argument 0x268dbf0 (0@[5:22]-[5:23]) [0x293cc40] [name: ]  [rest: 0]  [keyword: 0]
 args:   value: String_Quoted : 0x293cc40 (0@[5:22]-[5:23]) [a] <>

The interesting part is [a] {qm:*} vs [a], which shows the values we keep internally.

[HamptonMakes]

@mgreter I haven't repro'd what Chris says, but it sounded to me like the Sass_Value passed in did include the quotes, maybe re-attaching them with getValue()?

Chris, can you show what you mean in some code that we can write a test based on?

[chriseppstein]

Ok. It turns out I was mistaken about what was happening here. The quotes from the sass value are indeed stripped off -- this is a good thing. But this API is definitely broken.

Using node-sass I wrote a test case like this:

  it("should not change whether strings passed through are quoted", function (done) {
    var input = "#test1 { uquoted: testing(foo); }\n" +
                "#test2 { squoted: testing('bar'); }\n" +
                '#test3 { dquoted: testing("baz"); }\n';
    var options = {
      data: input,
      functions: {
        "testing($value)": function(value) {
          console.log(value.getValue());
          return value;
        }
      }
    };
    var expected = "#test1 { uquoted: foo; }\n" +
                   "#test2 { squoted: 'bar'; }\n" +
                   '#test3 { dquoted: "baz"; }\n';
    testutils.assertCompiles(options, expected, done);
  });

I get the following failure:

Actual:
#test1 { uquoted: foo; }
#test2 { squoted: bar; }
#test3 { dquoted: baz; }

Expected:
#test1 { uquoted: foo; }
#test2 { squoted: 'bar'; }
#test3 { dquoted: "baz"; }

And the console prints:

foo
bar
baz

So yes, the quotes are not passed from Sass to the host language. But as @mgreter said, there's no way to return a quoted string back unless you put quotes in the value itself. Furthermore, passing a value through should not change it like it does here.

[HamptonMakes]

Cool, then it looks like @mgreter's patch should fix this... because we can now pass the quoted-ness both directions in a civilized manner (and maintaining the original, if you don't change the Value)

[chriseppstein]

Agree. This fixes my issues. Thank you @mgreter ❤️!

[mgreter]

Glad we could figure this one out. I was pretty sure we had a misunderstanding somewhere. I'm currently considering, if we really need this option and use autodetection only. At least I'm going to do this in perl-libsass, as keeping this flag around, introduces problems on the host language side, since I want to have this Sass_Value beeing represented as a native perl string (which has no "native" way to attach this flag). There would be ways to do it (Tie'ing the scalar i.e.), but that would hurt performance quite a bit. IMO it should be enough for my implementation to check if quotes are needed and set the flag automatically when I pass a string back from perl to libsass. This should probably work well enough, so the API can IMO stay that way (I probably will expose another function to check if quotes can be omitted for a given string, completing the quote/unquote function family). I've choosen to add a new constructor sass_make_qstring since we cannot easily add a flag the existing sass_make_string function, since it would break the API. If you have any wishes for refactorings in this regard, feel free to post them here or in its own issue and I will consider it before the next major upgrade is due.

[chriseppstein]

I read the API changes, I'm not a direct consumer but it seems good to me.

https://github.com/sass/sass/blob/stable/lib/sass/script/value/helpers.rb is the nicest ruby sass API I could devise and it also provides different creation methods for unquoted and quoted strings.

[mgreter]

@chriseppstein I think I know what's happening in node-sass. The API has changed in this regard and I also forgot to adjust for it in the lastest perl-libsass release (already preparing next update). As I've written, the parser now unquotes the string if needed, so internally the representations are the same. Previousely I had an unquote step built into my bindings when converting strings between host languages, not sure if node-sass had something similar.

Also for your example I would argue that

Expected:
#test1 { uquoted: foo; }
#test2 { squoted: bar; }
#test3 { dquoted: baz; }

Is also a valid result. From my findings, we also only preserve the original quotemarks if the string involved is a static value, which is IMO never true for function arguments. As a side note, it only uses single quotes if the string contains double quotes, but no other single quote. See this example:

@function test($a) {
  @return $a;
}
div {
  foo: test(a);
  foo: test('a');
  foo: test("a");
  foo: test("\"");
}

Which compile to this with ruby sass:

div {
  foo: a;
  foo: "a";
  foo: "a";
  foo: '"';
}

[chriseppstein]

@mgreter If you meant:

Expected:
#test1 { uquoted: foo; }
#test2 { squoted: "bar"; }
#test3 { dquoted: "baz"; }

Then we agree.

[mgreter]

I agree that this would be the best way, but IMHO both are semantically the same in css context, but maybe I'm wrong? As I said, to get this behaviour would make it much more complex for me and perl-libsass, since I would need to attach the is_quoted flag internally to native perl strings, which adds a lot of complexity. We also have to choose when to retain this flag i.e. on concatenation. We would also need to upgrade the flag, once we introduce spaces etc. That's why an automatic way sounds most appealing to me, as long as the result is still valid css. Hope I got my point accross.
And to make it clear, this is perl-libsass specific behavior I'm talking now!
An automatic way really just solved a lot of complex things "out" of the box for me!
In node.js you might be able to extend the String object to carry that flag around ...

[chriseppstein]

Sass only unquotes strings when they are output via interpolation. Our policy has been to let the author decide when a string is quoted or not, any other behavior requires Sass to have intimate knowledge of what the css context is and that is a direction we actively avoid for future compatibility reasons.

As I said, to get this behaviour would make it much more complex for me and perl-libsass, since I would need to attach the is_quoted flag internally to native perl strings, which adds a lot of complexity.

You can make things as simple as they are, but not more simple. This is essential complexity.

We also have to choose when to retain this flag i.e. on concatenation.

Sass's handling of string concatenation (the + operator) keeps the quoted state of the string on the left side.

We would also need to upgrade the flag, once we introduce spaces etc.

Not even Sass does this.

$ echo "div{ something: a + 'b c'; }" | scss
div {
  something: ab c; }

That's why an automatic way sound most appealing to me if the result is still valid css.

I get why you think it's appealing, but in my experience dumb code with obvious rules is easier for users to work with than smart code when the smart code cannot be guaranteed to be correct.

[mgreter]

That's why I asked :) Two new things I learned with your last comment. Somewhat counter intuitive for me, but that's why I like to see code examples, makes it much easier to understand the issues at hand (specially for non native english people)! I will see if and how I can add this to perl-libsass, since the concatenation rules obviously behave different than automatically adding quotes would.

BTW. I wonder why node-sass hasn't choosen to use the native js data-types for most sass values, since with Null, Boolean, Number, String, Array, Object and Error they seems to have everything ready to be converted to native data types ... so far I only had to differentiate between two list types, which basically only differ by beeing separate classes, inheriting from the base array implementation (in perl-libsass). Now with the quotemark I probably will do the same, since the type of the class is something I can attach easily to the native data type, if I use a reference. In a real object oriented language I would expect to have specific sass value classes, that inherit from the base object classes (and probably also from a base sass_value class, if polymorphysm is possible).

[HamptonMakes]

I'm not a fan of making units seem natural between two languages that are interop, when they aren't. I'm fine mutating on something like SassValue as a thin wrapper on the C-API. It seems to kind of put things in a corner.

[chriseppstein]

@mgreter They do expose a js class for each type but those classes are written in c++ because it allows them to keep a hidden reference to the libsass struct and to use it as a backing store for the objects' values. It's actually how JS is meant to be used in an embedded way like this.

[chriseppstein]

@hcatlin units? you lost me.

[mgreter]

@chriseppstein what I mean is I don't understand why a Sass_String is not really a js String object, which would then natively behave correctly for equal or concat operations AFAIK (more on that below). It looks to me like you get an instance of an object, where you have to call getValue(), or am I missing something? I would expect to get a native data type there, IMHO this might be the only way to get convenient and native behavior as you IMO written in another issue. IMO you currently would need to call a.getValue() == b.getValue(), but if a and be would be a native instance (or something that directly inherits from it if possible), you should be able to write a == b. IMHO node-sass will probably hit a wall with numbers, since it adds a units, which cannot be represented easily by any native data type. Also at some point you may have to pass two values to another library, which does the compare operation. Therefore I've opted for perl-libsass to "delegate" any deep equality checks to the "toString" function (all overloaded equal ops check if a.toString() == b by default, implemented in base class).

Maybe give it a thought. If that's technically achievable, I would prefer it that way. Otherwise you probably will not get around to use syntax like a.equals(b). In perl-libsass I implemented both worlds. You can work with the explicit objects, but you can also return native types from sass functions. All objects overload operators, so they behave like native data types, which you IMO only get via prototype overloading in node.js, but maybe also via prototype inheritance. Really blunt native data types (created outside the sass context) get automatically upgraded to certain sass types. This basically means that js arrays become comma separated lists and strings will probably have their quote flag automatically set, since we cannot get any css context out of a native string object.

Since I talked about code examples, I would like to be able to return this from custom functions:

return null; // sass_make_null();
return 2; // sass_make_number(2, 0);
return false; // sass_make_boolean(false);
return 'a b'; // sass_make_qstring("a b"); => autodetect q(uote)mark
return { a: 1, b: 2 }; // sass_make_map(..); => recursive resolve keys, values
return ['a', 3, false, '2px']; // sass_make_list(comma) => recursive resolve values
// this might be against core sass principles?
return '2px'; // sass_make_number(2, 'px')
return '#000'; // sass_make_color(0, 0, 0, 1)

I hope you agree with most parts!? IMO node-sass will probably not be able to support native stuff like c = a + b, since operator overloading doesn't seem to be available for custom objects. But node-sass should at least be able to use native hash-maps (js Objects). There is only the question what meta information keys have to retain (IMO the native implementation really just can store a string, but what about a list as a key?). I'm pretty sure the rule is also a bit different in ruby sass here. Since IMO the map-keys are in the context of sass, we could apply automatic quotes for serialized output, same goes probably for the value itself? I also guess it's different with lists again, since they do appear in css context, and we know that libsass has some open issues with handling certain lists (specially list without whitespace between elements).

Sorry for the long post. In the end it got more like a brain-storming, but it documents quite a bit of the sass_value internals from the perl-libsass implementor perspective. Let's see how node-sass can improve in this area and I hope they can manage to bring it to a usefull and good level! But I somehow believe that it is not possible to support a + b in node.js, while respecting the quotemark rules you established a few comments above.

[xzyfer]

Sass_String::is_quoted has been exposed in 3.2.1

[chriseppstein]

@mgreter The semantics of the strings are different, they cannot be the same type. Specifically, the equality semantics of quoted vs. unquoted strings. Also it would be very strange API to sometimes pass a native type and sometimes pass a wrapper type. It's much more consistent to always pass a sass value that can be unwrapped. This would allow us to implement common methods that are part of a Sass Value contract API.

[xzyfer]

Am I correct in understanding this issue is now solved 3.2.1?

[mgreter]

IMO this issue here can be closed. It has been noted that we currently don't expose enough functionallity to implement all sass value related tasks in bindings neatly. It doesn't make much sense to hack all the features together from existing code, since it is scattered around our code base (and even using different input types, ast_nodes vs sass_values). So a refactor seems to be the right way to solve this correctly, which I cannot give any ETA (I currently plan to do this as one major change for 3.3)

To be clear: Bindings should be able to implement all OPs themselve, the information is there, but it makes a lot of sense to let libsass do the whole work (so you get the same logic as libsass uses).

[am11]

@mgreter, could you please explain when should we use sass_make_qstring?

Should we always expect LibSass to callback the CustomFunction with verbatim string (with inclusive quotes as is) and just use sass_make_string in wrapper? If this is the case, what is the use of sass_make_qstring?

Or is it so that we should inspect the string values returned by consumer in wrapper code and then call sass_make_string or sass_make_qstring based on whether the returned value has single or double quote within? Or always use sass_make_qstring variant as there is no point of not use verbatim strings.