Add support for line comments of the form `//` #243

dxu · 2016-03-30T01:01:50Z

This adds support for line comments of the form // comment.

It updates the lexer to recognize //, by maintaining a single_line_comment bool indicating whether we're currently parsing a single line comment. this differentiation prevents cases like // */ let x = 10 from executing, so that the */ does not get parsed as an end of comment. It also maintains a separate line_comment_start_loc to keep track of the line comment locations to separate the logic for checking for unmatched comments, so that newlines and nested */ comment endings can be differentiated. It also updates the pretty printer to handle these cases accordingly.
There is an edge case with regards to interleaved comments having to maintain their "multiline" status.

For example, in the wrappingTest.re cases in facebook/Reason,

/*A*/
let named
    /* a::a */
    a::a
    /* b::b */
    b::b =>
  /* a + b */
  a + b;

/*B*/
let namedAlias
    /* a::aa */
    a::aa
    /* b::bb */
    b::bb =>
  /* aa + bb */
  aa + bb;

must be compiled into

//A
let named /* a::a */ a::a /* b::b */ b::b => /* a + b */ a + b;

//B
let namedAlias
    /* a::aa */
    a::aa
    /* b::bb */
    b::bb => /* aa + bb */ aa + bb;

as opposed to

//A
let named // a::a  a::a // b::b  b::b => // a + b  a + b;

//B
let namedAlias
    // a::aa 
    a::aa
    // b::bb 
    b::bb => // aa + bb  aa + bb;

to fix this, I check if the listConfig every defines its break attribute to be Never or IfNeed, it will force a multiline comment.

As per the discussion on facebook/ReasonSyntax#41, there is also the edge case of nested inline comments within a line comment. To handle this, there is a new error within the lexer, Unterminated_nested_comments that get thrown when a nested comment is detected to not have been matched.

There are a few open questions that aren't answered by this pull request, both of which can be found in greater detail in the discussion on facebook/reasonsyntax#41:

How should we handle line breaking? Should it be handled through easy_format (which is causing the current edge cases)?
Converting from ML <-> reason, you could potentially run into problems with nested comments - for example, if you had strings of either /* or (* converting from one to the other, it could potentially cause issues.

dxu · 2016-04-13T00:09:58Z

This is ready for review, sorry for the delay.

dxu · 2016-04-13T00:43:57Z

There is currently a failing test on master, i think a result of the docstring issue #308 . This PR does not commit the new expected output of all the tests for fear that broken tests get changed accidentally.

jordwalke · 2016-04-13T05:57:39Z

I realized that failing test is blocking other tests. I'll update the test.sh to make sure it doesn't block other tests from running. And I'll make sure the file name clearly indicates it's intended to be failing (I wanted it as a constant reminder for us to fix it :) )

jordwalke · 2016-04-13T06:56:45Z

Btw: I think this diff provides a really cool feature, and we should merge it in, but these two other comment-related issues are blocking launch:

#313
#308

I propose we merge this diff in after those other ones are handled.

dxu · 2016-04-13T07:17:49Z

No problem, this can wait. I'm still looking at #308 right now, it's been giving me a bit of trouble.

dxu · 2016-04-17T01:48:17Z

I've updated this review with the new test output, since a lot of comments were converted into single line comments

dxu · 2016-04-24T19:11:58Z

This rebase is failing a few tests due to an issue with empty line comments of the form //<newline>, I'm fixing it now and cleaning up this review.

I was thinking about extending the current lexer token COMMENT structure to include a boolean indicating whether or not it's a line comment, so that we can differentiate when printing it - if the developer specified comments of the form /* */ it would be kept in that form, if they used // then they would change it to that. My impression is that the only changes that would be necessary is updating the token definition in parser.mly and the creation in .mll, as well as the places we match on it in lexer.mll, toolchain.ml, and refmt_impl.ml. We'd probably also have to extend the Ocaml_lexer's comments() to return comments of the new form as well.

Is this something that would be useful/is there a better way to address this?

dxu · 2016-04-24T23:26:32Z

This is ready for review. I cleaned up the code a bit, and also added the extra boolean to the COMMENT definition, so I reverted all the tests back to the originals, since they're now just kept as multiline comments. I kept the commit history in case we decide this isn't intended behavior.

jordwalke · 2016-04-24T23:37:54Z

Cool, I'll take a look tonight! I greatly simplified the comment interleaving code (will submit a diff) but it looks like the rebase conflicts won't be too bad with this one.

jordwalke · 2016-04-25T10:08:46Z

Sorry for the delay, I'll take a look as soon as I land the other launch blocking comment diff (Yunxing can feel free to accept/comment).

dxu · 2016-04-25T17:19:36Z

No worries, take your time!

jordwalke · 2016-05-01T06:01:36Z

I'll be getting back to this. I think we should open source with support for these.

bobzhang · 2016-05-03T17:00:42Z

This is nice to have

dxu · 2016-05-07T02:14:30Z

rebased, but i noticed that there are a few ml tests that are failing, also on master.

Failed tests:
./mlVariants.ml -- typecheck_test
./mlVariants.ml -- unit_test
./mlVariants.ml -- idempotent_test

Makes it easier to find out what's wrong when you change `reason_parser.mly`.

comments for lists that break `IfNeed`ed

definition

bslatkin · 2016-06-27T05:12:24Z

Did this ever land? This page says that Reason supports line comments in the form // blah blah, but in practice I can't get them to work. /* blah blah */ comments work fine.

dxu · 2016-06-27T05:30:23Z

