Skip to content

Issue #748: Fix arithmetic for loops in bash mode#760

Closed
bhumikamittal7 wants to merge 6 commits intobinpash:mainfrom
bhumikamittal7:issue-748
Closed

Issue #748: Fix arithmetic for loops in bash mode#760
bhumikamittal7 wants to merge 6 commits intobinpash:mainfrom
bhumikamittal7:issue-748

Conversation

@bhumikamittal7
Copy link

Problem

When running bash scripts with arithmetic for loops through PaSh with --bash flag, the following error occurred:

KeyError: 'Could not find appropriate preprocessor for arithfor'

Root Cause: The AST node type is ArithFor which gets lowercased to arithfor, but the preprocessor function was named preprocess_node_arith_for (with underscore). The dynamic dispatch in preprocess_node() couldn't find a matching handler.

Fix

Renamed preprocess_node_arith_forpreprocess_node_arithfor to match the naming convention.

Testing

Before fix:

./pa.sh --bash script_with_arith_for.sh
# KeyError: 'Could not find appropriate preprocessor for arithfor'

After fix:

$ cat test.sh
for (( x=0; x < 3; ++x )); do
  echo "iteration $x"
done

$ ./pa.sh --bash test.sh
iteration 0
iteration 1
iteration 2

Discovery Results

Ran discovery on all 268 commented-out bash tests:

Category Count
Passed (exact match) 55
Partial (runs, output differs) 163
Failed 50

This fix enables 55 tests to be uncommented immediately and allows 163 more tests to at least compile and run (with minor output differences).


Remark: Script to find passing tests: scripts/discover_passing_bash_tests.sh

@angelhof
Copy link
Member

Thanks for the change, it looks good but I am not sure why the tests are failing. Could you take a look?

@angelhof
Copy link
Member

Actually, let me take a look since I see that CI/CD is somewhat broken in general, I will let you know once I fix it.

@angelhof
Copy link
Member

@bhumikamittal7 I have been refactoring the project a lot but now I have a good sense of what is failing or not with the tests. Could you rebase your commit over main? Also, instead of pushing the script that discovers the new tests that pass, you should instead just push a commit that uncomments all the new tests that pass :) When you do that ping me and I can merge the PR!

@bhumikamittal7 bhumikamittal7 changed the base branch from future to main January 19, 2026 11:13
@angelhof
Copy link
Member

@bhumikamittal7 Since your change was just one line of code you could also just remake the change in a new branch forking off main!

@angelhof
Copy link
Member

Fixes #748

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

OS:ubuntu-24.04
Mon Feb 2 16:27:33 UTC 2026
intro: 3/3 tests passed.
interface: 43/43 tests passed.
compiler: 100/100 tests passed.

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

OS:ubuntu-24.04
Mon Feb 2 17:19:44 UTC 2026
intro: 3/3 tests passed.
interface: 43/43 tests passed.
compiler: 100/100 tests passed.

Signed-off-by: bhumikamittal7 <bhumika@amuselabs.com>
@github-actions
Copy link

github-actions bot commented Feb 2, 2026

OS:ubuntu-24.04
Mon Feb 2 18:56:34 UTC 2026
intro: 3/3 tests passed.
interface: 43/43 tests passed.
compiler: 100/100 tests passed.

Signed-off-by: bhumikamittal7 <bhumika@amuselabs.com>
Signed-off-by: bhumikamittal7 <bhumika@amuselabs.com>
@github-actions
Copy link

github-actions bot commented Feb 2, 2026

OS:ubuntu-24.04
Mon Feb 2 19:02:09 UTC 2026
intro: 3/3 tests passed.
interface: 43/43 tests passed.
compiler: 100/100 tests passed.

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

OS:ubuntu-24.04
Mon Feb 2 19:04:36 UTC 2026
intro: 3/3 tests passed.
interface: 43/43 tests passed.
compiler: 100/100 tests passed.

@bhumikamittal7
Copy link
Author

@angelhof Thanks for the comments and updates! Sorry for the delayed response on my end — I was swamped with work for the last few weeks and only got a chance to look into this today.

I am closing this PR and have opened a new one from a branch forked off main:
#784

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants