bublejs / buble

https://buble.surge.sh
MIT License
871 stars 67 forks source link

scope bug: read-only error #59

Closed kzc closed 6 years ago

kzc commented 6 years ago
$ bin/buble -v
Bublé version 0.18.0
$ cat scope_bug.js
const bar = "FAIL";
!function() {
    function foo() {
        --bar;
        bar = ["fail", "PASS", "Fail"][bar];
    }
    var bar = 2;
    foo();
    console.log(bar);
}();
$ cat scope_bug.js | node
PASS
$ cat scope_bug.js | bin/buble 
---
1 : const bar = "FAIL";
2 : !function() {
3 :     function foo() {
4 :         --bar;
            ^^^^^
bar is read-only (4:8)

Related sources:

src/program/types/AssignmentExpression.js#L7-L9 src/program/types/UpdateExpression.js#L7-L11

adrianheine commented 6 years ago

My first guess is that we can only improve this by delaying validation of update and assignment expressions till after we traversed the whole tree. A simpler alternative would be to turn this error into a warning.

kzc commented 6 years ago

I think the former is the way to go. A warning with false positives is not helpful.

Acorn catches a number of these types of errors doesn't it? If so, maybe this logic is not required at all.

adrianheine commented 6 years ago

Well, I think a warning would be a reasonable approach, exactly because it could be a false positive. Also, a) it is easily fixable b) it is confusing for humans, so fixing it makes sense anyway. But I'm not opposed to the more elaborate and correct solution.

As far as I know acorn doesn't do anything like this, for example it parses const x = 1; ++x.

kzc commented 6 years ago

The problem is that warnings are almost always ignored by users - even if valid. What's the downside of passing illegal code to Buble as you have above? The generated code will incorrectly behave as if the const is a let?

adrianheine commented 6 years ago

Yes, it will be transpiled to a var. Since most people never run their untranspiled code, this would effectively mean that they can theoretically use const in all kinds of wrong places, ignore the warnings and have incorrect assumptions about their code.

kzc commented 6 years ago

That doesn't sound desirable - particularly for users like me who never use linters.

adrianheine commented 6 years ago

I actually found an easy solution for this: The checks used to happen in initialise, now I moved them to transpile. I also added them to destructuring assignments.

kzc commented 6 years ago

@adrianheine Well done! Thanks.