-
-
Notifications
You must be signed in to change notification settings - Fork 801
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
Friendly error when using multiple spreads #4265
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- | ||
source: compiler-core/src/parse/tests.rs | ||
expression: "\npub fn main() -> Nil {\n let xs = [1, 2, 3]\n let ys = [5, 6, 7]\n [..xs, 4, ..ys]\n}\n" | ||
--- | ||
----- SOURCE CODE | ||
|
||
pub fn main() -> Nil { | ||
let xs = [1, 2, 3] | ||
let ys = [5, 6, 7] | ||
[..xs, 4, ..ys] | ||
} | ||
|
||
|
||
----- ERROR | ||
error: Syntax error | ||
┌─ /src/parse/error.gleam:5:13 | ||
│ | ||
5 │ [..xs, 4, ..ys] | ||
│ ^^ I wasn't expecting a spread here | ||
|
||
Comment on lines
+16
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit confusing to me, the spread there is technically correct, the problem is there's a prior spread that shouldn't have been there. Maybe it could be pointing to the one that is not last and highlight how there can only be one at the end of the list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the error at anything that follows the first spread would be fine and possibly easier to implement. The error can say that the spread must be the last thing in the list. |
||
Hint: use list.append(list1, list2) to join two lists. | ||
See: https://hexdocs.pm/gleam_stdlib/gleam/list.html#append |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -363,6 +363,28 @@ fn pointless_spread() { | |
); | ||
} | ||
|
||
#[test] | ||
fn double_spread() { | ||
assert_error!( | ||
"let xs = [1] let ys = [2, 3] [..xs, ..ys]", | ||
ParseError { | ||
error: ParseErrorType::ListSpreadWithAnotherSpread, | ||
location: SrcSpan { start: 36, end: 38 }, | ||
} | ||
); | ||
} | ||
|
||
#[test] | ||
fn double_spread2() { | ||
assert_error!( | ||
"let xs = [1] let ys = [3, 4] [..xs, 2, ..ys]", | ||
ParseError { | ||
error: ParseErrorType::ListSpreadWithAnotherSpread, | ||
location: SrcSpan { start: 39, end: 41 }, | ||
} | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you remove the two unit tests please 🙏 |
||
|
||
// https://github.com/gleam-lang/gleam/issues/1358 | ||
#[test] | ||
fn lowcase_bool_in_pattern() { | ||
|
@@ -788,6 +810,19 @@ pub fn main() -> Nil { | |
); | ||
} | ||
|
||
#[test] | ||
fn list_spread_followed_by_another_spread() { | ||
assert_module_error!( | ||
r#" | ||
pub fn main() -> Nil { | ||
let xs = [1, 2, 3] | ||
let ys = [5, 6, 7] | ||
[..xs, 4, ..ys] | ||
} | ||
"# | ||
); | ||
} | ||
|
||
#[test] | ||
fn list_spread_as_first_item_followed_by_other_items() { | ||
assert_module_error!( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want to recommend this, it's normally the wrong thing to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I guess it is likely that the problem is modeled poorly. Perhaps we can add that as a comment, or suggest a common "pattern" to model such problems better.
On the other hand, you could argue that it is still helpful for newcomers. You have to type
[..xs, ..ys]
to get the recommendation, so I think it is pretty clearly what you are trying to do.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't want to people to append lists, so helping them to do it would be counterproductive.