binpash / pash

PaSh: Light-touch Data-Parallel Shell Processing
MIT License
552 stars 39 forks source link

Refactor AST #671

Closed mgree closed 1 year ago

mgree commented 1 year ago

Pash currently uses ASTs in three forms:

We should have just one, well structured AST, from libdash on down.

class Command(ABC):
  ...

class SimpleCommand(Command):
  linno: int
  assigns: ...
  args: ...
  redirs: ...

class If(Command):
  cond: Command
  then_b: Command
  else_b: Command

...

class ArgChar(ABC):
  ...

Word = [ArgChar]

class C(ArgChar):
  c: str

class E(ArgChar):
  c: str

class V(ArgChar):
  fmt: ParamterFormat
  null: bool
  var: str
  arg: Optional[Word]

We should use this representation uniformly. We propose the following restructuring:

angelhof commented 1 year ago

PR #673 completes a part of the above requirements, defining classes for all command_nodes (but not for arguments, redirections, etc). It also cleans up many places where there was mixed use of typed and untyped AST objects, only using the typed ones now.

It should be straightforward to extend this to its own separate shasta library, while also defining the rest of the classes now that the initial structure is there. The two modules that will need straightforward modification are:

  1. untyped_to_ast.py
  2. ast_node.py

Then we would also need to make a few straightforward modifications to ast_to_ast.py and ast_to_ir.py so that they access fields and not indices of the unstructured objects.

angelhof commented 1 year ago

PR #675 extends PaSh to use the new ASTs throughout.

angelhof commented 1 year ago

@mgree I have two questions:

  1. When you say a pretty() function, do you mean that this (https://github.com/mgree/libdash/blob/master/libdash/printer.py) should be moved out of libdash and into shasta?
  2. Should the module that turns a json ast to an AstNode object (https://github.com/binpash/pash/blob/rest-ast-nodes/compiler/shell_ast/untyped_to_ast.py) be in shasta or somewhere else? It feels to me that it should be in shasta, but I am not sure. Or are you envisioning a shasta that only contains the ast definition?
mgree commented 1 year ago

I'd say in shasta, on both counts!

angelhof commented 1 year ago

Closing this! Will make an issue in libdash to be updated to import shasta, return proper ASTs, and remove the printer.