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

scope bug: read-only error #59

Closed
kzc opened this issue Nov 21, 2017 · 8 comments
Closed

scope bug: read-only error #59

kzc opened this issue Nov 21, 2017 · 8 comments
Labels

Comments

@kzc
Copy link
Contributor

kzc commented Nov 21, 2017

$ 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 adrianheine added the bug label Dec 7, 2017
@adrianheine
Copy link
Member

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
Copy link
Contributor Author

kzc commented Dec 7, 2017

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
Copy link
Member

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
Copy link
Contributor Author

kzc commented Dec 7, 2017

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
Copy link
Member

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
Copy link
Contributor Author

kzc commented Dec 7, 2017

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

@adrianheine
Copy link
Member

adrianheine commented Dec 9, 2017

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
Copy link
Contributor Author

kzc commented Dec 9, 2017

@adrianheine Well done! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants