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

expressions should be tagged oil:all #1148

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

expressions should be tagged oil:all #1148

andychu opened this issue May 26, 2022 · 6 comments

Comments

@andychu
Copy link
Contributor

andychu commented May 26, 2022

Analogous to #1147

I fixed this case in the last release:

var x = $(false)  # now command_sub_errexit is on

But there is still stuff like this:

$ bin/osh -c 'var x = "foo:bar"; IFS=:; var y = $(write -- $x); echo $y';
foo
bar

That is bad. So I think it must be oil:all, not just command_sub_errexit

The issue is that var y = $(write -- $x) should behave the same way regardless of context. Following the local reasoning principle

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Maybe this is not that bad.

I think the bottom line is that

  • Oil code should go in .oil files running bin/oil
  • OSH code should go in .osh files , and have shopt --set oil:upgrade and possibly strict:all
  • If you want them to interoperate, it should be done with processes !! NOT mixing different options in the same interpreter.
    • when you start a new process, the options at the top of the file work.

You should not source together files with different options.

The sticking point is

Although I guess you CAN run completion scripts in a different process! You just need a little protocol

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

So I think the bug here is that you started using:

var y = $(write -- $x)

WITHOUT adding shopt --set oil:upgrade. That includes simple_word_eval.

You should do these AT THE SAME TIME:

  • Start using var and proc
  • Set shopt --set oil:upgrade , for more error handling and so forth

It can't be that seamless and "local" by nature


So then does it make sense that command_sub_errexit is special cased? Hm

@bar-g
Copy link
Contributor

bar-g commented May 26, 2022

What's the reason some basic oil syntax is enabled in osh by default?

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Yup you are right! I just came to the same conclusion.

We have to disable var / proc in bin/osh to obey the local reasoning principle!

I was thinking about this a lot for the last hour :)

I will file a bug

@andychu
Copy link
Contributor Author

andychu commented May 26, 2022

Closing in favor of #1149

Thanks for the feedback -- this came out of thinking about the table and the upgrade path.

Because the table is confusing if you can use var and const without setting any options! Then it feels like there are two different ways to upgrade. There should only be one.

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

bar-g commented May 26, 2022

Yeah, great! Looks like a nice clean up.

Wondering if your enable-oil-within proc idea could also turn into a nice upgrade/addition improvent.

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