bs-classic: Swapping precedence of $ and :=

[rossc719g]

I made a pull request for this: #566
But I was curious what others have to say.

This only applies to the bluspec-classic (bluespec haskell?) language.

The precedence of the $ and := operators are currently defined in Prelude.bs as:

infixr  0 $
infixr  1 :=
...

I would like to switch them to:

infixr  0 :=
infixr  1 $
...

Things like this are not currently legal:

fooReg := f a $ g b c
fnReturningAReg $ f a b := 0
fnReturningAReg $ f a b :=  f a $ g b c

since they would be interpreted as this:

((fooReg) := (f a)) (g b c)
(fnReturningAReg) ((f a b) := (0))
(fnReturningAReg) (((f a b) := (f a)) (g b c))

This change would have them interpreted as:

(fooReg) := (f a (g b c))
((fnReturningAReg) (f a b)) := (0)
((fnReturningAReg) (f a b)) := ((f a) (g b c))

Making them legal.

What do you think?
Is there a good reason to keep the precedence as is?

The := always just returns Action, right?
So, It seems like the first form fooReg := f a $ g b c would never make sense currently.

The second form, might make sense as fnTakingAnAction $ f a b := 0. But this seems rare?
Maybe a better solution might be to set them both to the same precedence?

infixr  0 $
infixr  0 :=
...

I'd love to hear other peoples thoughts on this.

[mieszko]

there was a bit of informal discussion about this offline; i'll try to summarize it below.

there seems to be a general agreement that := should not have higher precedence than $, based on syntax of let and so on, as well as intuition from languages with mutable assignment operators. so, unless there is a sudden revolt, the remaining question is whether $ should the same precedence as := or higher.

i had a cursory look around the bs libraries and found some instances where $ and := appear on the same line (there are possibly more across lines, but i didn't look). let's have a look at these as well as one of your examples.

example 1, from ActionSeq.bs:

   addRules $ mkRules go var (as (go := False))

rewriting this as

   addRules $ mkRules go var $ as $ go := False

would seem natural (perhaps this is because of haskell, but then i guess most people who choose to use bs classic have at least some familiarity with haskell), but this would require := to bind tighter, at least on the right.

example 2, from SRAM.bs:

   dta := (pack $ case req of
                   Read _ -> _
                   Write (_, value) -> value
          )

in this case the parens look really out of place and it would indeed be nice to write instead

   dta := pack $ case req of
                  Read _ -> _
                  Write (_, value) -> value

now this suggests that $ should bind tighter, at least on the right.

example 3, from your post:

   fnReturningAReg $ f a b := 0
   fnReturningAReg $ f a b :=  f a $ g b c

now this would require $ to bind tighter, at least on the left.

so the two potential solutions are:

A. make $ bind tighter (with precedence 1) than := (at precedence 0), which means no parens in examples 2 and 3 but parens in example 1, and

B. make $ and := have the same precedence (i guess both infixr 0).

from the offline discussion, there seems to be a general agreement that it would be preferable to write example 2 without the parentheses, and that for example 3 putting the left-hand-side in parens would be at least stylistically preferable and would make the code more readable. some folks have a preference for no parens in example 1 as well, while others don't seem to mind either way.

personally, i would prefer both examples 1 and 2 without parens, and 3 with parens, meaning that i would vote for solution B.

i guess this would mean making both infixr 0 as you suggested in your second option. would you be on board with that?

if there is a consensus on this, perhaps we can leave this open for two weeks to allow any interested parties™ to comment and then make the change?

meanwhile i'll see if i can add some parse tests for this that rely on things functioning as they are right now and should break if / when the precedence changes are made.

[mieszko]

meanwhile, i also had a look at how := works. it turns out that it's a special grammar production that results in its own CSyntax node (Cwrite). it seems that this is still governed by the fixity declared in the prelude, so changing that would have the parse-time effect you're looking for even though you can't actually otherwise redefine the semantics in .bs sources.

but that's not all. because := has its own grammar production, it also has its own pretty-print code (instance of PPrint). and this should match, so that if you successfully compile a file with -dparsed you can feed the dump output back into the compiler and get the same thing. so this would need to be changed in addition to the fixity declaration in the Prelude.

incidentally, in the current tree the prettyprinting is bugged as well (see #568), but this seems worth fixing with the fixity change.

[rossc719g]

Yeah agreed.

After trying both alternatives in my code and thinking on your examples here, I am inclined to agree that setting the same precedence for $ and := (infixr 0) appears to be the more preferable choice. I feel like it aligns well with readability and intuition.

Waiting a few weeks for additional opinions seems wise.

The part about the implications on pretty-printing is interesting. I was unaware of all that. I am not sure I'll be able to make the change you're suggesting though, at least without some guidance. Is this something you would want me to attempt in my pull request?

Thanks again for your attention to this!

[mieszko]

So I looked at the source thinking I'd give you some pointers, and... I discovered that pretty-printing infix ops is actually broken altogether (#571). So it seems unfair to ask you to fix that as well :)

I did, however, add some tests for $ vs := precedence in the meantime. Do you think you could update your PR to be relative to that? You should be able to run make check in testsuite/bsc.syntax/bh to avoid running most of the testsuite, and still trigger those tests. Some of the $/:= tests should fail for you with the fix, but it's a matter of changing which should pass or fail in bh.exp in that dir.

Otherwise it seems nobody is objecting and we can merge the updated PR.

What do you think?

[rossc719g]

I discovered that pretty-printing infix ops is actually broken altogether (#571). So it seems unfair to ask you to fix that as well :)

I appreciate that!

I did, however, add some tests for $ vs := precedence in the meantime. Do you think you could update your PR to be relative to that? You should be able to run make check in testsuite/bsc.syntax/bh to avoid running most of the testsuite, and still trigger those tests. Some of the $/:= tests should fail for you with the fix, but it's a matter of changing which should pass or fail in bh.exp in that dir.

Happy to, but... , I may be missing something (I'm not very familiar with this repo), it seems that all 23 tests in testsuite/bsc.syntax/bh still pass for me with this change in place. Do you see them failing for you?

Otherwise it seems nobody is objecting and we can merge the updated PR.

What do you think?

Sounds great to me. :-)

[quark17]

The tests were added after your PR was submitted, so they wouldn't be in your branch. You'll need to sync your fork (pull the latest commits into your 'main' branch) and then bring those commits into your PR branch (by rebasing the branch, say).

A few things to note.

Mieszko mentioned the directory testsuite/bsc.syntax/bh, but there is also the directory testsuite/bsc.syntax/bh_parse_pretty which also needs to be updated. Both directories have new files added:

bh/DollarColonEqualsPrecedence1.bs
bh/DollarColonEqualsPrecedence2.bs
bh/DollarColonEqualsPrecedence3.bs

bh_parse_pretty/DollarColonEqualsPrecedencePretty1.bs
bh_parse_pretty/DollarColonEqualsPrecedencePretty2.bs
bh_parse_pretty/DollarColonEqualsPrecedencePretty3.bs

I observe the following differences, when the precedents are made the same:

Running ./bh/bh.exp ...
FAIL: `DollarColonEqualsPrecedence3.bs' shouldn't compile

Running ./bh_parse_pretty/bh-parse-pretty.exp ...
XPASS: `DollarColonEqualsPrecedencePretty3.bs' compiles, pretty-prints, and compiles again (PRMS github#568)

For bh: Examples 1 and 2 have the same code, but differ in the type signature according to the two directions of precedence (one should compile and one should fail). If we had swapped the precedence, I would expect to see a switch behavior; since we've made them the same precedence, I think it's OK that the behavior has not changed for those -- but have a look and make sure you agree. Example 3 used to fail, but now passes, so that just needs to be updated in the .exp file.

For bh_parse_pretty: The three examples have parentheses, so the input file isn't testing the resolution of precedence; the parsed source is printed to a file and a read back in, so we're testing that BSC has printed the code in a way that parses. At the moment, BSC leaves out all parentheses (which is a separate bug to fix). This used to mean that example 3 would fail, because the parentheses were needed for proper parsing, because the precedence. But now that precedence is changed, the example is correct without parentheses, so the example passes -- unexpectedly, hence XPASS. So that just needs changing in the .exp in that directory.

Note that when you run make check in a test directory (not at the top of the testsuite directory), it runs tests in two steps and therefore prints two reports. First it runs the tests in the current directory (and prints a report) and then it runs all the subdirectories (and their subdirectories) and prints a report. This is a quirk -- and if anyone knows how to fix it to do all the tests together, that'd be appreciated -- which means that you just need to be aware to look in two places. You said that there were 23 tests, but that's only the subdirectory report; the bh directory has 80+ tests itself (which you should see if you scroll up). Note also: if the first report has a failure/xpass, the system will stop there, and you'll just get that one report; it will only proceed into the subdirectories if the first step cleanly passed.

[rossc719g]

The tests were added after your PR was submitted, so they wouldn't be in your branch.

Ah, that was it! Thanks. Sorry, I'm still relatively new to life with git.

For bh:
For bh_parse_pretty:

That all makes sense to me. (Thanks, this explanation was very helpful).
I've updated the pull request.

Note that when you run make check in a test directory [...] it runs tests in two steps and therefore prints two reports.

Ah! I was fooled by this quirk. Now that I know to look for it, it makes sense. Thanks again.

Also, just thought I should mention:
A few times when running the checks, I got an unexpected failure:

XFAIL: `StructDeepPattern.bs' should compile (PRMS 1080)

Not sure what that was about, but it seemed to go away and come back a few times, and now I cannot make it come back to investigate further.
Unfortunately, I did not look at StructDeepPattern.bs.bsc-out when it was happening, so I don't know more.