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

Parse error for functions with arguments in next line #1

Closed
tunguski opened this issue May 29, 2016 · 14 comments
Closed

Parse error for functions with arguments in next line #1

tunguski opened this issue May 29, 2016 · 14 comments

Comments

@tunguski
Copy link

While testing your great library I've found this case:

import Html exposing (..)

main =
  div []
  [ text "Hello, World!" ]

that compiles on http://elm-lang.org/try, but does not parse on http://bogdanp.github.io/elm-ast/example/.

After modifying to div [] [ text "Hello, World!" ] (one line) code parses correctly.

Not sure do you work on full compatibility, but wanted to report you this issue.

shamansir added a commit to shamansir/elm-ast that referenced this issue Sep 28, 2016
@shamansir
Copy link
Contributor

shamansir commented Sep 28, 2016

Tried to fix it with changing:

application : OpTable -> Parser Expression
application ops =
  rec <| \() ->
    term ops `chainl` (Application <$ spaces')

to

application : OpTable -> Parser Expression
application ops =
  rec <| \() ->
    term ops `chainl` (Application <$ choise [spaces', whitespace])

but failed.

(multiple functions and module fixity scanning tests fail in this case, but the example passes)

@Bogdanp Bogdanp added the bug label Oct 1, 2016
@Warry
Copy link

Warry commented Oct 13, 2016

I made an attempt here: Warry@fa8bba8

So it works (seems as permissive as elm itself), but obviously not where indentation matters (case expressions and let ... in ... statements). That one will be tougher!

@wende wende changed the title Parse error for correct elm code Parse error for functions with arguments in next line May 3, 2017
@joonazan
Copy link
Contributor

I tried out what the Elm compiler does to some pathological cases. It seems pretty complicated: it allows reducing indentation after a function name, but it struggles when keywords like in are present.

I believe that a perfectly valid way to parse Elm would be to only care about whether there is indentation at all. If there is no indentation, parse it as the beginning of a statement.

@dynajoe
Copy link

dynajoe commented May 21, 2017

I think that some expressions that require indentation (i.e. case expressions) need to have all bindings on separate lines. I believe that is why shamansir's solution won't work (at least in part).

This currently works:

z = case Just 1 of Nothing -> 0 
Just y -> y

However, this should fail because Nothing -> 0 should be on a new line and indented.

This will require a change to the parser to track newline and indentation state.

@dynajoe
Copy link

dynajoe commented May 21, 2017

It seems that if there's only 1 binding for the case expression it can be on the same line as the of

z = case Just 1 of _ -> 12

@wende
Copy link
Collaborator

wende commented May 21, 2017

@joeandaverde Yup. But given that current parser is totally whitespace agnostic it looks like a major change to entire project

@joonazan
Copy link
Contributor

@joeandaverde I have thought about this a lot and made some experiments. Based on that I think that if it is ok to be less strict than the elm compiler, we do not need to track indentation.

It seems that lines containing =, -> or : are special, as they start a new statement or expression part. So just be whitespace-agnostic, but stop taking function arguments if the next line contains one of the aforementioned symbols.

I don't know what the best way to code this would be. Any suggestions? First thing I can think of is to just do a lookahead of the next line.

@dynajoe
Copy link

dynajoe commented May 22, 2017

Maybe I misunderstand your suggestion.

Would this still align with your thinking?

type alias Foo =
    { a : Int
    , b : Int
    }


type Bar
    = Apple { a : Int }
    | Baz { b : Int }

@wende
Copy link
Collaborator

wende commented May 22, 2017

@joonazan Yes. You're right. I even wrote a regex that tracks these lines but for some reason it doesn't want to work in Elm (only Regex 101)

(?:({-(?:\n|.)*?-})|([\w\])}\"][\t ]*)\n[\t ]+((?!.*\s->\s)(?!.*=)(?!.*\bin\b)[\w[({\"]))

I tried to recognize these lines using lookAhead parser but failed.
If we ignored lines containing -> and = it should be fine.

@tunguski
Copy link
Author

Hi All,
during my tests of this library I've found whole list of additional parsing errors. I've created fork of the library which you may find here:

https://github.com/tunguski/elm-ast

I've worked basing on test page parsing all core and http modules. I've got to the point where all these modules are parsed correctly. You may check results here:

https://tunguski.github.io/elm-ast/example/

I assume that most of the issues created in the fork are still open for this project:

https://github.com/tunguski/elm-ast/issues?utf8=%E2%9C%93&q=is%3Aissue%20

Still fork is drastically changed because I have merged Statements and Expressions modules. That was required to fix parsing of Let bindings if I remember correctly. Unfortunately on that stage I did not create issues for all problems found.

Hope this comment will help a bit.

Thanks for all your work!

@joonazan
Copy link
Contributor

@tunguski Making comments parse as whitespace is ingenious!

This repository has some advantages over yours, but it lacks in functionality. By incorporating your solutions to some issues we will arrive at a great result. Keep up the good work!

@wende
Copy link
Collaborator

wende commented May 24, 2017

@tunguski Great work.

I went through your issues and here is the outline of them in this repository:
tunguski#1 - fixed
tunguski#2 - fixed
tunguski#3 - still relevant
tunguski#4 - still relevant
tunguski#5 - fixed
tunguski#6 - fixed
tunguski#7 - fixed
tunguski#8 - fixed
tunguski#9 - fixed
tunguski#10 - fixed
tunguski#11 - not sure
tunguski#12 - fixed
tunguski#13 - fixed
tunguski#14 - still relevant
tunguski#15 - fixed
tunguski#16 - fixed
tunguski#17 - fixed

@wende
Copy link
Collaborator

wende commented May 24, 2017

Plus I noticed that your version also doesn't parse characters with escaped sequences like '\n'

@tunguski
Copy link
Author

tunguski commented May 25, 2017

@wende You're right! I've checked what is actual parsing status using this library and it is 19/27 files parsed correctly (tested on core and http). That's a fundamental change! I would be really happy to switch back to this library when it will be able to parse all core libraries. I'll keep my fingers crossed!

Edit (removed list of code examples): I've created two issues that seem to be only problems (beside proper comments parsing).

supermario pushed a commit to supermario/elm-ast that referenced this issue Jul 8, 2017
wende pushed a commit that referenced this issue Sep 27, 2017
Attempt to complete location information for expressions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants