Fix off-by-one column tracking for $-prefixed identifiers

[sequencer]

Summary

Fix incorrect column number tracking when lexing $-prefixed identifiers (Verilog system tasks).

The pattern ('$':x:cs) consumes two characters (the $ and the first identifier character x), so the remaining input cs starts at column c+2, not c+1.

Problem

This bug caused incorrect column positions to be reported for Verilog system tasks like $display, $finish, $time, etc. All subsequent tokens on the same line after a $-identifier would have their column numbers off by one.

[Copilot]

Pull request overview

This PR fixes an off-by-one error in column tracking when lexing Verilog system task identifiers that start with $. The bug caused all tokens following a $-prefixed identifier on the same line to report incorrect column positions.

Changes:

• Corrected column offset calculation for $-prefixed identifiers from c+1 to c+2 to account for both the $ and first identifier character being consumed

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[quark17]

Hi, thank you for contributing!

In the future, it would be immensely helpful if you can provide a working example that demonstrates the problem. We also need to add a test case to the testsuite, to demonstrate that the change works and test for regression.

I was able to create the following example (after wasting some time trying to make a BSV example until I remembered that Lex is only used for BH):

package Test where

f :: Bool -> Action
f x = $fwrite(x)

The message from BSC is currently this:

Error: "Test.bs", line 4, column 13: (T0020)
  Type error at:
    x

  Expected type:
    Prelude.File

  Inferred type:
    Prelude.Bool

With your change, the column number for x is 14. That is the correct position, if we consider the first column to be 0 -- offhand I couldn't remember if BH counts columns this way, but I confirm that it does with another example. So I believe your change is correct.

We can also confirm that the position matches that for functions without the dollar sign:

package Test2 where

f :: Bool -> Action
f x = $fwrite(x)

g :: Bool -> Action
g x = dfwrite(x)

dfwrite :: File -> Action
dfwrite x = $fwrite(x)

Currently BSC reports column 13 for f and 14 for g.

I can add a testsuite example, unless you're able to do that? I think it should go in testsuite/bsc.syntax/bh/ and there is already a test LexPos.hs that is tested in bh.exp. That was different lexer position bug, and unfortunately the file name doesn't indicate it more specifically. We don't have to fix that now, but can start adding files with better names. For example, LexPos_TaskArg.bs. I would use Test2 (above) for the contents, and check for both positions in the bh.exp file.

Finally, was that Copilot review initiated by you? or is that something new in the repo settings that was caused on our end? If it's something you initiated, can you not do that in the future? If you want to use to Copilot to review your code before submitting a PR, that's fine, but I only need to see the final PR that you submit. That would save me time and reduce clutter on the PR.

[quark17]

I have a patch that updates the testsuite, but your repo settings do not allow maintainers of bsc to push new commits. I'll open a new PR to submit the changes.

[sequencer]

sorry for the delay reply, I was on the business trip this week, glad to see this has been merged! Thank you! @quark17