Get out of the infinite loop by allowing to scan backwards when the current token is a comment.

[RaymondLim]

This fixes issue #9002.

[Mark-Simulacrum]

Can confirm that this fix works, just tested using @RaymondLim test case from #9002 - on Ubuntu 64bit 14.04.

[peterflynn]

@RaymondLim Could we add a unit test for this?

[RaymondLim]

Sure! It should be easy as the issue is caused by having the cursor right after { or } inside a comment.

[peterflynn]

I wonder if it would be more clear to structure it like this:

if (ctx.token.type !== "comment") {
    if (ctx.token.string === "}") {
        // (same as master)
    } else if (ctx.token.string === "{") {
        // (same as master)
    } else {
        // (same as master, plus...)
        if (!TokenUtils.movePrevToken(ctx)) {
            break;
        }
    }
} else {
    if (!TokenUtils.movePrevToken(ctx)) {
        break;
    }
}

This makes it a little more obvious how we guarantee that the loop makes progress every iteration in all cases, and it eliminates the if statement in this diff, which I think has somewhat tricky to follow logic since it's basically covering two independent cases.

[peterflynn]

Actually, an even nicer improvement would be to change while (true) { } to do { } while (!(ctx.pos.line === 0 && ctx.pos.ch === 0)). This way we wouldn't need the manual break calls after each movePrevToken(), and we get a little more safety in case this code changes more in the future.

[RaymondLim]

@peterflynn do { } while (!(ctx.pos.line === 0 && ctx.pos.ch === 0)) logic is not the same as the current logic which stops as soon as hitting a { or } for a normal css file, or a } in the top level for a preprocessor file. So I'll use your prior suggested code.

[peterflynn]

@RaymondLim I didn't mean to remove the break statements in those cases -- just remove the break statements that go with the movePrevToken() calls. That should preserve the existing behavior precisely.

[RaymondLim]

Fixed.

[marcelgerber]

It looks like these unit tests pass on master, too. (While they shouldn't)

[RaymondLim]

@marcelgerber You will get into infinite loop when running these unit tests.

[peterflynn]

@marcelgerber is correct: the test passes even on master, without the fix -- it doesn't get into the infinite loop. To hit the bug, you need a comment token whose value is exactly equal to "{" or "}", and afaik that can only happen when the { or } is alone on a line. In the offsets.css testcase file, the braces are together on a line with other text instead: /* p { color: "black" } */.

This is a good reminder of why it's always important to run testcases with your fix disabled to verify that the testcase itself is covering what you want it to cover.

[peterflynn]

@RaymondLim Done re-reviewing. We need to fix the test for sure. The loop cleanup I think would be nice, but it's not critical -- so it's up to you if you want to bother with that change or not.

[RaymondLim]

@peterflynn and @marcelgerber Thanks for catching the unit test issue. I might have tested with my own commented rule and later switched to use the existing one without retesting.

[RaymondLim]

@peterflynn Changes are pushed. Ready for final review (I hope).

[peterflynn]

Looks good -- thanks @RaymondLim!