Skip to content
This repository was archived by the owner on Oct 5, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ Oh God please yes! :heart: Feel free to look around the [<kbd>help wanted</kbd>]
| ----------------- | -------------------- | -------------------- | ------------------ | ------------------ | -------------------- | ------------------ | ------------------ |
| integers | :heavy_check_mark: | :warning: [[1]](#f1) | :heavy_check_mark: | :heavy_check_mark: | :warning: [[2]](#f2) | :heavy_check_mark: | :heavy_check_mark: |
| floats | :x: [[3]](#f3) | :x: [[4]](#f4) | :x: [[3]](#f3) | :x: [[3]](#f3) | :x: [[5]](#f5) | :x: [[3]](#f3) | :x: [[3]](#f3) |
| characters | :warning: [[6]](#f6) | :warning: [[7]](#f7) | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: |
| strings | :warning: [[8]](#f8) | :warning: [[9]](#f9) | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: |
| characters | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: |
| strings | :warning: [[6]](#f6) | :warning: [[7]](#f7) | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: |
| booleans | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: |
| variables | :warning: | :heavy_check_mark: | :heavy_check_mark: | :heavy_check_mark: | :warning: | :heavy_check_mark: | :heavy_check_mark: |
| lists | :x: | :x: | :x: | :x: | :x: | :x: | :x: |
Expand All @@ -99,10 +99,8 @@ Oh God please yes! :heart: Feel free to look around the [<kbd>help wanted</kbd>]
3. <span id="f3"></span> Not implemented; tracked in [#17](https://github.com/elm-in-elm/compiler/issues/17)
4. <span id="f4"></span> Not implemented; not tracked yet
5. <span id="f5"></span> To be optimized the same way Ints are; not tracked yet
6. <span id="f6"></span> Comprehensive tests missing; will be fixed in [#15](https://github.com/elm-in-elm/compiler/pull/15)
7. <span id="f7"></span> Escape sequences not implemented; not tracked yet
8. <span id="f8"></span> Comprehensive tests missing; not tracked yet
9. <span id="f9"></span> Multiline strings (and maybe more) missing; not tracked yet
6. <span id="f6"></span> Comprehensive tests missing; not tracked yet
7. <span id="f7"></span> Multiline strings (and maybe more) missing; not tracked yet
Copy link
Contributor

Choose a reason for hiding this comment

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

Next time please don't bother shifting the numbers, life's too short for that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phew 👍


## Prerequisites

Expand Down Expand Up @@ -228,6 +226,12 @@ Runs `elm-test` on the test suite (gasp!)
</br>
<a href="https://github.com/Warry">Maxime Dantec</a>
</td>
<td align="center">
<img width="150" height="150"
src="https://avatars0.githubusercontent.com/u/16829510">
</br>
<a href="https://github.com/aaronjanse">Aaron Janse</a>
</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

</tr>
<tbody>
</table>
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/Error.elm
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ type ParseProblem
| ExpectingHexadecimals
| ExpectingSingleQuote
| ExpectingChar
| ExpectingEscapeBackslash
| ExpectingEscapeCharacter
| ExpectingUnicodeEscapeLeftBrace
| ExpectingUnicodeEscapeRightBrace
| InvalidUnicodeCodePoint
| ExpectingDoubleQuote
| ExpectingPlusOperator
| ExpectingModuleDot -- `import Foo>.<Bar`
Expand Down
73 changes: 62 additions & 11 deletions src/compiler/Stage/Parse/Parser.elm
Original file line number Diff line number Diff line change
Expand Up @@ -410,23 +410,74 @@ literalInt =
|> P.inContext InLiteralInt


{-| TODO escapes
TODO Unicode escapes
-}
-- for literalChar and, in the future, literalString
stringHelp =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably rather name this character or insideQuotes or something. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

character is great because it parses one character, even if that character is represented by multiple bytes (e.g. \u{123}) 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 483bf8a

P.oneOf
[ P.succeed (identity)
Copy link
Contributor

Choose a reason for hiding this comment

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

probably unneeded parens for the identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 657fa28

|. P.token (P.Token "\\" ExpectingEscapeBackslash)
|= P.oneOf
[ P.map (\_ -> '\"') (P.token (P.Token "\"" ExpectingEscapeCharacter))
, P.map (\_ -> '\'') (P.token (P.Token "'" ExpectingEscapeCharacter))
, P.map (\_ -> '\n') (P.token (P.Token "n" ExpectingEscapeCharacter))
, P.map (\_ -> '\t') (P.token (P.Token "t" ExpectingEscapeCharacter))
, P.map (\_ -> '\r') (P.token (P.Token "r" ExpectingEscapeCharacter))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still thinking about these ParserProblems. The problem with this current solution is that the error messages will only be able to show five times the same thing ("I expected escape character, escape character, ...")

Please parameterize this one (| ExpectingEscapeCharacter Char) and give these usages of it the chars they're really about. That should be enough and not bloat the ParserProblem type so much. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about something like below?

The above behavior would interpret \x as a backslash followed by a separate x.
The below would throw an error if it sees \x.

 |= P.oneOf
-    [ P.map (\_ -> '\"') (P.token (P.Token "\"" ExpectingEscapeCharacter))
-    , P.map (\_ -> '\'') (P.token (P.Token "'"  ExpectingEscapeCharacter))
-    , P.map (\_ -> '\n') (P.token (P.Token "n"  ExpectingEscapeCharacter))
-    , P.map (\_ -> '\t') (P.token (P.Token "t"  ExpectingEscapeCharacter))
-    , P.map (\_ -> '\r') (P.token (P.Token "r"  ExpectingEscapeCharacter))
+    [ P.map (\_ -> '\"') (Parser.token "\""))
+    , P.map (\_ -> '\'') (Parser.token "'"))
+    , P.map (\_ -> '\n') (Parser.token "n"))
+    , P.map (\_ -> '\t') (Parser.token "t"))
+    , P.map (\_ -> '\r') (Parser.token "r"))
     , P.succeed identity
         |. P.token (P.Token "u" ExpectingEscapeCharacter)
         |. P.token (P.Token "{" ExpectingUnicodeEscapeLeftBrace)
         |= unicode
         |. P.token (P.Token "}" ExpectingUnicodeEscapeRightBrace)
+    , P.problem ExpectingValidEscapeChar
     ]

Please parameterize this one (| ExpectingEscapeCharacter Char) and give these usages of it the chars they're really about. That should be enough and not bloat the ParserProblem type so much. WDYT?

The problem is that I don't think in either solutions, both the current one in the PR and the diff above, the compiler would print out one of those token errors. Rather, it would either skip the oneOf (current PR code) or print ExpectingValidEscapeChar (diff above).

Copy link
Contributor

Choose a reason for hiding this comment

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

current PR state: https://ellie-app.com/5Z9tkd2hGSQa1
my suggestion: https://ellie-app.com/5Z9vStSJv72a1
The changes are on lines 31-37, and 77
I think that's enough! The ParserProblems will tell you what characters did it expect ➡️ we can write a nice error message.

If you think this still overlooks some corner case, please shout! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad!
Fixed by 7c495f7

, P.succeed identity
|. P.token (P.Token "u" ExpectingEscapeCharacter)
|. P.token (P.Token "{" ExpectingUnicodeEscapeLeftBrace)
|= unicode
|. P.token (P.Token "}" ExpectingUnicodeEscapeRightBrace)
]
, P.succeed identity
|= P.getChompedString (P.chompIf (always True) ExpectingChar)
|> P.andThen (\string ->
string
|> String.uncons
|> Maybe.map (Tuple.first >> P.succeed)
|> Maybe.withDefault (P.problem (CompilerBug "Multiple characters chomped in `literalChar`"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is not entirely correct, we're not inside literalChar. If we want to reuse this in Strings too. it would mislead the user seeing the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 483bf8a

)
]


literalChar : Parser_ Literal
literalChar =
(P.succeed identity
|. P.symbol (P.Token "'" ExpectingSingleQuote)
|= P.getChompedString (P.chompIf (always True) ExpectingChar)
|= stringHelp
|. P.symbol (P.Token "'" ExpectingSingleQuote)
)
|> P.andThen
(\string ->
string
|> String.uncons
|> Maybe.map (Tuple.first >> Char >> P.succeed)
|> Maybe.withDefault (P.problem (CompilerBug "Multiple characters chomped in `literalChar`"))
)
|> P.map (\n -> Char n)
Copy link
Contributor

Choose a reason for hiding this comment

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

|> P.map Char

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by b7a3cf4

Thank you, @Janiczek!


unicode : Parser_ Char
unicode =
P.getChompedString (P.chompWhile Char.isHexDigit)
|> P.andThen codeToChar


codeToChar : String -> Parser_ Char
codeToChar str =
let
length = String.length str
code = String.foldl addHex 0 str
Copy link
Contributor

Choose a reason for hiding this comment

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

let's check for the length, that's great, but let's use Hex.fromString for the actual "is this a hex string" and "what int value does it represent" functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 09fa46c

in
if length < 4 || length > 6 then
P.problem InvalidUnicodeCodePoint
else if 0 <= code && code <= 0x10FFFF then
P.succeed (Char.fromCode code)
else
P.problem InvalidUnicodeCodePoint


addHex : Char -> Int -> Int
addHex char total =
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use Hex.fromString instead of this. Also, it is buggy wrt. characters outside the hex range I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by 09fa46c

let
code = Char.toCode char
in
if 0x30 <= code && code <= 0x39 then
16 * total + (code - 0x30)
else if 0x41 <= code && code <= 0x46 then
16 * total + (10 + code - 0x41)
else
16 * total + (10 + code - 0x61)


{-| TODO escapes
Expand Down
30 changes: 30 additions & 0 deletions tests/ParserTest.elm
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,36 @@ expr =
, "'A'"
, Ok (Literal (Char 'A'))
)
-- https://github.com/elm/compiler/blob/dcbe51fa22879f83b5d94642e117440cb5249bb1/compiler/src/Parse/String.hs#L279-L285
, ( "escape n"
, "'\\n'"
, Ok (Literal (Char '\n'))
)
, ( "escape r"
, "'\\r'"
, Ok (Literal (Char '\r'))
)
, ( "escape t"
, "'\\t'"
, Ok (Literal (Char '\t'))
)
, ( "double quote"
, "'\\\"'"
, Ok (Literal (Char '"')) -- "
) -- ^ workaround for official elm
-- vscode syntax highlighter
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want, I'm happy to remove it.
Without it, however, the coloring is wrong for the next ~17 lines and a "parser error" prevents vscode from showing more helpful errors :-/

https://github.com/Krzysztof-Cieslak/vscode-elm/issues/244

, ( "single quote"
, "'\\\''"
, Ok (Literal (Char '\''))
)
, ( "emoji"
, "'😃'"
, Ok (Literal (Char '😃'))
)
, ( "escaped unicode code point"
, "'\\u{1F648}'"
, Ok (Literal (Char '🙈'))
)
]
)
, ( "literal string"
Expand Down