Add Information About Found Token in UnexpectedToken Error Type #3299

rdghosal · 2024-06-20T10:59:57Z

Addresses #3286.

This commit introduces:

A found member to ParserErrorType::UnexpectedToken of type struct UnexpectedTokenInfo
An impl of std::fmt::Display in UnexpectedTokenInfo that specifies whether the unexpected token is a reserved word (i.e., keyword).

This commit changes:

The details of ParserErrorType::UnexpectedToken to read "I was not expecting this. Found {.found}, expected..."
In the case of a keyword, details of ParserErrorType::UnexpectedToken read as "I was not expecting this. Found the keyword {.found}, expected..." (see updated snapshots).

Would appreciate any and all feedback.
Thanks!

lpil

Lovely! Thank you! I've left some small notes inline

lpil · 2024-06-20T11:10:34Z

compiler-core/src/parse.rs

+    else {
+        format!("{base}")
+    }
+}


This is error presentation logic so it needs to live in the warning printer, and the data in the warning needs to be designed data structures rather than strings. Perhaps it could contain the token itself.

@lpil Updated to include an UnexpectedTokenInfo that handles this. Thanks for the feedback. ~~This makes the original PR desc outdated... lmk if I should update this too.~~ (Updated the description of the PR accordingly too).

lpil · 2024-06-20T11:10:50Z

...re/src/parse/snapshots/gleam_core__parse__tests__assignment_pattern_invalid_bit_segment.snap

@@ -8,6 +8,6 @@ error: Syntax error
 3 │     let <<b1, pub>> = <<24, 3>>
  │               ^^^ I was not expecting this

-Expected one of: 
+Found "pub" (a keyword), expected one of: 


Suggested change

Found "pub" (a keyword), expected one of:

Found the keyword `pub`, expected one of:

@lpil Thanks for the suggestion. Backticks are definitely better on the eyes here.

Should this be the case for non-keywords as well, or shall we keep double quotes? I ask because currently the expected token that gets printed is also wrapped in double quotes.

@lpil Pushed some updates per your feedback and kept the behavior of non-keywords the same. Lmk if that works... thanks!

lpil

Very nice! Could we have a test for an unexpected string and an unexpected int too please 🙏

lpil · 2024-06-25T15:08:54Z

compiler-core/src/parse/error.rs

+            write!(f, "\"{}\"", base)
+        }
+    }
+}


Do not implement this using traits please. We avoid using traits, and this specifically is a misuse of Display as this isn't a general purpose to-string impl, it's specific to that one function.

lpil · 2024-06-25T15:11:22Z

compiler-core/src/parse/snapshots/gleam_core__parse__tests__capture_with_name.snap

@@ -8,6 +8,6 @@ error: Syntax error
 3 │   add(_name, 1)
  │       ^^^^^ I was not expecting this

-Expected one of: 
+Found "_name", expected one of: 


Let's say "a name". They can already see what the name is 2 lines above

lpil · 2024-06-25T15:12:42Z

