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

Be slightly more context aware of what is a doc comment and what isn't #2196

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sortraev
Copy link
Collaborator

@sortraev sortraev commented Nov 27, 2024

This PR is a QoL change which I hope will benefit more people besides just myself.

I often find myself commenting out lines beginning with a forward pipe, e.g. in a chained pipe expression such as this simple (but real) example:

def xs `filterBy` ps =
  zip xs ps
  |> filter (.1)
  |> map (.0)
  -- |> unzip
  -- |> (.0)

which will give a parsing error because lines 5 and 6 will have been lexed as doc comments -- in this case, an "unexpected EOF" error because a declaration is expected after line 6, but the error message on unexpected doc comments can also be e.g. "unexpected token" or "Doc comments can only precede declarations" in other contexts (as a side-note, it would be cool to uniformize the error messages given on unexpected doc comment lines).
EDIT: if the next token is a declaration then no error is thrown, but the comment will parse as a doc comment to the succeeding declaration. This is perhaps the trickiest case.

I'd like to be able to quickly comment out such lines without having to change my code comment plugin rules (which is easier but not so portable) -- this PR imlements the easy solution, which is to not lex as a doc comment if the | is followed immediately by a >. This is compatible with futhark-doc since that tool uses the same parser.
This change does not cover other (more or less plausible) cases, such as a code line starting with a vertical bar (bitwise OR) -- a better solution is perhaps to parse as a doc comment only if the lookahead is a declaration (but I don't know how to implement that).

I somewhat often find myself commenting lines beginning with a forward
pipe (e.g. when chaining pipe operators) and having to manually insert
an extra dummy character to avoid the "Doc comments can only precede
declarations" parser error. With this change a `DOC` token is only lexed
if the `|` is not *immediately* succeeded by a `>`.
@sortraev sortraev marked this pull request as draft November 28, 2024 10:51
@athas
Copy link
Member

athas commented Dec 16, 2024

An alternative way of doing this would be to require doc comments to have a space after the |.

@sortraev
Copy link
Collaborator Author

sortraev commented Dec 16, 2024

Yes! I thought about this one, and it might be the best solution.
However, this would unfortunately fail in certain other (granted, contrived and style-aberrant) cases, such as:

def foo x = x
            | 42
def bar = id

where (auto-)commenting line 2 gives:

def foo x = x
            -- | 42
def bar = id

where now the second declaration parses as having a doc comment. :(

I have been a little back and forth since opening this PR, and I can't think of an unambiguous solution aside from changing the doc comment prefix to use a symbol which could never appear anywhere in valid Futhark code, e.g. -- @.

There obviously exist solutions which do not involve breaking stuff. For example, GHC separates parsing the AST and attaching Haddock comments into two separate passes (GHC.Parser.parseModule). I believe this removes all ambiguities in compilation, but not in docs generation, as evident when you try to Haddock this basic example:

data Foo =
  Bar
-- | Baz

x :: Int
x = 42

@sortraev
Copy link
Collaborator Author

Actually, enforcing the extra space after the vertical bar is a good temporary solution! This will also serve to assert that source code doc strings which are missing that extra space are stored correctly (at present a doc string such as -- |Foo is stored as oo)

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

Successfully merging this pull request may close these issues.

2 participants