GNOME Bugzilla – Bug 611085
Comments at the end of certain blocks (e.g. "try") break compilation
Last modified: 2018-05-22 13:30:20 UTC
Created attachment 154693 [details] Example code to test compilation of try..except block with problematic comment In Genie, for try .. except blocks, indented comments at the end of the try block break compilation with valac. Although the given example is specific to try .. except I believe that I had the problem with other blocks, too and the problem might thus not be related to try .. except statements. I however failed to reproduce now. try print "called" // this comment breaks compilation, remove this line to test except e: GLib.Error print "unreachable" The (afaiu bogus) error messages are: block_ends_with_comment.gs:19.10-19.10: error: syntax error, expected `:' but got identifier with previous `except' except e: GLib.Error ^ block_ends_with_comment.gs:22.1-22.4: error: syntax error, expected `finally' but got `init' with previous `dedent' init ^^^^ Somewhat bloated example code is attached.
It does not fail with /* comment */ - style comments Obviously, it should be ++recursion.
I'm able to reproduce the bug with if/else and do/while statements, as well as class and function declarations. This is valac version 0.30.0 . if/else breaks: [indent=4] init var i = 0 if i < 0 i += 1 // comment else i -= 1 Errors: trailing_comment.gs:8.9-8.9: error: syntax error, expected `:' but got end of line with previous `else' else ^ trailing_comment.gs:10.1-10.0: error: syntax error, expected identifier do/while breaks: [indent=4] init var i = 0 do i++ // comment while i < 10 Errors: trailing_comment.gs:9.1-9.1: error: syntax error, embedded statement cannot be declaration ^ trailing_comment.gs:9.1-9.0: error: syntax error, expected `while' but got end of file with previous `dedent' Certain function declarations break (trailing indented comment followed by a declaration): [indent=4] init nop() //indented comment def nop() pass Error: trailing_comment.gs:6.5-6.7: error: syntax error, expected `:' but got identifier with previous `def' def nop() ^^^ Certain class declarations break (trailing indented comment again): [indent=4] class Nothing construct() pass // indented comment class Nada construct() pass init pass Errors: trailing_comment.gs:6.7-6.10: error: syntax error, expected `:' but got identifier with previous `class' class Nada ^^^^ trailing_comment.gs:7.5-7.14: error: `Nothing' already contains a definition for `.new' construct() ^^^^^^^^^^ trailing_comment.gs:3.5-3.14: note: previous definition of `.new' was here construct() ^^^^^^^^^^ Removing the comment in each example corrects the errors.
The problem appears to stem from the interaction between count_tabs() and comment() in the Genie scanner (vala/valageniescanner.h). count_tabs() reads indentation, skips whitespace and comments, then checks for a trailing '\n'. If the '\n' is there, count_tabs() assumes the whole line is empty and returns a reset value to indicate as much. Unfortunately, comment() strips the trailing '\n' character. This leads count_tabs() to continue reading indentation on the following line, which causes the indentation level to be corrupted. At least part of the problem is that comment() is doing some of count_tabs()'s job as a side-effect. Specifically, it's stripping the trailing '\n' and resetting the indentation level for single-line comments. Here is the code in question from comment(): /* do not ignore EOL if comment does not exclusively occupy the line */ if (current[0] == '\n' && last_token == TokenType.EOL) { current++; line++; column = 1; current_indent_level = 0; } (It's not clear to me if this code is strictly necessary or just a shortcut to avoid calling the normal newline-handling code. The overlap in functionality between this code and other code in the scanner suggests that it's a shortcut.) Retaining the '\n' after consuming the single-line comment (and removing the related side-effects -- that is, removing the block of code shown above) corrects the problem. This approach also allows both the normal empty-line handling code and the tab handling code to read or consume the trailing '\n' as they would if there was no comment. This seems to be a more correct way of handling this case since it simply removes the comment without altering any other state. Any thoughts on anything here?
I'm liking your analysis. I've not looked at that area of code myself yet, but it sounds very plausible. If you are looking to hack on the scanner then I would suggest submitting some tests first so we have something to compare. Your examples are very useful in the earlier comment. I presume the comment() function also handles /* */ style multiline comments so they should be part of the tests to ensure they don't break. If you can come up with a simple fix then great. Otherwise from my own thinking there maybe should be a more consistent way of counting tabs. For example the following won't compile: init print "two tabs create one indent" if true == true print "yes" else print "no" but add an extra tab before 'else' and it will. From the improve anonymous functions work I'm liking the idea of struct StateStackItem { public State state; public int indent; } see https://bug746704.bugzilla-attachments.gnome.org/attachment.cgi?id=305910 Maybe that struct could be a way forward for consistent counting of indents/dedents. Sorry my comments are high level and vague, but I hope it gets you thinking about the problem more. Thanks for your interest in Genie.
Created attachment 312685 [details] Test various compile-time and runtime errors produced by bug 611085 (Genie code) This attachment is a Genie program that tests the following scenarios as well as the multiline comment variants and indent/dedent/"nodent"/post-expression variants. An 'if' statement followed by an 'if' statement. This includes the type of compile-time errors discussed here already as well as runtime errors like the following failing code: [indent=4] init var hit = false if false pass //comment if true hit = true assert(hit is true) An if/else block. An 'if' statement followed by an assignment. For example, the following test fails: [indent=4] init var hit = false if false pass //comment hit = true assert(hit is true) A try/except block. A do/while block. A class definition followed by another. A function definition followed by another.
Created attachment 312686 [details] Regression test for file comments (Genie code) This attachment is a Genie program that ensures file comments are not adversely effected by the correction to this bug. File comments are extracted by calling parse_file_comments() on a Genie scanner before executing normal scanning. I don't know the typical use-case for extracting file comments so the test operates directly on a scanner. The results accurately represent the scanner's behavior as of valac 0.30.0 although the actual values may not be ideal.
Firstly, thanks for such an impressive number of tests! It looks as though you've uncovered most(all?) cases where the problem happens. I think the runtime errors you describe are the same problem because the compiler does give a warning about hit=true being unreachable code. So if it never executes at run time then the assertion will always fail. The warning is created by the semantic analyser, which does its work after the Genie parser. As to why the Genie parser lets those ones through, but generates a syntax error for the other examples I don't know. I think file comments are comments that get passed through to the generated C file. Doc comments, the /** */ variety, will get included in the C file if before anything that gets turned into a function in the C code. I have attached an example Genie program. Compile this with the --ccode switch and you have see where the comments appear. The parse_file_comments() function simply takes any comment at the beginning of the file and puts it in the C. Used for copyright notices - see https://git.gnome.org/browse/vala/commit/vala/valageniescanner.vala?id=8ccc4ebba62a315243fa05d9e19c97071681d9dd Your tests for parse_file_comments are an interesting way of doing unit tests on the Vala compiler. The code section you identified in your analysis seems like it needs modifying somehow. So it could be a simple fix. What I've found so far is it was modified in response to https://bugzilla.gnome.org/show_bug.cgi?id=592369 See the commit https://git.gnome.org/browse/vala/commit/vala/valageniescanner.vala?id=1382bccbdf331238fbae094835942fcd140887a0 The other commit I could find relating the scanning of comments is https://git.gnome.org/browse/vala/commit/vala/valageniescanner.vala?id=3fe8c8aa53c5c0bb074b7f45db873ded5afccd8e I've written up some notes on submitting patches - https://wiki.gnome.org/Projects/Genie/Developing I find it useful to have a check list to run through. Thanks for making so much progress on this. Al
Created attachment 312696 [details] Examples of comments being copied into the C file Compile with the --ccode flag. The copyright comment at the beginning should be included in the C file. Along with all doc comments (i.e. with double asterisk at the beginning, /** comment here */ ) that appear before Genie code that is translated to C functions.
Created attachment 312697 [details] Examples of comments being copied into the C file Compile with the --ccode flag. The copyright comment at the beginning should be included in the C file. Along with all doc comments (i.e. with double asterisk at the beginning, /** comment here */ ) that appear before Genie code that is translated to C functions.
Where "count_tabs ()" and "comment ()" are conflicting ? In "count_tabs ()". "count_tabs ()" calls space () which calls: "while (whitespace() || comment ()) {}" Widely about it: "read_token (" while (skip_newlines ()) current_indent_level = count_tabs (); /* if its an empty new line then ignore */ if (current_indent_level == -1) { continue; } Decision: Comment "bad part" which passes "\n" in "comment ()". It's normal, because just after going out "comment ()" there is a code, handling unhandled EOL symbols. Where "count_tabs ()" is used ? - in "read_token (out SourceLocation token_begin, out SourceLocation token_end)" Where "comment ()" is used ? - in skip_space_tabs () - in space () - in parse_file_comments () - in pp_space () Where "space ()" is used ? - in "count_tabs ()" - in "read_token (out SourceLocation token_begin, out SourceLocation token_end)" (2 times) What "space ()" do ? Skips while whitespace or comment. So helped just commenting that "shortcut", that P. Baldwin told. This tests are passed: tests: https://bug611085.bugzilla-attachments.gnome.org/attachment.cgi?id=312685 output: http://pastebin.com/tkxGU7WV This comments appear in .c files too: comments: https://bug611085.bugzilla-attachments.gnome.org/attachment.cgi?id=312697 result .c file: http://pastebin.com/qY5ASmzg This tests I could not do because of errors: regression tests: https://bug611085.bugzilla-attachments.gnome.org/attachment.cgi?id=312686 errors: http://pastebin.com/ZdhwLyU4
Created attachment 344002 [details] [review] Patch with commented (disabled) "shortcuts" Patch for it. "Shortcut" for skipping new lines was disabled. Seems it works well.
Please, try it. Do we need to remove that (commented) part ?
Created attachment 344007 [details] [review] code cleaning from extra spaces\tabs This is a little code cleaning from extra spaces\tabs, that appeared by mistake after I saved it with Atom text editor.
What version do you patch against? Because we have a patch, may Genie maintainers would like to review it before close this bug.
@Daniel Espinosa, I tried this on the last dev branch from near 2017-01-22 (I downloaded it near a week earlier). I asked Rico Tzschichholz about this bug: if we have working decision (it passed some tests), we could close it. Probably he corrected this patch.
Review of attachment 344002 [details] [review]: Removing this if-block seems to be correct. Due to the lack of syntax tests it is still risky to do so.
Created attachment 350775 [details] [review] new patch After that my patch almost all corrected, but was little bug, that sometimes broke comments. Corrected it, added tests. Here Python script for launching tests is newer, than here: https://bugzilla.gnome.org/show_bug.cgi?id=781966 So while merging, better to replace that with this.
Newer Python script for launching tests (updated to run under Python 3.4): https://github.com/vlad1777d/Genie/blob/genie-improved/run_genie_tests.py
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/75.