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

Fix sail-fmt block_comment indent #581

Closed
wants to merge 4 commits into from
Closed

Conversation

trdthg
Copy link
Contributor

@trdthg trdthg commented Jun 11, 2024

Hi, I'm going to test sail-fmt on riscv-sails
like this PR did riscv/sail-riscv#264, and then try to fix them one by one


This PR is for fix an error when formatting something like

/*! hahaha*/

Syntax error.
. /test/format/unbalanced_comment.sail:2.0-0: | current token: .
  | current token: . 

seems to be related to #237


I have a few questions, could you explain

  • why is the doc_comment here not added to the comments?
  • and why lexbuf.lex_start_p <- startpos; it looks strange
  • what is the Doc, I can't find it in any other code (I am just a newbie to ocamllex :|).

Copy link

github-actions bot commented Jun 11, 2024

Test Results

    9 files  ±0     20 suites  ±0   0s ⏱️ ±0s
  653 tests ±0    653 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 090 runs  ±0  2 089 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit 2289361. ± Comparison against base commit ac3e03a.

♻️ This comment has been updated with latest results.

@Alasdair
Copy link
Collaborator

Documentation comments are attached to the definitions they are associated with in the AST. Each top-level definition is annotated with a value of type def_annot, which optionally contains a documentation comment. This type is defined here:

type def_annot 'a = <|

When pattern matching on a toplevel definition you do something like

match def with
| DEF_aux (def, def_annot) -> (* def here is the actual definition within the aux wrapper *)
  let comment = def_annot.doc_comment in

You therefore don't need to add documentation comments to the comments variable — they will always be where you want them attached to the AST element they document.

'Doc' in the lexer is the token type for a documentation comment, there are then rules in the parser that create documented definitions like:

  | doc = Doc; def = def
    { DEF_aux (DEF_doc (doc, def), loc $startpos(doc) $endpos(doc)) }

the initial_check desugaring pass removes the separate DEF_doc constructors and attaches them into the annotation type I mentioned above here:

| P.DEF_doc (doc_comment, def) -> begin

Finally, the reason why the rule looks like:

let startpos = Lexing.lexeme_start_p lexbuf in
let arg = doc_comment startpos (Buffer.create 10) 0 false lexbuf in
lexbuf.lex_start_p <- startpos;
Doc arg

is because in Menhir the lexbuf positions are used to get the span information in each parsing rule. Ocamllex will automatically advance the lexbuf positions as it scans the documentation comment using the doc_comment rule, but we want menhir to see Doc as a single token with the correct start and end positions so we save the starting position and restore it after running the doc_comment rule.

@Alasdair
Copy link
Collaborator

Alasdair commented Jun 11, 2024

Actually in the context of the formatter you can ignore what I said about def_annot and DEF_aux, as it is run on the AST before initial_check.ml does the attaching, so you have the DEF_doc constructors the parser produces there.

@Alasdair
Copy link
Collaborator

I fixed the bug that caused documentation comments to fail here: #592

Should also mean they appear in the formatted output, although there may be indentation issues if whatever they are commenting is indented.

@trdthg
Copy link
Contributor Author

trdthg commented Jun 17, 2024

Looks good. I tried to modify ast before and added a DEF_doc_only to distinguish DEF_doc, and it also worked.
But your method does not modify ast, only affects the fmt part, I think your method is better.
As for indentation, I will test it later.

@trdthg
Copy link
Contributor Author

trdthg commented Jul 3, 2024

Hi, update here

the new code intend to fix indent issue
now the block_comment will be indented as rust-fmt, go-fmt, black(python)

  • the first line will keep nochanged
  • the middle line and the last line will be right shift if the lines's min indent < /*'s col

@trdthg trdthg reopened this Jul 3, 2024
@trdthg trdthg marked this pull request as ready for review July 3, 2024 08:39
@trdthg trdthg changed the title handle doc_comment as block_comment Fix sail-fmt block_comment indent Jul 3, 2024
@trdthg
Copy link
Contributor Author

trdthg commented Jul 21, 2024

close, because #619 merged

@trdthg trdthg closed this Jul 21, 2024
@trdthg trdthg deleted the fix-fmt branch July 21, 2024 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants