SomMeri / less4j

Less language is an extension of css and less4j compiles it into regular css. Less adds several dynamic features into css: variables, expressions, nested rules, and so on. Less was designed to be compatible with css and any correct css file is also correct less file.
146 stars 47 forks source link

extract() issue #338

Closed twhoff closed 8 years ago

twhoff commented 8 years ago

I've been running into a weird issue with the extract() method getting confused about what it should be extracting... The code I'm writing works fine in the Node compiler, but in LESS4J I get an error.

Sorry for the long example, but it's hard to replicate with a basic example.

LESS

.wrapper {
    .component {
        .grid {
            .test-data {
                .in-1(20px; col-1, 100px; col-2; col-3, hidden; col-4; m);
            }
        }
    }
}

.impl(@error) {
    there-was-an-error: @error;
}
.impl(@fixed-cols; @fluid-cols; @total-fixed-width) {
    //counter: @counter;
    fixed-cols-@{counter}: @fixed-cols;
    fluid-cols: @fluid-cols;
    total-fixed-width: @total-fixed-width;
}

.in-1(...) {

    @gutter-x: extract(@arguments, 1);
    gutter-x: @gutter-x;

    .loop(@counter; @args; @fixed; @fluid; @tfw) when (@counter > 1) {

        @col-args: extract(@args, @counter);

        // Is fixed
        .is-fixed-column(@col-args);
        .is-fixed-column(@col-args) when (length(@col-args) > 1) {

            @col-width: extract(@col-args, length(@col-args));

            .is-fixed-column-inner(@col-width);
            .is-fixed-column-inner(@col-width) when (ispixel(@col-width)), (isem(@col-width)) {
                .loop(@counter - 1; @args; (@fixed + 1); @fluid; @tfw + @col-width);
            }
        }

        // Is fluid
        .is-fluid-column(@col-args);
        .is-fluid-column(@col-args) when (length(@col-args) > 1) {

            @col-width: extract(@col-args, length(@col-args));

            .is-fluid-column-inner(@col-width);
            .is-fluid-column-inner(@col-width) when (ispercentage(@col-width)) {
                .loop(@counter - 1; @args; @fixed; @fluid + 1; @tfw);
            }

        }
        .is-fluid-column(@col-args) when (length(@col-args) = 1) {
            .loop(@counter - 1; @args; @fixed; @fluid + 1; @tfw);
        }

        // Is other
        .is-other-column(@col-args);
        .is-other-column(@col-args) when (length(@col-args) > 1) {
            @col-type: extract(@col-args, length(@col-args));

            .is-other-column-inner(@col-type);
            .is-other-column-inner(@col-type) when (iskeyword(@col-type)) {
                .loop(@counter - 1; @args; @fixed; @fluid; @tfw);
            }
        }
    }
    .loop(@counter; @args; @fixed; @fluid; @tfw) when (@counter = 1) {
        .impl(@fixed; @fluid; @tfw);
    }
    .loop(length(@arguments) - 1; @arguments; 0; 0; 0);
}

Node compiler output

.wrapper .component .grid .test-data {
  gutter-x: 20px;
  fixed-cols-1: 1;
  fluid-cols: 2;
  total-fixed-width: 100px;
}

LESS4J

LessEngine2Message.LESS_COMPILE_ERROR Failed to compile less content. Could not compile less. 1 error(s) occurred:
ERROR 5:43 Wrong argument type to function 'extract', expected LIST_EXPRESSION saw identifier.
4: .test-data {
5: .in-1(20px; col-1, 100px; col-2; col-3, hidden; col-4; m);
6: }

Using the debug console, I can see that it's failing because it thinks col-2 is a list, it has this notation: [col-2COMMA1] - it seems to be something to do with the way extract is resolved: extract(extract(2, 1), 1) - something like that?

SomMeri commented 8 years ago

I will have a look at this, thank you for report. Hopefully it will be just a small bug, I hope. In case you find smaller example, let me know please.

SomMeri commented 8 years ago

Minimized example:

test {
  @value: identifier;
  extract-identifier: extract(@value, 1);
}
twhoff commented 8 years ago

Hmm, I didn't notice there was a difference between Node LESS and LESS4J here - but this does actually work in Node and not in LESS4J (as LESS4J expects list but gets identifier) - however - I don't think that this is the same issue as what I'm experiencing.

In my case, it seems to be a problem with the way LESS4J is treating the list evaluation - it appears to be something to do with nested extract() statements causing LESS4J to see a list when it should actually see an identifier.

In the example: .in-1(20px; col-1, 100px; col-2; col-3, hidden; col-4; m); - col-2 should be treated as an identifier, but it is being treated as a list.

The best way I can explain the logic is like this:

LOOP THROUGH parameters_list
    @parameter: extract(parameters_list, index);

    // LESS4J & NODE LESS both see @parameter = extract(parameter_list, index)
    // NOT @parameter = col-2 - but LESS4J doesn't evaluate this when calculating length for 
    // mixin guard expression 

    // OUTPUT
    // Node LESS
    LENGTH(@parameter) OUTPUT -> 1

    // LESS4J
    LENGTH(@parameter) OUTPUT -> 1

    // MIXIN GUARD EVALUATION
    // Node LESS
    WHEN (length(@col-args) > 1) -> LENGTH IS 1 -- GUARD IS FALSE

    // LESS4J
    WHEN (length(@col-args) > 1) -> LENGTH IS 2 -- GUARD IS TRUE

The problem lies in the mixin guard evaluation. (I think)

Then, when it reaches this point: @col-width: extract(@col-args, length(@col-args));

LESS4J evaluates it like this:

// @col-args = col-2
// LENGTH @col-args now evaluates correctly to 1

@col-width: extract(extract(@col-args, length(@col-args)), length(@col-args));

-> @col-width: extract(col-2, 1);

--> [col-2COMMA1]

That's my theory ;)

twhoff commented 8 years ago

I should also mention, the way I've been getting around the issue is by scoping the extract() call within a separate mixin like this:

.loop(@counter) when (@counter > 1) {

    // Bubbles @-col-args into scope
    .extract-col-args(@col-args; @counter);

    // This now correctly evaluates length in mixin guard to 1 - so this mixin doesn't execute
    .is-fixed-column(@-col-args); // Return value is passed to mixin as parameter
    .is-fixed-column(@col-args) when (length(@col-args) > 1) {
        // Do stuff
    }
}

.extract-col-args(@list; @index) {
    @-col-args: extract(@list, @index);
}

Hope this helps!

SomMeri commented 8 years ago

In my case, it seems to be a problem with the way LESS4J is treating the list evaluation - it appears to be something to do with nested extract() statements causing LESS4J to see a list when it should actually see an identifier.

The problem is that extract function in less4j was implemented to throw error when argument was not list. The error you run into happens because extract seen identifier instead of list. When I made it accept identifier, your code started to work too :). I need 15 more minutes max till I can do commit.

In any case, I will make release today and then you can test it all in the context. If it still throws an error after, let me know.

SomMeri commented 8 years ago

Released, you can try it.

twhoff commented 8 years ago

Thanks, I will try! - will need to wait for Maven release 1.17.2

SomMeri commented 8 years ago

Maven should be available by now.