chipsalliance / verible

Verible is a suite of SystemVerilog developer tools, including a parser, style-linter, formatter and language server
https://chipsalliance.github.io/verible/
Other
1.33k stars 200 forks source link

Single-line-code in block without begin/end folded together with preceding end-line comment. #657

Open cxcxcxcx opened 3 years ago

cxcxcxcx commented 3 years ago

Test case

// Input to the formatter, preferably a reduced test case.
module abcd;                                 
 always @(posedge clk or negedge rst_n) begin
   if (~rst_n) begin                         
     d <= 1;                                 
   end else if (abc)                         
     // comment                              
     d <= 4;                                 
 end                                         
endmodule                                    

Include any options or configuration used.

Actual output

-: Error lex/parsing-ing formatted output.  Please file a bug.      
First error: token: "end" at 6:3-5; problematic formatter output is 
module abcd;                                                        
  always @(posedge clk or negedge rst_n) begin                      
    if (~rst_n) begin                                               
      d <= 1;                                                       
    end else if (abc)  // comment     d <= 4;                       
  end                                                               
endmodule                                                           
<<EOF>>                                                             
hzeller commented 3 years ago

Good find. Luckily the formatter will make sure to fail-safe and not modify the file when it discovered the discrepancy.

We can reduce that even further; it is not necessarily the 'else' part, but just the fact that a single line 'block' is spanning multiple lines of which there is an end-of line comment is then folded into a single line:

module abcd;                                 
 always @(posedge clk or negedge rst_n) begin
   if (~rst_n)                         
     // comment in multiline without begin/end
     d <= 1;                                 
 end                                         
endmodule        

(the diagnostic output then shows that it would arrive at the following and thus skips the formatting for this file)

module abcd;
  always @(posedge clk or negedge rst_n) begin
    if (~rst_n)  // comment in multiline without begin/end     d <= 1;
  end
endmodule

Humans will also have a problem reading such code, so adding a begin/end around such code that needs more than one line is probably a good idea in general. So while the formatter should of course deal with this correctly, maybe this should also be a linter rule to find potentially error-prone code in the first place ?

cxcxcxcx commented 3 years ago

I agree this is not good practice and it's a good idea for linter to complain.

And, I feel a formatter should be robust to such cases. The code itself is not ambiguous.

If it were not the checker, the problem also sounds very scary.

In fact, I think it works when there are multiple lines of comments. One line comment is when it fails.

Henner Zeller notifications@github.com 于 2021年1月28日周四 20:03写道:

Good find. Luckily the formatter will make sure to fail-safe and not modify the file when it discovered the discrepancy.

We can reduce that even further; it is not necessarily the 'else' part, but just the fact that a single line 'block' is spanning multiple lines of which there is an end-of line comment is then folded into a single line:

module abcd; always @(posedge clk or negedge rst_n) begin if (~rst_n) // comment in multiline without begin/end d <= 1; end endmodule

(the diagnostic output then shows that it would arrive at the following and thus skips the formatting for this file)

module abcd; always @(posedge clk or negedge rst_n) begin if (~rst_n) // comment in multiline without begin/end d <= 1; endendmodule

Humans will also have a problem reading such code, so adding a begin/end around such code that needs more than one line is probably a good idea in general. So while the formatter should of course deal with this correctly, maybe this should also be a linter rule to find potentially error-prone code in the first place ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/verible/issues/657#issuecomment-769559909, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZUDHZEVEGU74HFGQB6CTS4IXQNANCNFSM4WV3UFXQ .