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

procs could be tagged oil:all ? #1147

Closed
andychu opened this issue May 26, 2022 · 20 comments
Closed

procs could be tagged oil:all ? #1147

andychu opened this issue May 26, 2022 · 20 comments

Comments

@andychu
Copy link
Contributor

andychu commented May 26, 2022

This means they implicitly have a shopt --set oil:all { ... } around them

This eliminates the problem where the semantics of the function depend on the option state of the caller!

This could be a huge difference between shell functions and Oil procs.

Also it could eliminate the need to think about any options? You just start writing proc and it's a new language ???

  • Then we need an invariant -- procs can't call shell functions? That might be OK? You have to do everything from the bottom up ...
    • Otherwise shell functions would run with oil:all ? Maybe that's OK?

Similarly, haynode blocks run with oil:all

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Really we need to try porting our own shell scripts with this mechanism ...

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

I think this is a pretty good idea. It will allow opting into Oil without even thinking about options -- just write new proc, like you do var.

It also has the property of LOCAL REASONING

You can tell how your function behaves without looking at the top of the file!

  • TODO: are there any efficiency concerns? You will constantly be pushing and popping on self.opt_stacks. Before every function call.
    • maybe you can set a flag to avoid that

Recommended file extensions

  • .oil if running with bin/oil
  • .osh if running with bin/osh, but everything in proc is Oil.
  • .sh if running under multiple shells

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

@bar-g might want to give feedback because it affects the usage table

Idea

#!/usr/bin/env osh

ls $mydir /tmp    # this is run with no options

proc myproc {    # everything in here is run with `oil:all`
  ls $mydir /tmp
}

myproc  # oil:all is pushed on the stack

ls $mydir /tmp   # back to OSH options


This came up because of

Configuration blocks (hay blocks) are run with Oil.

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

Really a pretty nice idea to try out (maybe option oil_in_proc?) it could only enable oil:upgrade, to break as few things as possible.

I think it would be important to be able to use shell functions (as defined elswhere or in a lib), it seems pretty moot otherwise, wouldn't it?

Maybe a file extension .oil.upgrade, oil.sh or .sh.oil? (kinda like .tar.gz) But why would one be necessary, wouldn't only, and always, changing the shebang be necessary?

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

are there any efficiency concerns? You will constantly be pushing and popping on self.opt_stacks

Would this be avoidable by explicitly setting oil:update or oil:all at the top-level?

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

So the idea behind disallowing shell functions is forcing syntax to match semantics and local reasoning according to the Language Design Principles

If you run into that error

  myfunc
  ^ error: called function from proc; you should change it to proc

Then you're supposed to change the code, and it will be more readable. Right now you have two questions:

  • when I look at f() { ... }, what options does it run with?
  • when I look at proc p { ... }, what options does it run with?

You cannot answer this question without looking at the top of the file or tracing the program.


This bug proposes:

  • when I look at proc p { ... }, what options does it run with? ALWAYS oil:all

Then you have the remaining question:

  • when I look at f() { ... }, what options does it run with?

I don't want the answer to be:

  • if called from a proc, oil:all, because the proc set it
  • otherwise, whatever was in the global context

It's better if when you call top-level-proc, you are ensured that

  • everything BELOW that will be run with oil options.
  • AND you can see that in the source code. Everything with oil options is labeled proc.

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Would this be avoidable by explicitly setting oil:update or oil:all at the top-level?

It's will be slower to set options on every proc call than to set it once at the top level. But it might not be that slow, and if it is, we can find a way to optimize it.

We should get the language behavior right first :)

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

Looks like if it works as pictured, it could really be a candidate to be enabled by default.

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Yeah I think this can work and will be good.

Only issue is that you will still need oil:upgrade in order to PARSE { } and ( ) with their new meanings. Once you have started RUNNING a proc, it's too late to change how it's parsed.

I think that is OK.


This came up through "hay" config files #951, but your work on the table made me think of it.

Because the table is a little complex. It's totally accurate, but still a little to hard to explain to users.

So to simplify the upgrading we just say "use proc". That is even simpler.

Once you have used proc -- then it will be EASIER to run with bin/oil or oil:all. The porting will naturally happen gradually as you use proc.


So yes everything in the table will still be accurate -- we will have all the same option groups! And yes strict:all and oil:upgrade still make sense.

It's just that when you start writing proc, you won't have to LATER port your procs to new semantics! That would be bad.

You don't want to port Oil-looking code to a new Oil! It should just be Oil code, full stop!

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

myfunc
^ error: called function from proc; you should change it to proc

This would break the seamless addition of oil features and propably require to change all functions, even in sourced libs. I don't see an advantage over having to set oil:upgrade globally. Rather the contrary, doesn't that error even worsen selective feature addition? For not much of a practical reason?

I see that parsing oil within procs could simplify the selective implentation of new features using oil in existing scripts, and leveraging on the osh-oil compatibility when calling out to legacy shell from there.
The way to upgrade a whole script I think could still be to set oil:upgrade globally.

I'm not sure if it would ever be wanted to disallow non-procs in oil:all, but if yes, than I think that could only belong to oil:all but should not cut into having seamless feature additions and upgrade processes.

So may procs only enable oil:upgrade for its environment (only), if running in osh (without any oil:* option)?
I guess then much could already work as is, otherwise there should be an error message pointing to the thing to migrate.

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

Looks like if it works as pictured, it could really be a candidate to be enabled by default.

This comment from me was not meant as a confirmation to your (edit: two) message before it, messages crossed again. This is ugly.

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

local reasoning according to the Language Design Principles

Possibly apply that only to oil:all but not osh and oil:upgrade? And let automatically enabled oil:upgrade within procs be a nice upgrade feature of osh.

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

The way to upgrade a whole script I think could still be to set oil:upgrade globally.

Yes that's still true under this new proc.

I guess what I'm saying is we could avoid this situation

  1. Start upgrading your shell script with shopt --set oil:upgrade
  2. Start writing procs. However these are still not fully Oil !!! hm example? strict_tilde is not on?
  3. Now turn on oil:all. Now you have to port more?

So the example is:

shopt --set oil:upgrade

proc p {
  echo ~ZZZZZ     # this is still dependent on strict_tilde
}

BUT I think the answer is:

shopt --set oil:upgrade strict:all

proc p {
  echo ~ZZZZZ     # this is still dependent on strict_tilde
}

So on second thought

  1. I don't think there are that many things that are in oil:all but not in oil:upgrade. There won't be that much porting of procs after.
  2. Also most of them are parsing issues. We can't parse procs differently.

So let me think about this more... Not sure yet

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Another issue is that the converse situation also arises:

Situation:

  • You're running bin/oil
  • You do shopt --unset oil:all { source old-completion-scripts.sh }.
  • However when you RUN THE FUNCTIONS in old completion scripts, they get the current options.

So really there is an alternate semantic: functions could be tagged with the options at the time they were defined. OOOF

Yeah there is some risk to this.

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

So then another option is:

tag-functions-with-current-options {
  f() { ... ; }
  g() { ... ; }
}

h()

But you still have the call stack problem. If f calls h, then what options is it run with?

So yeah I think I will put this off for awhile ... It could be too subtle.

But #1148 is a real example of an issue

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

As you know I don't have your overview, but to add my two cents:

I imagine the migration process to to create errors and each one being a significant step to the migration target (and learning the improved idiom). Idealy, one idiom at a time.

  • May there is an advantageously order to add the oil options?
  • Even if not, it could maybe make things easier/better for the user. Maybe oil:upgrade could internally parse the code repeatedly only adding the options one at at a time to let the user fix the same thing at all places, before turning to the next thing.)

So instead of smoothing the upgrade process, I "fear" having oil:all! in procs and errors telling to add the next proc everywhere would break the seamless addition of oil "drops", and "up" the bar to suddenly have to jump to oil:all instead of oil:upgrade, and now also having convert all of the script.

However, if procs where just enabling oil:upgrade I could imagine this to simplify punctual addition of oil and gradual upgrades.

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

(That is, without any "artificial" errors when calling shell functions, only getting errors wherever something needs to be adjusted to run with oil.)

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Closing in favor of #1149, and I also figured out what we need for #499

@andychu andychu closed this as completed May 26, 2022
@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

Ok, could you still comment/sort out these after the rush.

Improved migration experience?:

I imagine the migration process to to create errors and each one being a significant step to the migration target (and learning the improved idiom). Idealy, one idiom at a time.

  • May there [be] an advantageously order to add the oil options [in the oil:upgrade group]?
  • Even if not, it could maybe make things easier/better for the user. Maybe oil:upgrade could internally parse the code repeatedly only adding the options one at at a time to let the user fix the same thing at all places, before turning to the next thing.)

And simplyfied local oil additions and partial "oil:upgrade"s using proc (triggering oil:upgrade in called code)?:

if procs where just enabling oil:upgrade I could [still] imagine this to simplify punctual addition of oil and gradual upgrades.

One problem to solve could be simple word eval, adapting code outside of the proc will need split/glob functions and those may have thus have a reason to stay enabled in osh?

@andychu
Copy link
Contributor Author

andychu commented May 27, 2022

Hm regarding the last comments, I think we need to get the next release of Oil out (could be 0.11.0) and then start trying the upgrade process ...

It's better to change it based on feedback than try to imagine what people will do

I have strict:all in my scripts. But it's probably a good idea to put oil:upgrade in some of them.

I'd be very interested if you have any scripts you can try it with

We can adjust based on feedback; I think we've done enough thinking as it is. The oil:upgrade and parse_proc (#1149 ) improvements are good

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

No branches or pull requests

2 participants