This unfortunately was never landed, and I think not fully reviewed. There was another issue regarding the comment interleaving logic introduced by another commit that is referenced above, which was causing issues at the time (#302). Due to some other responsibilities that cropped up in my personal life, I haven't gotten a chance to look at or address either in a while, so I'm not whether or not it's been fixed.

Also, this hasn't been rebased (as is apparent from the garbled history), but this PR also became bugged when the repo changed its name from facebook/Reason to facebook/reason (it caused my previous repo, dxu/Reason, to no longer be recognized as a fork), so there's a slightly more up-to-date fork at https://github.com/dxu/reason/tree/comments. It still isn't rebased with the most recent commits, but

Sorry for the delay! @yunxing, let me know if you wanted to take this over, otherwise I might not be able to get to this until a few weeks from now.

This pull request should also be closed and replaced with one from a repository that is detected as a fork.

yunxing · 2016-06-27T05:37:55Z

@dxu Thanks for the update. I will take over this diff as soon as I get a cycle next Thursday.

jaredly · 2016-09-02T01:40:25Z

bump! This would be super nice

yunxing · 2016-09-02T01:41:32Z

We have the necessary support in our comment interleaving logic now. Should be pretty easy, gimme some more days.

tekknolagi · 2017-02-27T18:54:48Z

@yunxing any update on this?

chenglou · 2018-01-10T06:30:50Z

Nooooooooo

tekknolagi · 2018-01-10T06:48:28Z

Does this work now?

chenglou · 2018-01-10T09:37:28Z

Nope

jordwalke · 2018-01-10T23:07:45Z

cc @let-def :D

jordwalke · 2018-02-13T06:19:17Z

:D
#1800

facebook-github-bot added the CLA Signed label Mar 30, 2016

dxu force-pushed the comments branch 2 times, most recently from 6a7b8ed to cbf37c2 Compare April 12, 2016 23:58

dxu changed the title ~~add tests for single line comments~~ Add support for line comments of the form // Apr 13, 2016

dxu force-pushed the comments branch 2 times, most recently from 9adc474 to d1a2070 Compare April 13, 2016 01:45

dxu force-pushed the comments branch 2 times, most recently from 1a0d7c7 to 9ee9901 Compare April 17, 2016 01:46

dxu force-pushed the comments branch 3 times, most recently from 3e562cd to a6ee21d Compare April 24, 2016 23:23

dxu force-pushed the comments branch from a6ee21d to a4e3e94 Compare April 25, 2016 17:32

dxu mentioned this pull request Apr 25, 2016

Add tests for expected errors #380

Merged

dxu force-pushed the comments branch from a4e3e94 to fb0992b Compare April 27, 2016 01:39

dxu force-pushed the comments branch from 464759a to 4dbb917 Compare May 7, 2016 02:13

dxu force-pushed the comments branch from 4dbb917 to 871919b Compare May 14, 2016 03:00

Yunxing Dai and others added 11 commits May 22, 2016 19:27

Fix the ocaml version at 4.02.3

73710e5

Create a Menhir conflicts file

3ff7fb3

Makes it easier to find out what's wrong when you change `reason_parser.mly`.

fix typos

230d1e9

Don't hardcode path to bash

3ead83a

add comments test input file and its expected output

4f2ab49

fix nested comments case

f9a4402

update lexer to handle line comment tokens

51c75aa

update pretty printer to print line comments while forcing multiline

4f85add

comments for lists that break `IfNeed`ed

update comments tests

8c95524

clean up single line comment code

f9c5a6c

add new tests and add is_line_comment boolean to lexer COMMENT

35f1604

definition

dxu force-pushed the comments branch from ce0be6a to 91f7709 Compare May 23, 2016 02:27

cleanup !single_line_comment, fix unmatched nested comment edge case

b235362

dxu force-pushed the comments branch from 91f7709 to b235362 Compare May 25, 2016 05:37

ghost added the CLA Signed label Jul 12, 2016

yunxing self-assigned this Aug 1, 2016

ghost added the CLA Signed label Aug 1, 2016

dxu unassigned yunxing Apr 24, 2017

dxu closed this Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for line comments of the form `//` #243

Add support for line comments of the form `//` #243

dxu commented Mar 30, 2016

dxu commented Apr 13, 2016

dxu commented Apr 13, 2016

jordwalke commented Apr 13, 2016

jordwalke commented Apr 13, 2016

dxu commented Apr 13, 2016

dxu commented Apr 17, 2016

dxu commented Apr 24, 2016 •

edited

Loading

dxu commented Apr 24, 2016

jordwalke commented Apr 24, 2016

jordwalke commented Apr 25, 2016

dxu commented Apr 25, 2016

jordwalke commented May 1, 2016

bobzhang commented May 3, 2016

dxu commented May 7, 2016

bslatkin commented Jun 27, 2016

dxu commented Jun 27, 2016

yunxing commented Jun 27, 2016

jaredly commented Sep 2, 2016

yunxing commented Sep 2, 2016

tekknolagi commented Feb 27, 2017

chenglou commented Jan 10, 2018

tekknolagi commented Jan 10, 2018

chenglou commented Jan 10, 2018

jordwalke commented Jan 10, 2018

jordwalke commented Feb 13, 2018 •

edited

Loading

Add support for line comments of the form // #243

Add support for line comments of the form // #243

Conversation

dxu commented Mar 30, 2016

dxu commented Apr 13, 2016

dxu commented Apr 13, 2016

jordwalke commented Apr 13, 2016

jordwalke commented Apr 13, 2016

dxu commented Apr 13, 2016

dxu commented Apr 17, 2016

dxu commented Apr 24, 2016 • edited Loading

dxu commented Apr 24, 2016

jordwalke commented Apr 24, 2016

jordwalke commented Apr 25, 2016

dxu commented Apr 25, 2016

jordwalke commented May 1, 2016

bobzhang commented May 3, 2016

dxu commented May 7, 2016

bslatkin commented Jun 27, 2016

dxu commented Jun 27, 2016

yunxing commented Jun 27, 2016

jaredly commented Sep 2, 2016

yunxing commented Sep 2, 2016

tekknolagi commented Feb 27, 2017

chenglou commented Jan 10, 2018

tekknolagi commented Jan 10, 2018

chenglou commented Jan 10, 2018

jordwalke commented Jan 10, 2018

jordwalke commented Feb 13, 2018 • edited Loading

Add support for line comments of the form `//` #243

Add support for line comments of the form `//` #243

dxu commented Apr 24, 2016 •

edited

Loading

jordwalke commented Feb 13, 2018 •

edited

Loading