NetLogo / Tortoise

Compiler and runtime engine for NetLogo models that runs in JavaScript 🐢
https://netlogoweb.org
Other
56 stars 27 forks source link

Compiler issue: Defining a local variable called `world` would result in inability to set world variables #236

Closed CIVITAS-John closed 2 years ago

CIVITAS-John commented 2 years ago

Hi,

This is a bug found by a user in Turtle Universe (麦子 / Maizi).

It is critical.

To reproduce, simply try to do the following:

global [ test ]

to setup
  let world "anything"
  set test "everything"
end

He further reports that only world, tortoise_require, and workspace might be impacted since other names are properly escaped. The reason is that we use world.observer.setGlobal to set local variables, and once this is overridden by the user-defined local variable, the program would break.

Best, John Chen

CIVITAS-John commented 2 years ago

The proposed solution:

Add the world to the keyword list.

image

By Maizi, again.

TheBizzle commented 2 years ago

@LaCuneta, what do you think we should do for test coverage around this?

Reproducing the issue is simple in Galapagos:

I wanted to add a language test for this in Tortoise, but I wasn't able to reproduce this bug with language tests. It's not clear to me that language tests can.

I was able to trigger the bug by modifying a procedure in Wolf Sheep Predation.nlogo and then running dock.TestModels. So I'm sure there's some solution where we add a dummy model that just exists to catch these sorts of bugs, but that sounds a bit unsavory to me.

Any insight or suggestions you have on how to automate testing for this would be welcome.

CIVITAS-John commented 2 years ago

Hi Jeremy,

I edited the original issue description. There might be two more words that need to be added.

Best, John Chen

CIVITAS-John commented 2 years ago

Thank you!!

CIVITAS-John commented 2 years ago

Hi, have we also added the two other words here? i.e. tortoise_require and workspace.

Or added relevant tests for them if they are not required?

TheBizzle commented 2 years ago

Turns out that I was wrong about not being able to test for this with language tests. Now that I understand the bug better, I wrote some language tests for it in https://github.com/NetLogo/NetLogo/commit/4ac36d7a4c5c5253e9bc4213a9820c6355d8c620.

I also removed unneeded name-mangling entries in https://github.com/NetLogo/Tortoise/commit/751f8619590156a03c70c1a78a14a775a2daf02d, and added new ones for async, await, and workspace in https://github.com/NetLogo/Tortoise/commit/305310b0aec4ff5f4dec09a109efc2784a3c7506.

There's mention in the original report of tortoise_require needing to be name-mangled, but I don't see how that bug would be possible, so I would need to see some code that demonstrates an error with it, before adding it to the name-mangler.