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

ysh exits after ctx push (&a) { true } #1864

Closed
bar-g opened this issue Mar 15, 2024 · 14 comments
Closed

ysh exits after ctx push (&a) { true } #1864

bar-g opened this issue Mar 15, 2024 · 14 comments

Comments

@bar-g
Copy link
Contributor

bar-g commented Mar 15, 2024

I somehow wrongly used a place and ysh terminated:

ysh ysh-0.21.0$ ctx push (&a) { true }

ysh: _gen/bin/oils_for_unix.mycpp.cc:29646: syntax_asdl::loc_t* location::TokenForExpr(syntax_asdl::expr_t*): Assertion `0' failed.

$
andychu pushed a commit that referenced this issue Mar 15, 2024
@andychu
Copy link
Contributor

andychu commented Mar 15, 2024

Thank you, this is now fixed! We forgot an error location.

$ bin/ysh -c 'ctx push (&a) { true }'
  ctx push (&a) { true }
             ^
[ -c flag ]:1: fatal: Arg 1 should be a Dict, got Place

You can get a new tarball here under cpp-tarball:

http://travis-ci.oilshell.org/github-jobs/6488/

@andychu
Copy link
Contributor

andychu commented Mar 15, 2024

I can see why there is confusiong between mydict and &mydict

I guess this is what ->mydict is meant to solve

ctx push (->mydict) {  # emphasize mutability
  true
}

@andychu
Copy link
Contributor

andychu commented Mar 15, 2024

This also suggests that our flag parser API could be

var spec = {}  # we know it's going to be a Dict, unlike json read

parser (spec) {  # NOT parser (&spec) 
  flag -v --verbose
}

or

var spec = {}
parser (->spec) {  # emphasize mutability
  flag -v --verbose
}

@PossiblyAShrub - there is some possible inconsistency between mutating a dict and rebinding a name spec here

Somehow I find parser (&spec) OK though ? It's just shorter ?


But I can also take the @bar-g viewpoint and say that it is inconsistent :)

I was trying to explain or emphasize mutability vs rebinding names. Kind of a subtle concept. I wish we didn't have that distinction.

@andychu
Copy link
Contributor

andychu commented Mar 15, 2024

I basically wanted to save people from "declaring" spec, but maybe that's a false economy ???

This also has implications for Hay !

@bar-g
Copy link
Contributor Author

bar-g commented Mar 15, 2024

Hm, when compiling the tables in

I my thought was to let ->var always "put the variable into a place" if it isn't a place, yet.

Could that maybe also still and consistently save people from having to "declare" the var?

My idea with that, though, initially was that I just wanted to save people from having to use special "place syntax" where ever it makes sense (and require the -> where it makes sense), and have everything work seamlessly with:

But as you mention it now, another nice aspect of both together could be that the mutating vs. rebinding difference may be practically gone. (People always pass places by default, when passing mutable ->things.)

I think the mutating vs. rebinding difference would then merely remain relevant
for rare special cases where the ->:var syntax is used.

@PossiblyAShrub
Copy link
Collaborator

@PossiblyAShrub - there is some possible inconsistency between mutating a dict and rebinding a name spec here

Somehow I find parser (&spec) OK though ? It's just shorter ?

I basically wanted to save people from "declaring" spec, but maybe that's a false economy ???

I prefer passing a place to parser, not just because it's shorter, but because it provides better encapsulation. I've purposely avoided documenting the internal structure of spec because I don't intend for that to be public right now. By asking the user to pass in an empty dict to parser, we require that they know something about spec.


For example -- and for fun :) -- we could make the spec a value.Block and offload all the argument parsing to python:

# args.ysh

proc flag (short, long) {
  # We don't support `--indent -1` anymore?
  json write --indent 0 ({'type': 'flag', short, long}) | tr --delete '\n'
  echo  # For a trailing newline
}

proc arg (name) {
  json write --indent 0 ({'type': 'arg', name}) | tr --delete '\n'
  echo
}

proc parser (; spec ;; block) {
  call spec->setValue(block)
}

func parseArgs(spec, argv) {
  try {
    eval (spec) | python3 parser.py @argv | json read (&res)
  }
  if (_status !== 0) {
    error "Bad Usage: See $0 --help" (status=2)
  }
  return (res)
}

parser (&spec) {
  flag -v --verbose
  arg file
}

var args = parseArgs(spec, :| --verbose README.md |)
= args  # => (Dict)   {"verbose":true,"file":"README.md"}
# parser.py
import sys
import json
import argparse

nodes = [json.loads(l) for l in sys.stdin.readlines()]

parser = argparse.ArgumentParser()
names = []
for node in nodes:
    if node['type'] == 'flag':
        parser.add_argument(node['short'], node['long'],
                            action="store_true")
        names.append(node['long'].removeprefix('--'))
    elif node['type'] == 'arg':
        parser.add_argument(node['name'])
        names.append(node['name'])

# A bit of a pain to serialize the argparse.Namespace class...
args_namespace = parser.parse_args()
args = {}
for name in names:
    args[name] = getattr(args_namespace, name)
json.dump(args, sys.stdout)

I think that the "pass a place" pattern is a good practice for when you don't care about internal structure but will instead forward the value to another func/proc in the API. (This is a one way implication, there are other reasons why you'd want to pass by place. eg. json read)


Why shouldn't ctx take a place? In the case of ctx, the caller should be very interested in the internal structure of the context dictionary. It is a case where the caller needs to decide what the context dictionary should look like. Passing a place moves that responsibility to the callee.

@bar-g
Copy link
Contributor Author

bar-g commented Mar 15, 2024

Why shouldn't ctx take a place? [...] a case where the caller needs to decide what the context dictionary should look like

Hm, now I'm confused. That sounds as if it is not possible to create a place that points to an existing var, and as if &var would always destoy or rebinding the existing var?

If that's the case, couldn't &var point to var if existing? Alternatively, let only the ->var syntax work like that (preserving vars)?

Idea again is to using places in much more situations (everything with ->) to get rid of a pile of edge cases, problems, and inconsistencies. (And also, as you say, better encapsulated while hopefully still allowing the caller to decide about the type of the var the place points to, i.e. if it actually has been declared beforehand and already exists).

@andychu
Copy link
Contributor

andychu commented Mar 15, 2024

Hm very interesting ... I think there are two sides to this, technical and philosophical

Why shouldn't ctx take a place? In the case of ctx, the caller should be very interested in the internal structure of the context dictionary. It is a case where the caller needs to decide what the context dictionary should look like. Passing a place moves that responsibility to the callee.

  • Philosophically, I would call this something like an opaque pointer in C.

    • What is FILE* in C? Actually you can't really know, it's not specified. All you know is that you get it back from open(), and you can pass it to read() and write().
  • However, technically {} can express almost anything. I think you do the python3 parser.py example even with a dict??


BUT if that's the case, then this line means almost nothing:

var spec = {}  # does this add anything?   ANYTHING opaque can be a dict.

You can argue it's just "noise".

I think this is a pretty good argument for parser (&spec) . I think we will have to wait for Hay / Test framework to make a final decision. It's possible decision there require global consistency.


I am still a little bugged by &x vs. ->x. However, the rebinding vs. mutation distinction exists in pretty much every language.

Even Elixir has it I think

We didn't invent that problem and -> I think gives people a little more explicit readability if they want

@bar-g
Copy link
Contributor Author

bar-g commented Mar 15, 2024

We didn't invent that problem and -> I think [...]

Ha, yes.

I think, you could be the first with a valuable strict_type_interaction setting to solve it, though.
(Also all its implied/created problems and pitfalls.)

@bar-g
Copy link
Contributor Author

bar-g commented Mar 16, 2024

Hm, I'm still pondering on this:

[Could] &var point to var if [var is already] existing?

Or more precisely, if that would be technically possible for ->var? Allowing:

the ->var syntax [to] work like that (preserving vars)?

And then, when calling myproc (->var), if the variable access/mutate syntax within the proc could accept and work with the place, i.e. same syntax accepting vars as well as vars referenced by a place?


[Precision added:]

Idea again is to using places in much more situations [] (every [proc/func call] with ->) to get rid of a pile of edge cases,
problems, and inconsistencies. (And also, as @PossiblyAShrub said, better encapsulated while hopefully still allowing the caller to decide about the type of the var the place points to, i.e. [let a new place reference it] if [a variable of that name] actually has been declared beforehand and already exists).

@andychu
Copy link
Contributor

andychu commented Mar 16, 2024

The main reason the ctx builtin would change is to satisfy a use case documented here - https://www.oilshell.org/blog/2023/06/ysh-sketches.html

It has to be designed in the context of real code.

@bar-g
Copy link
Contributor Author

bar-g commented Mar 16, 2024

The main reason the ctx builtin would change is to satisfy a use case

It has to be designed in the context of real code.

That's only one of different design "methods".

My gripes are rather principles like

  • simplicity to explain and understand
  • same semantics should use the same syntax
  • introduces a separate variable setting syntax, and only for rarely used cases
  • it's not really "guessable" based on standard ysh variable mutation, and users don't read the manual

@bar-g
Copy link
Contributor Author

bar-g commented Mar 16, 2024

@andychu
Copy link
Contributor

andychu commented Jun 20, 2024

@andychu andychu closed this as completed Jun 20, 2024
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

3 participants