Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decimal litterals are truncated #923

Closed
npiguet opened this issue Mar 6, 2015 · 7 comments
Closed

Decimal litterals are truncated #923

npiguet opened this issue Mar 6, 2015 · 7 comments
Assignees
Milestone

Comments

@npiguet
Copy link

npiguet commented Mar 6, 2015

I'm currently working on libsass-maven-plugin, and I've encountered a strange behavior. For some reason all decimal litterals in my .scss file are truncated.

Here is an example of .scss code that causes the problem for me:

.sqw-contextMenu {
    li {
        a {
            padding: 0 0.8em 0 0.8em;
        }
    }
}

The code that is generated is the following:

sqw-contextMenu li a {
    padding: 0 0em 0 0em;
}

I'm obviously doing something wrong when calling sass_compile_file(), I have no idea what.

Before calling sass_compile_file(), I create obtain a sass_file_context via sass_new_file_context(), then set the options as needed. All the options seem to work fine.

The only problem I have is the one outlined above. Setting the precision field of the sass_options struct seems to have no effect.

I'm using version 3.1 of libsass. commit 31521ef to be precise.

Do anyone have an idea if I'm doing anything wrong, or is that a bug in the library.

@mgreter
Copy link
Contributor

mgreter commented Mar 6, 2015

a) No idea what you're doing wrong and b) no, sorry, works correctly here!
By the way you could have checked this via http://sassmeister.com/ yourself!
Just make sure you select the libsass engine under options!

@mgreter
Copy link
Contributor

mgreter commented Mar 9, 2015

Going to close this as cannot reproduce.

@npiguet
Copy link
Author

npiguet commented Mar 9, 2015

I just managed to get a little more information to locate my error: It seems that the truncation of the numbers happens at parse time (as opposed to output time), because of the two following things:

  • the value of precision has no effect on the output
  • the value of fractions are calculated based on the truncated version of the number (example below)
.something {
  margin: (4.5/9);
}

is compiled to

.something {
  margin:0.44444 }

when the value of the margin should clealy be 0.5, and not 0.4444 (Which by the way is equal to 4/9, so the 4.5 value was first truncated to 4 and then the fraction was calculated based on the truncated number.)

I can't figure out where the problem could be coming from by looking at the libsass source code. Could I be missing some compiler flag, or could I possibly have a version of some library (maybe the one that is used to convert strings to numbers) that is not what libsass expects?

@npiguet
Copy link
Author

npiguet commented Mar 9, 2015

Actually, I just found the source of the problem and it is indeed a bug in libsass. In version 3.1.0 it occurs at eval.cpp:627

 case Textual::NUMBER:
        result = new (ctx.mem) Number(t->path(),
                                      t->position(),
                                      atof(num.c_str()),
                                      "",
                                      zero);

the problem here is that atof is locale dependent. On my system, the locale is the following:

$ locale
LANG=en_US.UTF-8
LANGUAGE=en_US
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC=fr_FR.UTF-8
LC_TIME=fr_FR.UTF-8
LC_COLLATE="en_US.UTF-8"
LC_MONETARY=fr_FR.UTF-8
LC_MESSAGES="en_US.UTF-8"
LC_PAPER=fr_FR.UTF-8
LC_NAME=fr_FR.UTF-8
LC_ADDRESS=fr_FR.UTF-8
LC_TELEPHONE=fr_FR.UTF-8
LC_MEASUREMENT=fr_FR.UTF-8
LC_IDENTIFICATION=fr_FR.UTF-8
LC_ALL=

Specifically, LC_NUMERIC specifies the french locale. In the french locale, the decimal separator is , not .. Therefore, atof stops parsing the number when it encounters ., because . is not a valid character for numbers in the french locale.

There are multiple other usages of atof that would also need to be fixed.

I think this issue can be re-opened now.

@mgreter mgreter reopened this Mar 9, 2015
@mgreter
Copy link
Contributor

mgreter commented Mar 9, 2015

Can you try the fix I have pushed to my current WIP branch!
I did not test it myself, but I guess it should solve your issue!

@mgreter mgreter added this to the 3.2 milestone Mar 9, 2015
@mgreter mgreter self-assigned this Mar 9, 2015
@mgreter mgreter closed this as completed in 14bfd99 Mar 9, 2015
@npiguet
Copy link
Author

npiguet commented Mar 9, 2015

By the way, the best way to reproduce this reliably on any system for me was to modify sassc.c as follow:

  • add #include <locale.h>
  • add setlocale(LC_ALL, ""); as the first line of the main() function.

Then run

make -C libsass; make -C sassc
export LC_ALL="fr_FR.utf8"
sassc/bin/sassc test.scss

where the contents of test.scss was

.something {
        margin: 2.3px;
}

@mgreter
Copy link
Contributor

mgreter commented Mar 9, 2015

Can you confirm that the latest master fixes this issue? If not I will have to take deeper look and test this more thoroughly. I just "hope" the fix I've added does what it should (and if it doesn't, it should be easy to correct it once and for all)!

npiguet pushed a commit to npiguet/libsass that referenced this issue Mar 10, 2015
Fixes sass#923

Conflicts:
	eval.cpp
	util.cpp
	util.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants