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

Friendly error when using multiple spreads #4265

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
9 changes: 9 additions & 0 deletions compiler-core/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,15 @@ where
elements_after_tail = Some(elements);
}
};

// See if there is another spread
// like `[..wibble, wobble, ..wabble]`
if let Some((start, end)) = self.maybe_one(&Token::DotDot) {
return parse_error(
ParseErrorType::ListSpreadWithAnotherSpread,
SrcSpan { start, end },
);
}
};

if tail.is_some() && !elements_end_with_comma {
Expand Down
8 changes: 8 additions & 0 deletions compiler-core/src/parse/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ utf16_codepoint, utf32_codepoint, signed, unsigned, big, little, native, size, u
"See: https://tour.gleam.run/basics/lists/".into(),
],
),
ParseErrorType::ListSpreadWithAnotherSpread => (
"I wasn't expecting a spread here",
vec![
"Hint: use list.append(list1, list2) to join two lists.".into(),
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

@lpil lpil Feb 22, 2025

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.

"See: https://hexdocs.pm/gleam_stdlib/gleam/list.html#append".into(),
],
),
ParseErrorType::ListSpreadFollowedByElements => (
"I wasn't expecting elements after this",
vec![
Expand Down Expand Up @@ -375,6 +382,7 @@ pub enum ParseErrorType {
UnknownTarget, // an unknown target was used
ListSpreadWithoutElements, // Pointless spread: `[..xs]`
ListSpreadFollowedByElements, // trying to append something after the spread: `[..xs, x]`
ListSpreadWithAnotherSpread, // trying to use multiple spreads: `[..xs, ..ys]`
LowcaseBooleanPattern, // most likely user meant True or False in patterns
UnexpectedLabel, // argument labels were provided, but are not supported in this context
UnexpectedEof,
Expand Down
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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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
35 changes: 35 additions & 0 deletions compiler-core/src/parse/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
}
);
}
Copy link
Member

Choose a reason for hiding this comment

The 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() {
Expand Down Expand Up @@ -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!(
Expand Down