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

start_line behaviour was changed at v2 #558

Closed
dalance opened this issue Jan 20, 2025 · 5 comments
Closed

start_line behaviour was changed at v2 #558

dalance opened this issue Jan 20, 2025 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@dalance
Copy link

dalance commented Jan 20, 2025

I'm debugging some test failures at veryl-lang/veryl#1169,
and found a behaviour change about start_line.

I added the following code, and executed cargo test -- "formatter::test_47_embed" --nocapture.

diff --git a/crates/formatter/src/formatter.rs b/crates/formatter/src/formatter.rs
index c1cda965..e5484a2c 100644
--- a/crates/formatter/src/formatter.rs
+++ b/crates/formatter/src/formatter.rs
@@ -210,6 +210,12 @@ impl Formatter {
     fn process_token(&mut self, x: &VerylToken, will_push: bool) {
         match self.mode {
             Mode::Emit => {
+                dbg!(x.token.line);
+                dbg!(x.token.text.to_string());
+                for x in &x.comments {
+                    dbg!(x.line);
+                    dbg!(x.text.to_string());
+                }
                 self.push_token(&x.token);

                 let loc: Location = x.token.into();

The results are below:

  • master branch (parol v1)
crates/formatter/src/formatter.rs:216:21] x.line = 11
[crates/formatter/src/formatter.rs:217:21] x.text.to_string() = "// comment\n"
  • parol_v2 branch
[crates/formatter/src/formatter.rs:216:21] x.line = 12
[crates/formatter/src/formatter.rs:217:21] x.text.to_string() = "// comment\n"

The x.line comes from location.start_line of parol_runtime::lexer::token::Token.
I think the expected start_line is 11.

@jsinger67 jsinger67 added the bug Something isn't working label Jan 20, 2025
@jsinger67 jsinger67 self-assigned this Jan 20, 2025
@jsinger67
Copy link
Owner

Hi @dalance ,
thank you for reporting.
I can confirm that this is a bug. I guess in the scnr crate but I have to analyze this more thoroughly.

@jsinger67
Copy link
Owner

I found the problem and will prepare new releases.
It was a combined problem in both crates parol_runtime and scnr.
The problem was related to scanner mode switching at the end of the file combined with the new feature of lossless parse trees in version 2.
I'll fill you in when new releases are available.

Again, thanks for reporting 👍

@jsinger67
Copy link
Owner

The mentioned test succeeds in my fixed environment:

running 1 test
[crates\formatter\src\formatter.rs:213:17] x.token.line = 1
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = ""
[crates\formatter\src\formatter.rs:213:17] x.token.line = 1
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "module"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 1
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "Module47"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 1
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "{"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 1
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "}"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "embed"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "("
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "inline"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = ")"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "sv"
[crates\formatter\src\formatter.rs:213:17] x.token.line = 3
[crates\formatter\src\formatter.rs:214:17] x.token.text.to_string() = "{{{\r\nmodule test;\r\n initial begin\r\n $display("hello");\r\n end\r\nendmodule\r\n}}}"
[crates\formatter\src\formatter.rs:216:21] x.line = 11
[crates\formatter\src\formatter.rs:217:21] x.text.to_string() = "// comment\r\n"
test formatter::test_47_embed ... ok

@jsinger67
Copy link
Owner

@dalance,

Good news. New releases are available.

  • scnr 0.7.1
  • parol_runtime 2.1.1
  • parol 2.1.2

scnr is a dependency of parol_runtime, therefore I only mention it for the sake of completeness.

All tests on branch parol_v2_migration in my clone are finally passing.

I'll close this issue for now. Feel free to reopen it in case of any problems or create a new one.

Keep in mind that version 2 isn't really production ready and occasionally bugs can occur.
That's why I'm all the more pleased about constructive feedback.

@dalance
Copy link
Author

dalance commented Jan 22, 2025

Thank you for your work!
I also confirmed all tests are passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants