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

static-promotion: pretty printer error #1795

Closed
anshumanmohan opened this issue Nov 28, 2023 · 1 comment
Closed

static-promotion: pretty printer error #1795

anshumanmohan opened this issue Nov 28, 2023 · 1 comment

Comments

@anshumanmohan
Copy link
Contributor

There seems to be a small bug in the pretty printer.

In this commit I have pushed two files. a.futil and b.futil. The former was generated via the eDSL and the latter by running

cargo run -- a.futil -p static-promotion -m file > b.futil

The file b.futil has a stray closing paren here.

@sampsyo
Copy link
Contributor

sampsyo commented Dec 4, 2023

Super weird thing!!

I was able to reduce the test case. Here's a file that triggers the problem on a round-trip through the pretty printer:

static<1> component stats() -> () {
  cells {}
  wires {}
  control {}
}

component main() -> () {
  cells {
    stats = stats();
  }
  wires {}
  control {
    seq {
      static<1> invoke stats()();
    }
  }
}

To see the problem, run:

cargo run -- -d all that_futil.futil

You will see these offending lines:

      static<1> invoke stats()()
      );

Anyway, the problem seems to come from having a static invoke inside a seq... nothing more than that appears to be required.


If you're curious, most of the reduction work was done by Shrink Ray. Here was the original interesting test I used most of the time:

#!/bin/sh -x
set -e

CALYX=/Users/fabian/cu/research/futil/target/debug/calyx

# Check that the original program parses.
$CALYX -d all $1 > /dev/null

# Apply a pass and re-parse.
outfile="$1-compiled"
$CALYX -p static-promotion -m file $1 > $outfile

# Abort here (it's uninteresting) when parsing this file *succeeds*.
! $CALYX -d all $outfile > /dev/null 2> err.txt

# This is interesting if we find the error we're looking for.
grep 'expected identifier' err.txt

This test follows @anshumanmohan's advice directly (check that the original file parses, but passing it through the static-promotion pass and pretty-printing produces an invalid file). Shrink Ray left me with a big control nest and wasn't quite capable of deleting (for example) an entire if foo { ... } wrapper while preserving the body, so I helped it out from there. I then noticed that we could probably get the same effect by manually doing the promotion (writing static<1> in front of the target component and the invoke), so I changed the interestingness test to just look for stuff that failed two pretty-printing round-trips, which got us here.

Along the way, Shrink Ray discovered a fascinating fact I didn't know about our parser: it doesn't require whitespace between keywords and lots of stuff! For example, this file parses:

componentstats() -> () {
  cells {}
  wires {}
  control {}
}

componentmain() -> () {
  cells {
    stats = stats();
  }
  wires {}
  control {
    seq {
      invokestats()();
    }
  }
}

Tired of typing the space in component main? No problem! Calyx will parse componentmain just fine.

sampsyo added a commit that referenced this issue Dec 4, 2023
Fixes #1795, which identified a problem with the closing parentheses in
`static invoke` invocations.
sampsyo added a commit that referenced this issue Dec 4, 2023
A bonus fix, not directly discovered in #1795 but still obviously a
problem.
sampsyo added a commit that referenced this issue Dec 4, 2023
@sampsyo sampsyo closed this as completed in 75302d5 Dec 4, 2023
rachitnigam pushed a commit that referenced this issue Feb 16, 2024
* Pretty printer: fix a closing parentheses

Fixes #1795, which identified a problem with the closing parentheses in
`static invoke` invocations.

* Pretty printer: fix a stray semicolon

A bonus fix, not directly discovered in #1795 but still obviously a
problem.

* Save expect files affected by #1795
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

No branches or pull requests

2 participants