Skip to content

MPR#7701: avoid dropping docstrings in otherwise empty files#1562

Closed
Octachron wants to merge 3 commits intoocaml:trunkfrom
Octachron:lost_doc_comments
Closed

MPR#7701: avoid dropping docstrings in otherwise empty files#1562
Octachron wants to merge 3 commits intoocaml:trunkfrom
Octachron:lost_doc_comments

Conversation

@Octachron
Copy link
Copy Markdown
Member

As stated in MPR#7701, documentation comments in otherwise empty files can be silently dropped by the parser.

More precisely, the first documentation comment is dropped if it is not preceded by two empty lines. This happens due to the lexer trying to attach these documentation comments to the non-existing previous token.

This PR proposes to fix this issue by making the lexer considers that any documentation comment is always preceded by a blank line in the begining of a file.

Copy link
Copy Markdown
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly simple fix, I like it.

| Before(a, f, b), BlankLine -> Before(a, b @ f, [doc])
in
loop NoLine docs' lexbuf
loop !docs_initial_newline_state docs' lexbuf
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to pass NoLine explicitly here?
With the current implementation that is (I believe) the only possible value for !docs_initial_newline_state at this point.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, fixed.

set_pre_extra_docstrings pre_pos (List.rev a)
in
let rec loop lines docs lexbuf =
docs_initial_newline_state := NoLine;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you lift that write out of the loop?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, lifted.

Changes Outdated
to ill-typed columns
(Thomas Refis, with help from Gabriel Scherer and Luc Maranget)

- MPR#7701,GPR#1561: do not drop documentation comments in (otherwise) empty
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • MPR#7138

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that 7701 was a duplicate. Fixed.

@lpw25
Copy link
Copy Markdown
Contributor

lpw25 commented Jan 10, 2018

Can you guys wait to merge this until I've had time to have a proper look? (May be a few weeks). This code is quite subtle and I want to make sure that we are not just fixing a symptom of a deeper bug.

@Octachron Octachron requested a review from lpw25 January 10, 2018 13:27
@Octachron
Copy link
Copy Markdown
Member Author

Certainly.

@lpw25
Copy link
Copy Markdown
Contributor

lpw25 commented Apr 3, 2018

This was indeed a symptom of a deeper bug. #1693 is my attempt to fix that.

@Octachron
Copy link
Copy Markdown
Member Author

Good, do you want me to review #1693 ?

@Octachron Octachron closed this Apr 3, 2018
@lpw25
Copy link
Copy Markdown
Contributor

lpw25 commented Apr 3, 2018

That'd be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants