[builtin/declare] Support -p #671

akinomyoga · 2020-03-21T06:07:28Z

Support declare and declare -p (cf #647, but declare -f, trap, trap -p are not yet supported)

Note: Bash quotes the values with double quotations, but this patch uses single quotations provided by string_opt.ShellQuote.
Note: Bash ignores the flag -g when -p flag is specified, but this patch supports it.
Note: Bash ignores the flags -nrxaA when -p flag and variable names are specified, but this patch supports them.
Please see the changes in spec/assign-extended.test.sh for the detailed behavior

akinomyoga · 2020-03-21T06:15:29Z

Also, there is a problem in implementing declare -p for an array that have unset elements. In Bash, arrays have a literal of the form ([index]=value), so unset elements can be skipped in the literals. However, Oil does not support the form ([index]=value) for indexed arrays, so I don't know how to represent the array with unset elements. Current implementation outputs an empty string '' for a value None as follows:

$ bash -c 'arr=(); arr[3]=foo; declare -p arr'
declare -a arr=([3]="foo")
$ osh -c 'arr=(); arr[3]=foo; declare -p arr'
declare -a arr=('' '' '' 'foo')

So the question is:

Q. Is there any literal to represent an array with unset elements in Oil?

If Oil does not support such a literal, maybe the output of declare could be changed to the following form. Do you have any other ideas?

declare -a arr; arr[3]='foo'

andychu

Thanks for doing this!

Yes the solution you describe seems good, if the intention is to have declare -p output understandable by eval. I guess it can switch to that form only if None is in the array?

I mentioned a few places where we will likely have to change things to translate to C++... so the important part is the tests. (there are some threads on Zulip about how I translate to C++)

They look good pretty exhausitive to me, but I didn't look at every line. As mentioned, I think at least one test should do eval.

andychu · 2020-03-21T08:03:18Z

spec/assign-extended.test.sh

+[local]
+test_var5='555'
+## END
+## OK bash STDOUT:


Maybe instead of listing exact output of OSH and bash, you can eval the output of declare -p?

I think that's the property you want, right?

It makes the test a little more meaningful if both bash and Oil pass the exact same way.

Not all tests have to be like that , but I think at least one should no?

Maybe instead of listing exact output of OSH and bash, you can eval the output of declare -p?

Thank you for the suggestion! That's a good idea! I added such a test d1c85b4, but it is still a very simple test.

Actually, the problem is "how to check the results" of eval & declare: The existence, the flags, and the values of each variable should be checked (i.e., printed for the test). In the case of indexed/associative array variables, the existence and the value of each element, and non-existence of redundant elements should also be checked (i.e., printed). If one wants to test everything in the exact same way in Bash and Oil, it seems that one needs to implement another declare -p in pure shell scripts (which may involve other bugs/divergences on the detailed behavior of each shell), so I gave up.

I think that's the property you want, right?

Yes! Thank you.

Yes that's true, it's tricky. One possibility is to sed to different variable names, and then check for equality. Something like:

eval -- $(declare -p | grep ^myarray | sed s/^myarray=/restored=/) # now check myarray == restored arrays_equal ... # I wrote a function that takes two lengths and then @A and @B concatenated

But yes this is a lot of trouble ...

Actually in Oil we may be able to help with this. We simply have

if (A == B) { # are string, arrays, associative arrays equal? }

but that's in the future

andychu · 2020-03-21T08:05:20Z

osh/builtin_assign.py

@@ -25,10 +26,108 @@
  from frontend import arg_def


+def _PrintVariables(mem, cmd_val, arg, print_flags, readonly = False, exported = False):
+  flag_g = getattr(arg, 'g', None)


These can all just be arg.g, arg.n, etc. right? And then you don't really need the temporary

This function _PrintVariables is shared by class Export, Readonly, and NewVar. Each has its own definition for arg. Export, Readonly, and NewVar supports flags -n, -aA, and -gnrxaA, respectively. For example, some have flag g and the others don't have the flag g. So, one first needs to check whether each attribute is defined in arg or not, or otherwise, I get AttributeError.

I thought about one possibility to create a dictionary at the caller side something like _PrintVariables(..., arg={'g': None, 'n': None, 'r': None, 'x': None, 'a': arg.a, 'A': arg.A}, ...). But it looks like redundant codes. Also, we need to keep consistency between this function call and the definitions of arg of each classes; every time one updates the set of flags in arg, one needs to update this function call accordingly, which is not so maintainable.

Another possibility may be to support the same set of flags for export and readonly as declare.

Do you have an idea of a more clever way to implement this?

Ah OK I see. This is OK for now but could use a comment saying that.

(I think I have an idea of how it will translate to C++ ... the dynamic types of arg.X are an issue I've been thinking about.)

OK!

This is OK for now but could use a comment saying that.

It has been already merged, so could you write a comment for this? Thank you!

andychu · 2020-03-21T08:06:38Z

core/state.py

+        result[name] = cell
+    return result
+
+  def IsGlobalScope(self):


this should have # type: () -> bool

TODO: I think we can set up the continuous build to alert of that, since type checking is running there now

Thank you! Updated 9e94f90.

andychu · 2020-03-21T08:09:38Z

osh/builtin_assign.py

+    cells = {}
+    for pair in cmd_val.pairs:
+      name = pair.lval.name
+      if pair.rval and pair.rval.tag == value_e.Str:


It's better to use tag_() though it's unfortunately uglier. That's because it translates to C++ better -- C++ doesn't have "virtual" fields like Python, only methods. I plan to get rid of .tag and make it .tag(), but for now it's .tag_()

OK! Updated 0bd3c4a.

andychu · 2020-03-21T08:14:37Z

osh/builtin_assign.py

+        flags += 'A'
+      if flags == '-': flags += '-'
+
+      decl = 'declare ' + flags + ' ' + name


generally speaking I use the style of creating a parts = [] array and appending to it, then print(''.join(parts)).

But that is not super important now, if it passes the tests it seems fine

Thank you! I changed to use that strategy in d3dd9c9.

andychu · 2020-03-21T08:22:25Z

core/state.py

@@ -1497,6 +1497,31 @@ def GetAllVars(self):
          result[name] = str_val.s
    return result

+  def GetAllCells(self, lookup_mode = scope_e.Dynamic):


Also I use the style of foo='default' (no space).

I suppose at some point we should do something about issue #1 (autoformatting for Python). If enough contributors want it, I will be motivated to :)

Oh, thank you! Hm, PEP 8 (mentioned in #1) says that spaces are not recommended for keyword arguments and default arguments.

PEP-8 - Other Recommendations

Don't use spaces around the = sign when used to indicate a keyword argument, or when used to indicate a default value for an unannotated function parameter.

Updated e2c89a0.

akinomyoga · 2020-03-21T12:44:54Z

Thank you for your fast review and useful comments!

I guess it can switch to that form only if None is in the array?

I implemented 3a5eb65.

andychu · 2020-03-21T17:06:25Z

osh/builtin_assign.py

@@ -25,10 +26,108 @@
  from frontend import arg_def


+def _PrintVariables(mem, cmd_val, arg, print_flags, readonly = False, exported = False):
+  flag_g = getattr(arg, 'g', None)


Ah OK I see. This is OK for now but could use a comment saying that.

(I think I have an idea of how it will translate to C++ ... the dynamic types of arg.X are an issue I've been thinking about.)

andychu · 2020-03-21T17:11:37Z

spec/assign-extended.test.sh

+[local]
+test_var5='555'
+## END
+## OK bash STDOUT:


Yes that's true, it's tricky. One possibility is to sed to different variable names, and then check for equality. Something like:

eval -- $(declare -p | grep ^myarray | sed s/^myarray=/restored=/) # now check myarray == restored arrays_equal ... # I wrote a function that takes two lengths and then @A and @B concatenated

But yes this is a lot of trouble ...

Actually in Oil we may be able to help with this. We simply have

if (A == B) { # are string, arrays, associative arrays equal? }

but that's in the future

andychu · 2020-03-21T17:15:50Z

osh/builtin_assign.py

+        flags.append('A')
+      if len(flags) == 0: flags.append('-')
+
+      decl.extend(["declare -", ''.join(flags), " ", name])


It looks like you don't need ''.join(flags) or ''.join(body) below, because we join it at the end anyway. But this is a minor detail

Just for curiosity, how can I write in this case? The reason why I maintained other lists flags and body is because I wanted to use len(flags) and len(body). Ah, OK. Maybe I could write something like the following, but I just didn't want to split a simple line into many lines.

decl.append("declare -") decl.extend(flags) decl.extend([" ", name])

Or this? But the following code seems even less efficient because it induces addtional list generations.

decl.extend(["declare -"] + flags + [" ", name])

I usually check guard space separation in a loop with i != 0 , and then you don't need to check len(body). And I think you can check len(assoc_val.d) for the last check.

But those are minor details and I think it's fine. There is one more string allocation for ''.join(flags) and one more for ''.join(body), but like you say it's easy to introduce other allocations elsewhere. After translating to C++ the previous style of += would introduce a lot more allocations.

I actually did go back and optimize allocations in C++ generated from Python for the parser! e.g. the last step of http://www.oilshell.org/blog/2020/01/parser-benchmarks.html

But I think this is pretty close to optimal, and it's not a hot path anyway, i.e. declare -p is not that common.

OK! I see, it is aimed to be translated to C++. Thank you!

andychu · 2020-03-21T18:15:57Z

BTW the place where I expect it to matter was osh/word_eval.py. It initally built up a lot of intermediate lists and used + to concatenate them, but now it uses the .append() or .write() style. There are a lot of tiny objects in this algorithm, as opposed to the escape codes in strings that other shells use:

https://github.com/oilshell/oil/wiki/OSH-Word-Evaluation-Algorithm

But I expect we can make it pretty fast ...

andychu reviewed Mar 21, 2020

View reviewed changes

akinomyoga added 7 commits March 21, 2020 19:27

[builtin/declare] Support -p

8caf8eb

[test/spec] Add tests for 'eval & declare'

d1c85b4

[builtin/declare] Support arrays with unset elements for 'declare -p'

3a5eb65

[builtin/declare] Update type annotations of functions

9e94f90

[builtin/declare] Remove spaces in default/keyword arguments

e2c89a0

[builtin/declare] Use .tag_() instead of .tag

0bd3c4a

[builtin/declare] Use ''.join(buff) to build strings

d3dd9c9

akinomyoga force-pushed the declare-print-definitions branch from ef68ded to d3dd9c9 Compare March 21, 2020 12:44

akinomyoga mentioned this pull request Mar 21, 2020

[core/process] Fix/Implement redirections <>, 5>&-, 6>&5-, {fd}>file, etc. #672

Merged

andychu merged commit 091db69 into oils-for-unix:master Mar 21, 2020

andychu reviewed Mar 21, 2020

View reviewed changes

akinomyoga deleted the declare-print-definitions branch March 21, 2020 18:03

akinomyoga mentioned this pull request Mar 22, 2020

Try to parse and run ble.sh #653

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[builtin/declare] Support -p #671

[builtin/declare] Support -p #671

akinomyoga commented Mar 21, 2020

akinomyoga commented Mar 21, 2020

andychu left a comment

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

akinomyoga commented Mar 21, 2020

andychu Mar 21, 2020

andychu Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 21, 2020

andychu Mar 21, 2020

akinomyoga Mar 22, 2020

andychu commented Mar 21, 2020

[builtin/declare] Support -p #671

[builtin/declare] Support -p #671

Conversation

akinomyoga commented Mar 21, 2020

akinomyoga commented Mar 21, 2020

Q. Is there any literal to represent an array with unset elements in Oil?

andychu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akinomyoga commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andychu commented Mar 21, 2020