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

[experiment] script state metatable #436

Merged
merged 1 commit into from Sep 21, 2018
Merged

[experiment] script state metatable #436

merged 1 commit into from Sep 21, 2018

Conversation

@pq
Copy link
Collaborator

@pq pq commented Jun 26, 2018

another script environment management experiment.

sets up a metatable for script state allowing scripts to shadow but not clobber state in _G [1]. script state is zeroed on script runs so scripts don't accumulate surprising state.

unlike #426 which introduces a fresh env that scripts are loaded into, this continues to use _G and so the repl "just works".

(tested on grid apps; not midi.)

see also: #258

/cc @catfact @tehn @ngwese


[1] qualification: "shallow" state or something; writing into (pre)existing tables in _G will stick.

@catfact
Copy link
Collaborator

@catfact catfact commented Jun 26, 2018

oh, this is clever. by changing the _G metatable you basically freeze the global namespace. i like it. i would even do this right away after declaring all the globals that we actually want / need (in startup.lua or whatever)

as you say, it doesn't prevent a script from smashing contents of existing global tables. maybe its sufficient to cover that by convention.

@pq
Copy link
Collaborator Author

@pq pq commented Jun 26, 2018

as you say, it doesn't prevent a script from smashing contents of existing global tables. maybe its sufficient to cover that by convention.

i'm wondering that too. also curious how this gotcha would come up and impact folks in practice. i haven't written enough lua myself to know but my gut says this wouldn't happen very often when writing scripts for making music but dunno!

@tehn
Copy link
Member

@tehn tehn commented Jun 26, 2018

re: trampling on globals and conventions.

are we still possibly interested in renaming all of the c functions to include a preceding underscore?

@catfact
Copy link
Collaborator

@catfact catfact commented Jun 26, 2018

i think its fine. this covers the two practical problems: scripts accidentally clobbering global names used by the norns system, and stumbling over global state left over from another script.

@tehn
Copy link
Member

@tehn tehn commented Jul 5, 2018

@catfact @pq i'm trying to understand how to best implement this. basically we need something like state_save() at launch right before the script is loaded and then have state_reset() when a new script is loaded.

@pq
Copy link
Collaborator Author

@pq pq commented Jul 6, 2018

@tehn : not sure i'm following. (sorry, late back to the conversation!)

are you thinking about user state that should persist between script runs? if so, what is that state? (if not, sorry if i'm muddying the waters!)

@tehn
Copy link
Member

@tehn tehn commented Jul 6, 2018

@pq apologies perhaps i'm likewise totally confused.

my assumption of the goal:

  1. script-instantiated globals should not persist between script loads, so that scripts don't contaminate other scripts
  2. we want to be able to restore the startup state of globals in case someone overwrites metro or whatever

so mainly i have no idea what the code in this does :)

what i'm looking for is some sort of drop-in replacement for my lvm_reset work-- instead of resetting the LVM i'd just like to restore the original state of globals

@pq
Copy link
Collaborator Author

@pq pq commented Jul 6, 2018

so mainly i have no idea what the code in this does :)

achievement unlocked! 😄

this little patch should ensure script-written globals only stick until the next script is run which is meant to handle cases 1 and 2. in your example, metro would be clobbered in the script but not the next one. we could go further and forbid that from happening too if we wanted to police the meta-table.

as for how well this would stand-in for the lvm-reset idea, we should enumerate the issues and do some experimenting. (down the road, this might be a good seed for some docs -- see e.g., #437.)

@tehn
Copy link
Member

@tehn tehn commented Jul 6, 2018

well then, this seems perfect then?

i'm curious how we'd police the meta-table?

@pq
Copy link
Collaborator Author

@pq pq commented Jul 6, 2018

i'm curious how we'd police the meta-table?

briefly discussed in #426 but the basic idea is to be opinionated in the __newindex function and potentially dissallow certain index writes. i haven't tested this but on the surface it should work to do something like:

 __newindex = function(t,k,v)
      if (isDisallowed(k)) then
        warn('writing to '..k..' is a bad idea!')
      else
       state[k] = v
    end,
@tehn
Copy link
Member

@tehn tehn commented Sep 19, 2018

just tested this.

  • works: globals created by the script are nil after running another script.
  • does not: plowed globals (i tried with metro) are still plowed on running another script.
  • desired but confusing: changing values such as the master volume level (a parameter) seems like it should get reset by the logic, but it in fact does not. this approach is tricky because there are some globals we want to be preserved (not reset) on script load. the audio parameters are one of these (though we can have a secondary mechanism to reload these easily from the preferences file).

in general, this is likely a huge improvement as-is, particularly during development when globals can make a huge mess.

@pq
Copy link
Collaborator Author

@pq pq commented Sep 19, 2018

thanks for nudging this. i've been meaning for like forever to cycle back to it.

does not: plowed globals (i tried with metro) are still plowed on running another script.

ok, yeah. that's because those writes are to a table -- there's a kind of veiled caveat above about how this works "shallowly".

could you give me an example of what you're doing? are you using metro.alloc or writing to metro[0..n] directly?

changing values such as the master volume level (a parameter) seems like it should get reset by the logic, but it in fact does not.

same thing. we'll need to go another level deeper for those table entries.

to help me get my head back into this, could you enumerate some of the globals we want to have stick and those we don't? (aside: this would be good fodder for #437.)

i have a few ideas and concrete examples will help me test 'em out. 👍

@tehn
Copy link
Member

@tehn tehn commented Sep 19, 2018

i think there are two issues at hand:

  1. littering the environment with globals created by a script or in the REPL, which make debugging difficult during development. this fix works for this!
  2. protecting system globals we don't want wiped out. this doesn't protect. but this is a secondary issue IMO.

for example, in the REPL:

metro = nil

and then reset a script, and it's still nil. but again, this isn't the most pressing issue.

also suggested above is renaming a whole ton of the c function calls with a preceding underscore to clear up usable namespace with a naming rule (ie, don't use leading underscores)

@tehn
Copy link
Member

@tehn tehn commented Sep 20, 2018

further testing shows this as a good solution to the first problem.

perhaps we merge/close and create a secondary issue for the other parts? or makes some checkboxes for #425 ?

@pq
Copy link
Collaborator Author

@pq pq commented Sep 20, 2018

@tehn 👍 -- i have a few ideas for the other bits but they just build on this basic idea.

@tehn tehn merged commit 6e1bd0c into master Sep 21, 2018
@tehn tehn deleted the script_state branch Sep 21, 2018
This was referenced Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants