diff --git a/compiler-core/src/parse.rs b/compiler-core/src/parse.rs index 51541dc0a..5998ab115 100644 --- a/compiler-core/src/parse.rs +++ b/compiler-core/src/parse.rs @@ -2567,90 +2567,65 @@ where let documentation = self.take_documentation(start); let (name_start, name, parameters, end, name_end) = self.expect_type_name()?; let name_location = SrcSpan::new(name_start, name_end); - let (constructors, end_position) = if self.maybe_one(&Token::LeftBrace).is_some() { - // Custom Type - let constructors = Parser::series_of( - self, - &|p| { - // The only attribute supported on constructors is @deprecated - let mut attributes = Attributes::default(); - let attr_loc = Parser::parse_attributes(p, &mut attributes)?; - - if let Some(attr_span) = attr_loc { - // Expecting all but the deprecated atterbutes to be default - if attributes.external_erlang.is_some() - || attributes.external_javascript.is_some() - || attributes.target.is_some() - || attributes.internal != InternalAttribute::Missing - { - return parse_error( - ParseErrorType::UnknownAttributeRecordVariant, - attr_span, - ); - } - } - match Parser::maybe_upname(p) { - Some((c_s, c_n, c_e)) => { - let documentation = p.take_documentation(c_s); - let (arguments, arguments_e) = - Parser::parse_type_constructor_arguments(p)?; - let end = arguments_e.max(c_e); - Ok(Some(RecordConstructor { - location: SrcSpan { start: c_s, end }, - name_location: SrcSpan { - start: c_s, - end: c_e, - }, - name: c_n, - arguments, - documentation, - deprecation: attributes.deprecated, - })) - } - _ => Ok(None), - } - }, - // No separator - None, - )?; - let (_, close_end) = self.expect_custom_type_close(&name, public, opaque)?; - (constructors, close_end) - } else { - match self.maybe_one(&Token::Equal) { - Some((eq_s, eq_e)) => { - // Type Alias - if opaque { - return parse_error( - ParseErrorType::OpaqueTypeAlias, - SrcSpan { start, end }, - ); - } + let (constructors, end_position) = match self.tok0.take() { + // If we see `type Wibble {`, then we know we're parsing a custom type. + Some((_, Token::LeftBrace, _)) => { + self.advance(); - match self.parse_type()? { - Some(t) => { - let type_end = t.location().end; - return Ok(Some(Definition::TypeAlias(TypeAlias { - documentation, - location: SrcSpan::new(start, type_end), - publicity: self.publicity(public, attributes.internal)?, - alias: name, - name_location, - parameters, - type_ast: t, - type_: (), - deprecation: std::mem::take(&mut attributes.deprecated), - }))); - } - _ => { - return parse_error( - ParseErrorType::ExpectedType, - SrcSpan::new(eq_s, eq_e), - ); - } - } + // If we see a lowercase name, rather than an uppercase one. We + // know there's a syntax error! So now we can try and provide a + // nice error message, based on what that wrong code looks like. + if let Some((name_start, Token::Name { .. }, name_end)) = &self.tok0 { + return Err(self.invalid_record_constructor_error( + name, + public, + opaque, + *name_start, + *name_end, + )); + } + + let constructors = self.series_of( + &|parser| parser.parse_record_constructor(), + // No separator + None, + )?; + let close_end = self.expect_custom_type_close()?; + (constructors, close_end) + } + + // If we see `type Wibble =` then we know we're parsing a type alias. + Some((equal_start, Token::Equal, equal_end)) => { + self.advance(); + + if opaque { + return parse_error(ParseErrorType::OpaqueTypeAlias, SrcSpan { start, end }); } - _ => (vec![], end), + + if let Some(type_) = self.parse_type()? { + return Ok(Some(Definition::TypeAlias(TypeAlias { + documentation, + location: SrcSpan::new(start, type_.location().end), + publicity: self.publicity(public, attributes.internal)?, + alias: name, + name_location, + parameters, + type_ast: type_, + type_: (), + deprecation: std::mem::take(&mut attributes.deprecated), + }))); + } else { + return parse_error( + ParseErrorType::ExpectedType, + SrcSpan::new(equal_start, equal_end), + ); + } + } + + token @ (Some(_) | None) => { + self.tok0 = token; + (vec![], end) } }; @@ -2671,6 +2646,189 @@ where }))) } + fn parse_record_constructor(&mut self) -> Result>, ParseError> { + // The only attribute supported on constructors is @deprecated + let mut attributes = Attributes::default(); + let attr_loc = self.parse_attributes(&mut attributes)?; + + if let Some(attr_span) = attr_loc { + // Expecting all but the deprecated atterbutes to be default + if attributes.external_erlang.is_some() + || attributes.external_javascript.is_some() + || attributes.target.is_some() + || attributes.internal != InternalAttribute::Missing + { + return parse_error(ParseErrorType::UnknownAttributeRecordVariant, attr_span); + } + } + + match self.maybe_upname() { + Some((name_start, constructor_name, name_end)) => { + let documentation = self.take_documentation(name_start); + let (arguments, arguments_end) = self.parse_record_constructor_arguments()?; + + Ok(Some(RecordConstructor { + location: SrcSpan { + start: name_start, + end: arguments_end.max(name_end), + }, + name_location: SrcSpan { + start: name_start, + end: name_end, + }, + name: constructor_name, + arguments, + documentation, + deprecation: attributes.deprecated, + })) + } + _ => Ok(None), + } + } + + /// This takes place when we find a lowercase name as a record constructor + /// variant (that name is passed as an argument here). + /// We want to look at the following tokens to produce a nice error message: + /// + /// ```gleam + /// pub type Wibble { + /// wibble + /// //^^^^^^ Error, this should be uppercase! + /// } + /// ``` + /// + /// But if the thing looks like a record definition, we want a specialised + /// error message: + /// + /// ```gleam + /// pub type Wibble { + /// wibble: Int, + /// wobble: String + /// } + /// // Suggest wrapping this in a constructor. + /// ``` + /// + fn invalid_record_constructor_error( + &mut self, + type_name: EcoString, + public: bool, + opaque: bool, + name_start: u32, + name_end: u32, + ) -> ParseError { + let fields = self.series_of( + &|parser| parser.parse_record_constructor_field(), + Some(&Token::Comma), + ); + + match fields { + // If there's a list of fields right inside the type that means the + // developer might have forgotten to wrap the thing in a constructor. + // Basically writing something like this: + // + // ```gleam + // pub type Wibble { + // String, + // wibble: Int, + // } + // ``` + // + // So we want to produce a specialised error message pointing them + // in the right direction. + Ok(fields) if let Some((_, Token::RightBrace, _)) = self.tok0 => ParseError { + location: SrcSpan { + start: fields + .first() + .map_or(name_start, |field| field.location.start), + end: fields.last().map_or(name_end, |field| field.location.end), + }, + error: ParseErrorType::ExpectedRecordConstructor { + type_name: type_name.clone(), + public, + opaque, + fields, + }, + }, + + // Otherwise we fall back to telling them the lowercase name should + // be uppercased! + Ok(_) | Err(_) => ParseError { + error: ParseErrorType::IncorrectUpName, + location: SrcSpan { + start: name_start, + end: name_end, + }, + }, + } + } + + // examples: + // *no args* + // () + // (a, b) + fn parse_record_constructor_arguments( + &mut self, + ) -> Result<(Vec>, u32), ParseError> { + if self.maybe_one(&Token::LeftParen).is_some() { + let arguments = Parser::series_of( + self, + &|parser| parser.parse_record_constructor_field(), + Some(&Token::Comma), + )?; + let (_, end) = self + .expect_one_following_series(&Token::RightParen, "a constructor argument name")?; + Ok((arguments, end)) + } else { + Ok((vec![], 0)) + } + } + + fn parse_record_constructor_field( + &mut self, + ) -> Result>, ParseError> { + match (self.tok0.take(), self.tok1.take()) { + (Some((start, Token::Name { name }, name_end)), Some((_, Token::Colon, end))) => { + let _ = Parser::next_tok(self); + let _ = Parser::next_tok(self); + let doc = self.take_documentation(start); + match Parser::parse_type(self)? { + Some(type_ast) => { + let end = type_ast.location().end; + Ok(Some(RecordConstructorArg { + label: Some((SrcSpan::new(start, name_end), name)), + ast: type_ast, + location: SrcSpan { start, end }, + type_: (), + doc, + })) + } + None => parse_error(ParseErrorType::ExpectedType, SrcSpan { start, end }), + } + } + (t0, t1) => { + self.tok0 = t0; + self.tok1 = t1; + match Parser::parse_type(self)? { + Some(type_ast) => { + let doc = match &self.tok0 { + Some((start, _, _)) => self.take_documentation(*start), + None => None, + }; + let type_location = type_ast.location(); + Ok(Some(RecordConstructorArg { + label: None, + ast: type_ast, + location: type_location, + type_: (), + doc, + })) + } + None => Ok(None), + } + } + } + } + // examples: // A // A(one, two) @@ -2727,72 +2885,6 @@ where } } - // examples: - // *no args* - // () - // (a, b) - fn parse_type_constructor_arguments( - &mut self, - ) -> Result<(Vec>, u32), ParseError> { - if self.maybe_one(&Token::LeftParen).is_some() { - let arguments = Parser::series_of( - self, - &|p| match (p.tok0.take(), p.tok1.take()) { - ( - Some((start, Token::Name { name }, name_end)), - Some((_, Token::Colon, end)), - ) => { - let _ = Parser::next_tok(p); - let _ = Parser::next_tok(p); - let doc = p.take_documentation(start); - match Parser::parse_type(p)? { - Some(type_ast) => { - let end = type_ast.location().end; - Ok(Some(RecordConstructorArg { - label: Some((SrcSpan::new(start, name_end), name)), - ast: type_ast, - location: SrcSpan { start, end }, - type_: (), - doc, - })) - } - None => { - parse_error(ParseErrorType::ExpectedType, SrcSpan { start, end }) - } - } - } - (t0, t1) => { - p.tok0 = t0; - p.tok1 = t1; - match Parser::parse_type(p)? { - Some(type_ast) => { - let doc = match &p.tok0 { - Some((start, _, _)) => p.take_documentation(*start), - None => None, - }; - let type_location = type_ast.location(); - Ok(Some(RecordConstructorArg { - label: None, - ast: type_ast, - location: type_location, - type_: (), - doc, - })) - } - None => Ok(None), - } - } - }, - Some(&Token::Comma), - )?; - let (_, end) = self - .expect_one_following_series(&Token::RightParen, "a constructor argument name")?; - Ok((arguments, end)) - } else { - Ok((vec![], 0)) - } - } - // // Parse Type Annotations // @@ -4046,57 +4138,29 @@ where /// Expect the end to a custom type definiton or handle an incorrect /// record constructor definition. - /// - /// Used for mapping to a more specific error type and message. - fn expect_custom_type_close( - &mut self, - name: &EcoString, - public: bool, - opaque: bool, - ) -> Result<(u32, u32), ParseError> { + fn expect_custom_type_close(&mut self) -> Result { match self.maybe_one(&Token::RightBrace) { - Some((start, end)) => Ok((start, end)), + Some((_, end)) => Ok(end), None => match self.next_tok() { None => parse_error(ParseErrorType::UnexpectedEof, SrcSpan { start: 0, end: 0 }), Some((start, token, end)) => { - // If provided a Name, map to a more detailed error - // message to nudge the user. - // Else, handle as an unexpected token. - let field = if let Token::Name { name } = token { - name - } else { - let hint = match (&token, self.tok0.take()) { - (&Token::Fn, _) | (&Token::Pub, Some((_, Token::Fn, _))) => { - let text = "Gleam is not an object oriented programming language so + let hint = match (&token, self.tok0.take()) { + (&Token::Fn, _) | (&Token::Pub, Some((_, Token::Fn, _))) => { + let text = "Gleam is not an object oriented programming language so functions are declared separately from types."; - Some(wrap(text).into()) - } - (_, _) => None, - }; - - return parse_error( - ParseErrorType::UnexpectedToken { - token, - expected: vec![ - Token::RightBrace.to_string().into(), - "a record constructor".into(), - ], - hint, - }, - SrcSpan { start, end }, - ); - }; - let field_type = match self.parse_type_annotation(&Token::Colon) { - Ok(Some(annotation)) => Some(Box::new(annotation)), - _ => None, + Some(wrap(text).into()) + } + (_, _) => None, }; + parse_error( - ParseErrorType::ExpectedRecordConstructor { - name: name.clone(), - public, - opaque, - field, - field_type, + ParseErrorType::UnexpectedToken { + token, + expected: vec![ + Token::RightBrace.to_string().into(), + "a record constructor".into(), + ], + hint, }, SrcSpan { start, end }, ) diff --git a/compiler-core/src/parse/error.rs b/compiler-core/src/parse/error.rs index df01dda3e..31a664ace 100644 --- a/compiler-core/src/parse/error.rs +++ b/compiler-core/src/parse/error.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: 2020 The Gleam contributors -use crate::ast::{SrcSpan, TypeAst}; +use crate::ast::{RecordConstructorArg, SrcSpan, TypeAst}; use crate::diagnostic::{ExtraLabel, Label}; use crate::error::{wrap, wrap_format}; use crate::parse::Token; @@ -145,12 +145,23 @@ pub enum ParseErrorType { RedundantInternalAttribute, // for a private definition marked as internal InvalidModuleTypePattern, // for patterns that have a dot like: `name.thing` ListPatternSpreadFollowedByElements, // When there is a pattern after a spread [..rest, pattern] + + /// This happens when someone forgets to write the type constructor around + /// its fields in a custom type definition. For example: + /// + /// ```gleam + /// pub type Wibble { + /// String, + /// field: Int, + /// field_2: a, + /// } + /// ``` ExpectedRecordConstructor { - name: EcoString, + type_name: EcoString, public: bool, opaque: bool, - field: EcoString, - field_type: Option>, + /// Those are the fields that have been written. + fields: Vec>, }, CallInClauseGuard, // case x { _ if f() -> 1 } IfExpression, @@ -684,11 +695,10 @@ See: https://tour.gleam.run/flow-control/case-expressions/" }, ParseErrorType::ExpectedRecordConstructor { - name, + type_name, public, opaque, - field, - field_type, + fields, } => { let (accessor, opaque) = match *public { true if *opaque => ("pub ", "opaque "), @@ -696,22 +706,29 @@ See: https://tour.gleam.run/flow-control/case-expressions/" false => ("", ""), }; - let mut annotation = EcoString::new(); - match field_type { - Some(t) => t.print(&mut annotation), - None => annotation.push_str("Type"), - }; + let fields = fields + .iter() + .map(|field| { + let mut type_ = EcoString::new(); + field.ast.print(&mut type_); + + match field.label.as_ref() { + Some((_, label)) => format!(" {label}: {type_},"), + None => format!(" {type_},"), + } + }) + .join("\n"); ParseErrorDetails { - text: [ - "Each custom type variant must have a constructor:\n".into(), - format!("{accessor}{opaque}type {name} {{"), - format!(" {name}("), - format!(" {field}: {annotation},"), - " )".into(), - "}".into(), - ] - .join("\n"), + text: format!( + "Each custom type variant must have a constructor: + +{accessor}{opaque}type {type_name} {{ + {type_name}( +{fields} + ) +}}" + ), hint: None, label_text: "I was not expecting this".into(), extra_labels: vec![], diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__lowercase_constructor_name_in_custom_type.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__lowercase_constructor_name_in_custom_type.snap new file mode 100644 index 000000000..edd6057cd --- /dev/null +++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__lowercase_constructor_name_in_custom_type.snap @@ -0,0 +1,20 @@ +--- +source: compiler-core/src/parse/tests.rs +expression: "\npub type Wibble {\n wibble(Int, String)\n}\n" +--- +----- SOURCE CODE + +pub type Wibble { + wibble(Int, String) +} + + +----- ERROR +error: Syntax error + ┌─ /src/parse/error.gleam:3:3 + │ +3 │ wibble(Int, String) + │ ^^^^^^ I'm expecting a type name here + +Hint: Type names start with a uppercase letter, and can contain a-z, A-Z, +or 0-9. diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__missing_constructor_name_with_multiple_fields.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__missing_constructor_name_with_multiple_fields.snap new file mode 100644 index 000000000..48304dc6a --- /dev/null +++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__missing_constructor_name_with_multiple_fields.snap @@ -0,0 +1,28 @@ +--- +source: compiler-core/src/parse/tests.rs +expression: "\npub type Wibble(a) {\n field: Int,\n other: a\n}\n" +--- +----- SOURCE CODE + +pub type Wibble(a) { + field: Int, + other: a +} + + +----- ERROR +error: Syntax error + ┌─ /src/parse/error.gleam:3:3 + │ +3 │ ╭ field: Int, +4 │ │ other: a + │ ╰──────────^ I was not expecting this + +Each custom type variant must have a constructor: + +pub type Wibble { + Wibble( + field: Int, + other: a, + ) +} diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor.snap index 06207571f..54fa33b94 100644 --- a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor.snap +++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor.snap @@ -14,7 +14,7 @@ error: Syntax error ┌─ /src/parse/error.gleam:3:5 │ 3 │ name: String, - │ ^^^^ I was not expecting this + │ ^^^^^^^^^^^^ I was not expecting this Each custom type variant must have a constructor: diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_invalid_field_type.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_invalid_field_type.snap index c88b13e02..4581720e5 100644 --- a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_invalid_field_type.snap +++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_invalid_field_type.snap @@ -14,12 +14,7 @@ error: Syntax error ┌─ /src/parse/error.gleam:3:5 │ 3 │ name: "Test User", - │ ^^^^ I was not expecting this + │ ^^^^ I'm expecting a type name here -Each custom type variant must have a constructor: - -type User { - User( - name: Type, - ) -} +Hint: Type names start with a uppercase letter, and can contain a-z, A-Z, +or 0-9. diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_without_field_type.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_without_field_type.snap index 96fa42ffe..44529f2f4 100644 --- a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_without_field_type.snap +++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__type_invalid_record_constructor_without_field_type.snap @@ -20,6 +20,6 @@ Each custom type variant must have a constructor: pub opaque type User { User( - name: Type, + name, ) } diff --git a/compiler-core/src/parse/tests.rs b/compiler-core/src/parse/tests.rs index fdd91867f..579c5e825 100644 --- a/compiler-core/src/parse/tests.rs +++ b/compiler-core/src/parse/tests.rs @@ -2510,3 +2510,26 @@ fn error_message_when_using_discard_pattern_for_as_pattern() { }" ); } + +#[test] +fn missing_constructor_name_with_multiple_fields() { + assert_module_error!( + r#" +pub type Wibble(a) { + field: Int, + other: a +} +"# + ); +} + +#[test] +fn lowercase_constructor_name_in_custom_type() { + assert_module_error!( + r#" +pub type Wibble { + wibble(Int, String) +} +"# + ); +}