Shasta extension for libbash #5

BolunThompson · 2024-12-05T00:34:13Z

Integrates @sethsabar’s shasta for libbash work into the mainline. The tests pass (besides one bash expected failure).

angelhof

This looks good! Two big questions:

Is this backwards compatible?
Can we add some text in README or another document that really describes the interplay between dash and abash shasta? And how should one navigate this repo if they need to use one of the two only?

shasta/subst.py

shasta/ast_node.py

shasta/bash_to_shasta_ast.py

BolunThompson · 2024-12-05T23:22:23Z

The changes are mostly backwards compatible, but to accommodate bash some of the existing AstNodes are modified. Namely, the type of fds (and the arg field in DupRedirectNode) in the AST have been changed to the tuple (str, [list[ArgChar], int]) from int, since in bash commands can redirect to variables that are then assigned an fd. Besides that, it’s just adding new optional fields. Are there projects using shasta that would break on these changes? (besides PaSh).

The tests for dash still pass, both in shasta and my branch of PaSh with the bash changes merged in, which is a good sign.

I’ll add some documentation on that. Repo file layout:

ast_node.py: AST nodes shared between dash and bash (by design). In principle, if you want to use libdash you can just ignore the bash fields.
json_to_ast.py: Libdash to shasta
bash_to_shasta_ast.py: Libbash to shasta
print_lib.py: Pretty prints the resulting AST in the same way for both bash and dash.
subst.py: Bash only helper functions.

If you’re using the library, the only difference is whether you import from json_to_ast (for dash) or bash_to_shasta_ast (for bash).

angelhof · 2024-12-06T02:15:12Z

I now saw your overall comment, that is great, thanks a lot! The documentation plan sounds perfect!

Regarding the changes to AST nodes, could we have a dash-bash-diffs.md document that enumerates them? Are they only the ones you mentioned here? If so, things should be fine more or less, I don't suspect anything will break due to them (and if something does break we should be able to fix it pretty easily).

Once we get these merged we should also make a new major release of shasta so that downstream projects don't really break.

Thank you so much for the very diligent work here @BolunThompson !!

Somewhat of a bad practice but, otherwise, every class that calls string_of_arg would have to have a bash_mode field to change behavior for it

Currently, bash mode interprets all characters as CArgChar, relying on the user to manually expand as necessary. This breaks string_of_arg, since it escapes all '$' CArgChars, assuming that it shouldn't be interprted as variable interpolation when it's pretty printed. This disables escaping in bash mode.

Only requires adding a few "from __future__ import annotations" to allow type hints like list[int] to be used instead of List[int]

Type hinting is only being used for documentation at this time, but it's good to use specific and syntactically correct type hints. However, to use the type checker in a useful way, you'd have to use isinstance instead of string tags for the AstNodes (or an equivalent "is_type(*AstNode)" function).

angelhof

I think this looks great, what are peoples' thoughts on the libbash dependency?

dash-bash-diffs.md

angelhof · 2024-12-08T22:33:51Z

pyproject.toml

@@ -14,6 +14,7 @@ classifiers = [
    "License :: OSI Approved :: MIT License",
    "Operating System :: POSIX",
 ]
+dependencies = ["libbash"]


I am scared of this dependency because I am afraid of dependency hell (either due to circular dependencies or because a downstream program (like PaSh) might want a different version of one and the other).

I guess we don't have this issue with libdash because we use a JSON object? Could we do sth similar here (maybe in a different PR?)? Alternatively, is my fear irrational and we just have to bite the bullet and do it? I am wondering whether someone could use shasta without needing to import libbash and whatever that includes.

I agree with your intuition — although I can’t think of an obvious scenario that would cause a problem. The code right now doesn’t actually import anything from libbash (in a runtime capacity) except a handful of enums, so I can easily remove the dependency after copying those over (± some minor changes).

See binpash#5 (comment) For tests, libbash (and now libdash) are dev dependencies.

BolunThompson · 2024-12-11T19:58:13Z

That’s weird — the tests sometimes pass on my machine, and I’ve somehow messed up the state of my local git repo, so I‘ll fix this in a day or two.

angelhof · 2024-12-11T20:11:28Z

No rush!

See binpash#5 (comment) For tests, libbash (and now libdash) are dev dependencies.

Caused very annoying bug in that bash_node was being set in the bash parsing code, but then dash parsing code is called internally, then bash parsed objects print out in dash mode. Side note: this is why they teach that globals are bad in intro to CS!

Signed-off-by: Bolun Thompson <[email protected]>

BolunThompson · 2024-12-15T22:29:11Z

Should be ready to review now!

I suspect shasta converts libbash into shasta incorrectly for a few edge cases because they compare roundtrip 1 and 2 (ex: there was a bug where shasta ignored whether a case node was supposed to fall through or not). Comparing the outputs of the original script and the roundtrips on valid bash tests could catch these (another issue?).

Signed-off-by: Bolun Thompson <[email protected]>

angelhof · 2024-12-17T00:08:06Z

I suspect shasta converts libbash into shasta incorrectly for a few edge cases because they compare roundtrip 1 and 2 (ex: there was a bug where shasta ignored whether a case node was supposed to fall through or not). Comparing the outputs of the original script and the roundtrips on valid bash tests could catch these (another issue?).

Do you mean that to catch these bugs we would need to compare the original script and the output after the roundtrip? instead of roundtrip 1 vs roundtrip 2? I think this would catch some bugs but would also make things a bit flaky because our pretty printing would affect tests (even though semantically equivalent). In any case, we should look at this in a different PR :)

BolunThompson · 2024-12-17T02:41:58Z

That’d work, but as you said it’d be finicky. #6 is what I meant to say.

sethsabar added 10 commits December 5, 2024 00:11

Updates to shasta for bash

46b8ebf

Implemented all types of bash redirects

1037f11

update redirection structure

e9bc760

fix time node implementation

73dadf9

all bash tests pass shasta

8355528

Shasta passing all bash scripts including those with SOH

3617f0b

bug fixes to make sure we're still compatible with dash

55446d4

minor bug fix

bd2690d

bug fix for arith nodes

c48c376

bug fix eof edge case

59303c2

angelhof reviewed Dec 5, 2024

View reviewed changes

shasta/subst.py Outdated Show resolved Hide resolved

shasta/ast_node.py Show resolved Hide resolved

shasta/ast_node.py Outdated Show resolved Hide resolved

shasta/bash_to_shasta_ast.py Outdated Show resolved Hide resolved

BolunThompson force-pushed the bash branch 2 times, most recently from f43b1cc to cd36e5b Compare December 8, 2024 17:48

BolunThompson requested a review from angelhof December 8, 2024 17:54

BolunThompson force-pushed the bash branch from cd36e5b to 390a1ca Compare December 9, 2024 01:25

BolunThompson added 9 commits December 11, 2024 05:00

Integrate bash and dash tests

95ea75c

Add global BASH_MODE

6167577

Somewhat of a bad practice but, otherwise, every class that calls string_of_arg would have to have a bash_mode field to change behavior for it

Backport bash changes to python 3.8

bc3d55b

Only requires adding a few "from __future__ import annotations" to allow type hints like list[int] to be used instead of List[int]

Add BashNode superclass marker

93660aa

Remove unused bash code

d64fde1

Add bash documentation

cd751b2

Add libbash as dependency

070e805

BolunThompson force-pushed the bash branch from 390a1ca to 070e805 Compare December 11, 2024 05:19

angelhof reviewed Dec 11, 2024

View reviewed changes

BolunThompson added a commit to BolunThompson/shasta that referenced this pull request Dec 11, 2024

Remove libbash as dependency

9bd5e31

See binpash#5 (comment) For tests, libbash (and now libdash) are dev dependencies.

BolunThompson added 5 commits December 12, 2024 05:02

Remove libbash as dependency

a8c3441

See binpash#5 (comment) For tests, libbash (and now libdash) are dev dependencies.

Fix typo

1afbeb8

Format new code

fbf8d51

Fix linting errors

dcf1e32

Refactor utf-8 check

ea94fec

BolunThompson force-pushed the bash branch from 1afc3a0 to ea94fec Compare December 12, 2024 05:28

BolunThompson added 2 commits December 12, 2024 19:23

Fix code typo

01db807

BolunThompson mentioned this pull request Dec 15, 2024

Merge changes in to support parsing bash scripts binpash/pash#737

Open

Update shasta version

1a1d50c

Signed-off-by: Bolun Thompson <[email protected]>

BolunThompson added 2 commits December 15, 2024 22:54

Add case fall through support

7beed18

Signed-off-by: Bolun Thompson <[email protected]>

Fix case bug

650d818

Signed-off-by: Bolun Thompson <[email protected]>

angelhof merged commit be560cf into binpash:main Dec 17, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shasta extension for libbash #5

Shasta extension for libbash #5

BolunThompson commented Dec 5, 2024

angelhof left a comment

BolunThompson commented Dec 5, 2024 •

edited

Loading

angelhof commented Dec 6, 2024

angelhof left a comment

angelhof Dec 8, 2024

BolunThompson Dec 11, 2024 •

edited

Loading

BolunThompson commented Dec 11, 2024 •

edited

Loading

angelhof commented Dec 11, 2024

BolunThompson commented Dec 15, 2024 •

edited

Loading

angelhof commented Dec 17, 2024

BolunThompson commented Dec 17, 2024 •

edited

Loading

Shasta extension for libbash #5

Shasta extension for libbash #5

Conversation

BolunThompson commented Dec 5, 2024

angelhof left a comment

Choose a reason for hiding this comment

BolunThompson commented Dec 5, 2024 • edited Loading

angelhof commented Dec 6, 2024

angelhof left a comment

Choose a reason for hiding this comment

angelhof Dec 8, 2024

Choose a reason for hiding this comment

BolunThompson Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

BolunThompson commented Dec 11, 2024 • edited Loading

angelhof commented Dec 11, 2024

BolunThompson commented Dec 15, 2024 • edited Loading

angelhof commented Dec 17, 2024

BolunThompson commented Dec 17, 2024 • edited Loading

BolunThompson commented Dec 5, 2024 •

edited

Loading

BolunThompson Dec 11, 2024 •

edited

Loading

BolunThompson commented Dec 11, 2024 •

edited

Loading

BolunThompson commented Dec 15, 2024 •

edited

Loading

BolunThompson commented Dec 17, 2024 •

edited

Loading