microsoft / pxt-arcade

Arcade game editor based on Microsoft MakeCode
https://arcade.makecode.com
MIT License
484 stars 210 forks source link

hoisted variable name while decompiling conflicts with namespace #1463

Open jwunderl opened 5 years ago

jwunderl commented 5 years ago

Describe the bug

When a variable is hoisted to global scope while decompiling, it can conflict with namespace names

To Reproduce Steps to reproduce the behavior:

  1. Go to https://makecode.com/_HquY2D9C7gXc in beta
  2. decompile to blocks
  3. See error
game.onUpdate(() => {
    const light = 5;
    console.log(light)
})

or

if (true) {
    let light = 5
}

Expected behavior

shouldn't break; gets renamed to light2

Additional context

naming a variable light in blocks doesn't get renamed as well with the same error message / you can repro this bug with just blocks

really needs a better error message than "Cannot assign to '{}' because it is not a variable."

It took me a minute to recognize what the error was, because I didn't realize we had a light namespace - it looks like it only has light.sendBuffer exported?

FranklinWhale commented 5 years ago

microsoft/pxt#6155 will fix this issue by not hoisting the variable to global.

To verify:

  1. Go to https://franklinwhale.github.io/pxt-microbit-test/#editor
  2. Switch to JavaScript
  3. Add custom.ts
  4. Paste the code below to custom.ts:
    namespace light {
    export function test() { }
    }
  5. Paste the code below to main.ts:
    if (true) {
    let light = 5
    basic.showNumber(light)
    }
  6. Switch to Blocks
  7. Switch to JavaScript
  8. Replace the content of main.ts with the code below:
    basic.forever(function () {
    let light = "T"
    if (Math.randomBoolean()) {
        basic.showString(light)
    } else {
        basic.showString("F")
    }
    })
  9. Repeat Step 6 and 7
jwunderl commented 5 years ago

Nice! There are three other 'sub-bugs' under additional context that should probably all be addressed as well (e.g. that creating a variable light in an on start should get renamed, not produce an error); if your PR gets merged / fixes the main issue I'll file those separately~

kevinjwalters commented 4 years ago

This ticket may be about another aspect of variable naming/renaming but I've noticed screen is problematic in Blocks, mentioned in MakeCode Forum: Reserved Words and confusing errors.