compiler-core/src/parse.rs

            // there are still more tokens
            let expected = vec!["An import, const, type, or function.".into()];
            return parse_error(
                ParseErrorType::UnexpectedToken {
+                    found: UnexpectedTokenInfo {
+                        display: tok.to_string(),


Use an enum rather than a string here. It could be the token. I would probably put the entire token as found and not have this struct. The is_reserved_word can be moved to be a method on Token

rdghosal · 2024-06-26T01:06:59Z

@lpil Thanks for all the detailed feedback! Pushed some updates per your comments... Do let me know if anything else appears off

lpil · 2024-06-26T11:35:02Z

compiler-core/src/parse.rs

@@ -3606,7 +3525,9 @@ pub fn make_call(
                if name != "_" {
                    return parse_error(
                        ParseErrorType::UnexpectedToken {
+                            found: ParserErrorCause::Descriptor("a name".into()),


We can get rid of the ParserErrorCause enum by making a token here!

Suggested change

found: ParserErrorCause::Descriptor("a name".into()),

found: Token::Name(name),

Oh, man! This was so obvious... thanks for pointing that out!

lpil

Looking great!!

lpil · 2024-06-26T11:37:27Z

...iler-core/src/parse/snapshots/gleam_core__parse__tests__const_invalid_bit_array_segment.snap

@@ -8,6 +8,6 @@ error: Syntax error
 2 │ const a = <<1, 2, <->>
  │                   ^^ I was not expecting this

-Expected one of: 
+Found "<-", expected one of: 


Where do these double quotes come from? I couldn't find them in the codebase 😅

Could you replace them all with backticks please, to match the keywords.

lpil · 2024-06-26T11:38:15Z

compiler-core/src/parse/snapshots/gleam_core__parse__tests__function_invalid_signature.snap

+2 │ fn f(a, "b") -> String {
+  │         ^^^ I was not expecting this
+
+Found "b", expected one of: 


For any token with content can you say "a string", "a float", "an int" etc rather than display the content a second time in the error. Thank you

rdghosal · 2024-06-26T12:29:02Z

@lpil Pushed up a few more updates per your feedback. Thank you!

lpil

Beautiful work, thank you. Would you mind updating the changelog file also?

lpil

Beautiful work, thank you. Would you mind updating the changelog file also?

lpil · 2024-06-30T11:02:54Z

We don't permit merge commits in this repo. Would you like to rebase to remove the ones from this branch, or should I squash-merge this PR?

…hether tok is keyword

…ails with backticks

…saging for reserved words

…ding token variants and replace ParseErrorCause enum with Token

…hether tok is keyword

…ails with backticks

…saging for reserved words

…ding token variants and replace ParseErrorCause enum with Token

lpil

Wonderful!! Thank you very much!!

lpil · 2024-06-30T13:49:39Z

I think something may have gone a bit wrong with the rebase, there's multiple copies of each commit in the history, and there's conflicts preventing a rebase into main. I'll squash merge for this one. Thank you

lpil reviewed Jun 20, 2024

View reviewed changes

lpil reviewed Jun 25, 2024

View reviewed changes

lpil reviewed Jun 26, 2024

View reviewed changes

lpil reviewed Jun 30, 2024

View reviewed changes

lpil approved these changes Jun 30, 2024

View reviewed changes

Rahul Ghosal added 11 commits June 30, 2024 09:11

feat: add to UnexpectedToken and include in details with info about w…

1d63d74

…hether tok is keyword

feat: move formatting logic out of parser and display keywords in det…

5bfc19b

…ails with backticks

chore: apply cargo fmt

0eb33f9

refac: capture as an enum rather than a struct

4864247

feat: move is_reserved_word to Token, re-apply the specific error mes…

ab72271

…saging for reserved words

test: add tests for unexpected String and unexpected Int

bb84827

style: cargo fmt

570aec3

feat: use backticks to surround all tokens instead of dquotes in Display

c40ca3d

feat: display token type instead of repeating content for content-hol…

8a7cf13

…ding token variants and replace ParseErrorCause enum with Token

chore: remove to_string on Token to resolve clippy warning

602447c

refac: change field name in UnexpectedTokenError from found to token

824a324

rdghosal force-pushed the unclear-unexpected-tok-details branch from 8a55084 to 824a324 Compare June 30, 2024 13:17

Rahul Ghosal added 9 commits June 30, 2024 09:26

chore: update changelog

d60c21d

feat: add to UnexpectedToken and include in details with info about w…

ba9df14

…hether tok is keyword

feat: move formatting logic out of parser and display keywords in det…

cd7f308

…ails with backticks

chore: apply cargo fmt

fc7bb42

refac: capture as an enum rather than a struct

aab3cc3

feat: move is_reserved_word to Token, re-apply the specific error mes…

600061b

…saging for reserved words

test: add tests for unexpected String and unexpected Int

c892dc0

style: cargo fmt

176437e

feat: use backticks to surround all tokens instead of dquotes in Display

624b560

Rahul Ghosal added 4 commits June 30, 2024 09:32

feat: display token type instead of repeating content for content-hol…

58be22c

…ding token variants and replace ParseErrorCause enum with Token

chore: remove to_string on Token to resolve clippy warning

de83dce

refac: change field name in UnexpectedTokenError from found to token

db009f2

chore: update changelog

6c46b3c

lpil approved these changes Jun 30, 2024

View reviewed changes

lpil merged commit a2b12fa into gleam-lang:main Jun 30, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Information About Found Token in UnexpectedToken Error Type #3299

Add Information About Found Token in UnexpectedToken Error Type #3299

rdghosal commented Jun 20, 2024 •

edited

Loading

lpil left a comment

lpil Jun 20, 2024

rdghosal Jun 20, 2024 •

edited

Loading

lpil Jun 20, 2024

rdghosal Jun 20, 2024

rdghosal Jun 20, 2024

lpil left a comment

lpil Jun 25, 2024

lpil Jun 25, 2024

lpil Jun 25, 2024

rdghosal commented Jun 26, 2024

lpil Jun 26, 2024

rdghosal Jun 26, 2024

lpil left a comment

lpil Jun 26, 2024

lpil Jun 26, 2024

rdghosal commented Jun 26, 2024

lpil left a comment

lpil left a comment

lpil commented Jun 30, 2024

lpil left a comment

lpil commented Jun 30, 2024

	Found "pub" (a keyword), expected one of:
	Found the keyword `pub`, expected one of:

	found: ParserErrorCause::Descriptor("a name".into()),
	found: Token::Name(name),

Add Information About Found Token in UnexpectedToken Error Type #3299

Add Information About Found Token in UnexpectedToken Error Type #3299

Conversation

rdghosal commented Jun 20, 2024 • edited Loading

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdghosal Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdghosal commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdghosal commented Jun 26, 2024

lpil left a comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

lpil commented Jun 30, 2024

lpil left a comment

Choose a reason for hiding this comment

lpil commented Jun 30, 2024

rdghosal commented Jun 20, 2024 •

edited

Loading

rdghosal Jun 20, 2024 •

edited

Loading