From d2ae17ffc9d26327c584cb26b59510c7ed511efa Mon Sep 17 00:00:00 2001 From: Kaz Wesley Date: Mon, 16 Dec 2024 07:54:19 -0800 Subject: [PATCH] Disallow a confusing constructor syntax (#11856) Previously, a constructor or type definition with a single argument defined inline could use spaces within the argument's definition without parentheses, as in the draft spec. This syntax was found confusing and is no longer allowed. No usages occurred in our `.enso` files. Fixes #10812. --- CHANGELOG.md | 4 ++ .../test/AnnotationsCompilerTest.java | 2 +- lib/rust/parser/debug/src/lib.rs | 9 +++- lib/rust/parser/debug/tests/parse.rs | 8 +-- lib/rust/parser/src/macros/built_in.rs | 3 +- .../src/syntax/statement/function_def.rs | 53 +++---------------- .../parser/src/syntax/statement/type_def.rs | 4 +- 7 files changed, 30 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ddee6d8bd..4df253fba6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,12 @@ #### Enso Language & Runtime - [Intersection types & type checks][11600] +- A constructor or type definition with a single inline argument definition was + previously allowed to use spaces in the argument definition without + parentheses. [This is now a syntax error.][11856] [11600]: https://github.com/enso-org/enso/pull/11600 +[11856]: https://github.com/enso-org/enso/pull/11856 # Next Release diff --git a/engine/runtime-integration-tests/src/test/java/org/enso/compiler/test/AnnotationsCompilerTest.java b/engine/runtime-integration-tests/src/test/java/org/enso/compiler/test/AnnotationsCompilerTest.java index b78b8b06cd..c115fae832 100644 --- a/engine/runtime-integration-tests/src/test/java/org/enso/compiler/test/AnnotationsCompilerTest.java +++ b/engine/runtime-integration-tests/src/test/java/org/enso/compiler/test/AnnotationsCompilerTest.java @@ -67,7 +67,7 @@ public class AnnotationsCompilerTest extends CompilerTests { type Foo @a foo @b bar - Cons a b = a b + Cons a b """); var typeDefinition = (Definition.SugaredType) ir.bindings().apply(0); diff --git a/lib/rust/parser/debug/src/lib.rs b/lib/rust/parser/debug/src/lib.rs index 3c55e29423..987a30f9bd 100644 --- a/lib/rust/parser/debug/src/lib.rs +++ b/lib/rust/parser/debug/src/lib.rs @@ -67,7 +67,11 @@ where T: serde::Serialize + Reflect { let text_escape_token = rust_to_meta[&TextEscape::reflect().id]; let token_to_str = move |token: Value| { let range = token_code_range(&token, base); - code[range].to_owned().into_boxed_str() + if range.is_empty() { + "".into() + } else { + code[range].to_owned().into_boxed_str() + } }; for token in identish_tokens { let token_to_str_ = token_to_str.clone(); @@ -158,6 +162,9 @@ fn token_code_range(token: &Value, base: usize) -> std::ops::Range { |field| fields(token).find(|(name, _)| *name == field).unwrap().1.as_u64().unwrap() as u32; let begin = get_u32(":codeReprBegin"); let len = get_u32(":codeReprLen"); + if len == 0 { + return 0..0; + } let begin = (begin as u64) | (base as u64 & !(u32::MAX as u64)); let begin = if begin < (base as u64) { begin + 1 << 32 } else { begin }; let begin = begin as usize - base; diff --git a/lib/rust/parser/debug/tests/parse.rs b/lib/rust/parser/debug/tests/parse.rs index d567a46ed4..6795084da8 100644 --- a/lib/rust/parser/debug/tests/parse.rs +++ b/lib/rust/parser/debug/tests/parse.rs @@ -346,12 +346,14 @@ fn type_def_full() { #[test] fn type_def_defaults() { - let code = ["type Result error ok=Nothing", " Ok value:ok = Nothing"]; - test!(code.join("\n"), + test!("type Result error ok=Nothing\n Ok value:ok=Nothing\n Error (value:e = Nothing)", (TypeDef Result #((() (Ident error) () ()) (() (Ident ok) () ((Ident Nothing)))) #(,(Constructor::new("Ok") - .with_arg(sexp![(() (Ident value) (":" (Ident ok)) ((Ident Nothing)))]))))); + .with_arg(sexp![(() (Ident value) (":" (Ident ok)) ((Ident Nothing)))])) + ,(Constructor::new("Error") + .with_arg(sexp![(() (Ident value) (":" (Ident e)) ((Ident Nothing)))]))))); + expect_invalid_node("type Result\n Ok value:ok = Nothing"); } #[test] diff --git a/lib/rust/parser/src/macros/built_in.rs b/lib/rust/parser/src/macros/built_in.rs index 97af2301b1..548494d13b 100644 --- a/lib/rust/parser/src/macros/built_in.rs +++ b/lib/rust/parser/src/macros/built_in.rs @@ -277,7 +277,8 @@ fn lambda_body<'s>( let (body, mut rest) = segments.pop(); let arguments = rest.pop().unwrap(); let backslash = arguments.header.with_variant(token::variant::LambdaOperator()); - let arguments = syntax::parse_args(&mut arguments.result.tokens(), 0, precedence); + let arguments = + syntax::parse_args(&mut arguments.result.tokens(), 0, precedence, &mut default()); let arrow = body.header.with_variant(token::variant::ArrowOperator()); let body_expression = precedence.resolve(&mut body.result.tokens()); let body_expression = diff --git a/lib/rust/parser/src/syntax/statement/function_def.rs b/lib/rust/parser/src/syntax/statement/function_def.rs index 545375bbe9..ae9b2b6cb2 100644 --- a/lib/rust/parser/src/syntax/statement/function_def.rs +++ b/lib/rust/parser/src/syntax/statement/function_def.rs @@ -182,6 +182,7 @@ pub fn parse_args<'s>( items: &mut Vec>, start: usize, precedence: &mut Precedence<'s>, + args_buffer: &mut Vec>, ) -> Vec> { let mut arg_starts = vec![]; for (i, item) in items.iter().enumerate().skip(start) { @@ -189,13 +190,11 @@ pub fn parse_args<'s>( arg_starts.push(i); } } - let mut defs: Vec<_> = arg_starts - .drain(..) - .rev() - .map(|arg_start| parse_arg_def(items, arg_start, precedence)) - .collect(); - defs.reverse(); - defs + args_buffer.extend( + arg_starts.drain(..).rev().map(|arg_start| parse_arg_def(items, arg_start, precedence)), + ); + debug_assert_eq!(items.len(), start); + args_buffer.drain(..).rev().collect() } pub fn parse_constructor_definition<'s>( @@ -270,43 +269,12 @@ fn parse_constructor_decl<'s>( precedence: &mut Precedence<'s>, args_buffer: &mut Vec>, ) -> (token::Ident<'s>, Vec>) { - let args = parse_type_args(items, start + 1, precedence, args_buffer); + let args = parse_args(items, start + 1, precedence, args_buffer); let name = items.pop().unwrap().into_token().unwrap().try_into().unwrap(); debug_assert_eq!(items.len(), start); (name, args) } -pub fn parse_type_args<'s>( - items: &mut Vec>, - start: usize, - precedence: &mut Precedence<'s>, - args_buffer: &mut Vec>, -) -> Vec> { - if start == items.len() { - return default(); - } - let mut arg_starts = vec![start]; - let mut expecting_rhs = false; - for (i, item) in items.iter().enumerate().skip(start + 1) { - if expecting_rhs { - expecting_rhs = false; - continue; - } - if let Item::Token(Token { variant: token::Variant::AssignmentOperator(_), .. }) = item { - expecting_rhs = true; - continue; - } - if Spacing::of_item(item) == Spacing::Spaced { - arg_starts.push(i); - } - } - args_buffer.extend( - arg_starts.drain(..).rev().map(|arg_start| parse_arg_def(items, arg_start, precedence)), - ); - debug_assert_eq!(items.len(), start); - args_buffer.drain(..).rev().collect() -} - pub fn try_parse_foreign_function<'s>( items: &mut Vec>, start: usize, @@ -493,12 +461,7 @@ fn parse_arg_def<'s>( .or_else(|| open2.as_ref().map(|t| t.code.position_after())) .or_else(|| open1.as_ref().map(|t| t.code.position_after())) .or_else(|| type_.as_ref().map(|t| t.operator.left_offset.code.position_before())) - // Why does this one need a type annotation??? - .or_else(|| { - close2 - .as_ref() - .map(|t: &token::CloseSymbol| t.left_offset.code.position_before()) - }) + .or_else(|| close2.as_ref().map(|t| t.left_offset.code.position_before())) .or_else(|| default.as_ref().map(|t| t.equals.left_offset.code.position_before())) .or_else(|| close1.as_ref().map(|t| t.left_offset.code.position_before())) .unwrap(), diff --git a/lib/rust/parser/src/syntax/statement/type_def.rs b/lib/rust/parser/src/syntax/statement/type_def.rs index 828a22dfaf..cc6ee56aa4 100644 --- a/lib/rust/parser/src/syntax/statement/type_def.rs +++ b/lib/rust/parser/src/syntax/statement/type_def.rs @@ -5,8 +5,8 @@ use crate::syntax::maybe_with_error; use crate::syntax::operator::Precedence; use crate::syntax::statement::apply_excess_private_keywords; use crate::syntax::statement::compound_lines; +use crate::syntax::statement::function_def::parse_args; use crate::syntax::statement::function_def::parse_constructor_definition; -use crate::syntax::statement::function_def::parse_type_args; use crate::syntax::statement::parse_statement; use crate::syntax::statement::scan_private_keywords; use crate::syntax::statement::EvaluationContext; @@ -66,7 +66,7 @@ pub fn try_parse_type_def<'s>( default() }; - let params = parse_type_args(items, start + 2, precedence, args_buffer); + let params = parse_args(items, start + 2, precedence, args_buffer); let name = items.pop().unwrap().into_token().unwrap().try_into().unwrap();