From f58214327ba2c85d51900d83e12c2f9b789a170d Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Mon, 8 Jul 2024 07:40:20 -0500 Subject: [PATCH 01/16] commonize the codepaths to ensure spans are the same --- compiler-core/src/parse.rs | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/compiler-core/src/parse.rs b/compiler-core/src/parse.rs index 6df9c5ca1..1f0222850 100644 --- a/compiler-core/src/parse.rs +++ b/compiler-core/src/parse.rs @@ -3147,14 +3147,20 @@ where ) -> Result { let (_, name, _) = self.expect_name()?; + match name.as_str() { + "erlang" | "javascript" | "nix" => {} + _ => return parse_error(ParseErrorType::UnknownAttribute, SrcSpan::new(start, end)), + }; + + let _ = self.expect_one(&Token::Comma)?; + let (_, module, _) = self.expect_string()?; + let _ = self.expect_one(&Token::Comma)?; + let (_, function, _) = self.expect_string()?; + let _ = self.maybe_one(&Token::Comma); + let (_, end) = self.expect_one(&Token::RightParen)?; + match name.as_str() { "erlang" => { - let _ = self.expect_one(&Token::Comma)?; - let (_, module, _) = self.expect_string()?; - let _ = self.expect_one(&Token::Comma)?; - let (_, function, _) = self.expect_string()?; - let _ = self.maybe_one(&Token::Comma); - let (_, end) = self.expect_one(&Token::RightParen)?; if attributes.external_erlang.is_some() { return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); } @@ -3163,12 +3169,6 @@ where } "javascript" => { - let _ = self.expect_one(&Token::Comma)?; - let (_, module, _) = self.expect_string()?; - let _ = self.expect_one(&Token::Comma)?; - let (_, function, _) = self.expect_string()?; - let _ = self.maybe_one(&Token::Comma); - let _ = self.expect_one(&Token::RightParen)?; if attributes.external_javascript.is_some() { return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); } @@ -3177,12 +3177,6 @@ where } "nix" => { - let _ = self.expect_one(&Token::Comma)?; - let (_, module, _) = self.expect_string()?; - let _ = self.expect_one(&Token::Comma)?; - let (_, function, _) = self.expect_string()?; - let _ = self.maybe_one(&Token::Comma); - let _ = self.expect_one(&Token::RightParen)?; if attributes.external_nix.is_some() { return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); } From 7d9250dfc60e765f8d3f11bf6ea88cb7068a89af Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Mon, 8 Jul 2024 07:42:52 -0500 Subject: [PATCH 02/16] cleanup more by introduction has_external_for and set_external_for helpers --- compiler-core/src/parse.rs | 53 ++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/compiler-core/src/parse.rs b/compiler-core/src/parse.rs index 1f0222850..2c249b6fb 100644 --- a/compiler-core/src/parse.rs +++ b/compiler-core/src/parse.rs @@ -128,6 +128,22 @@ impl Attributes { || self.external_javascript.is_some() || self.external_nix.is_some() } + + fn has_external_for(&self, target: Target) -> bool { + match target { + Target::Erlang => self.external_erlang.is_some(), + Target::JavaScript => self.external_javascript.is_some(), + Target::Nix => self.external_nix.is_some(), + } + } + + fn set_external_for(&mut self, target: Target, ext: Option<(EcoString, EcoString)>) { + match target { + Target::Erlang => self.external_erlang = ext, + Target::JavaScript => self.external_javascript = ext, + Target::Nix => self.external_nix = ext, + } + } } // @@ -3147,8 +3163,10 @@ where ) -> Result { let (_, name, _) = self.expect_name()?; - match name.as_str() { - "erlang" | "javascript" | "nix" => {} + let target = match name.as_str() { + "erlang" => Target::Erlang, + "javascript" => Target::JavaScript, + "nix" => Target::Nix, _ => return parse_error(ParseErrorType::UnknownAttribute, SrcSpan::new(start, end)), }; @@ -3159,33 +3177,12 @@ where let _ = self.maybe_one(&Token::Comma); let (_, end) = self.expect_one(&Token::RightParen)?; - match name.as_str() { - "erlang" => { - if attributes.external_erlang.is_some() { - return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); - } - attributes.external_erlang = Some((module, function)); - Ok(end) - } - - "javascript" => { - if attributes.external_javascript.is_some() { - return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); - } - attributes.external_javascript = Some((module, function)); - Ok(end) - } - - "nix" => { - if attributes.external_nix.is_some() { - return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); - } - attributes.external_nix = Some((module, function)); - Ok(end) - } - - _ => parse_error(ParseErrorType::UnknownAttribute, SrcSpan::new(start, end)), + if attributes.has_external_for(target) { + return parse_error(ParseErrorType::DuplicateAttribute, SrcSpan { start, end }); } + + attributes.set_external_for(target, Some((module, function))); + Ok(end) } fn parse_deprecated_attribute( From 128341f637ff71c480e218ea84c77546ee254e32 Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Mon, 8 Jul 2024 07:44:24 -0500 Subject: [PATCH 03/16] update snapshot (which demonstrates the fix) --- ...e__tests__multiple_external_for_same_project_javascript.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler-core/src/parse/snapshots/glistix_core__parse__tests__multiple_external_for_same_project_javascript.snap b/compiler-core/src/parse/snapshots/glistix_core__parse__tests__multiple_external_for_same_project_javascript.snap index 5e6aaa2b6..697777e80 100644 --- a/compiler-core/src/parse/snapshots/glistix_core__parse__tests__multiple_external_for_same_project_javascript.snap +++ b/compiler-core/src/parse/snapshots/glistix_core__parse__tests__multiple_external_for_same_project_javascript.snap @@ -6,6 +6,6 @@ error: Syntax error ┌─ /src/parse/error.gleam:3:1 │ 3 │ @external(javascript, "three", "four") - │ ^^^^^^^^^ Duplicate attribute + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Duplicate attribute This attribute has already been given. From 189295f55c69996c43dff548a6ba96ef673b4a55 Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Wed, 10 Jul 2024 20:20:12 -0500 Subject: [PATCH 04/16] update changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89ef9a7e5..c03851387 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +### Bug Fixes + +- Fixed a bug where the compiler would report errors for duplicate `@external` + attributes with inconsistent spans between Erlang and JavaScript. + ([Connor Szczepaniak](https://github.com/cszczepaniak)) + +## v1.3.0-rc1 - 2024-06-30 + ### Build tool ### Compiler From 9ea5d01f43753c1838b688ae8b53ca871092a131 Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Wed, 10 Jul 2024 20:23:01 -0500 Subject: [PATCH 05/16] fix bad changelog merge --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c03851387..3355fc067 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,6 @@ attributes with inconsistent spans between Erlang and JavaScript. ([Connor Szczepaniak](https://github.com/cszczepaniak)) -## v1.3.0-rc1 - 2024-06-30 - ### Build tool ### Compiler From a9855031a3b8653f4d95c773701435cf4c519604 Mon Sep 17 00:00:00 2001 From: Connor Szczepaniak Date: Wed, 10 Jul 2024 20:23:43 -0500 Subject: [PATCH 06/16] fix bad changelog update --- CHANGELOG.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3355fc067..7a12260a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,6 @@ ## Unreleased -### Bug Fixes - -- Fixed a bug where the compiler would report errors for duplicate `@external` - attributes with inconsistent spans between Erlang and JavaScript. - ([Connor Szczepaniak](https://github.com/cszczepaniak)) - ### Build tool ### Compiler @@ -18,6 +12,10 @@ ### Bug Fixes +- Fixed a bug where the compiler would report errors for duplicate `@external` + attributes with inconsistent spans between Erlang and JavaScript. + ([Connor Szczepaniak](https://github.com/cszczepaniak)) + ## v1.3.1 - 2024-07-10 ### Bug Fixes From 357bbf84d6ce0f704a963d7707bb7ee375f6e8c3 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Wed, 10 Jul 2024 15:31:29 +0200 Subject: [PATCH 07/16] Do not show completions when typing strings --- CHANGELOG.md | 6 +++- compiler-core/src/language_server/engine.rs | 3 ++ .../src/language_server/tests/completion.rs | 28 +++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a12260a5..042c8be33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ ### Language Server +- The language server no longer shows completions when inside a literal string. + ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) + ### Bug Fixes - Fixed a bug where the compiler would report errors for duplicate `@external` @@ -20,5 +23,6 @@ ### Bug Fixes -- Fixes a bug with import cycle detection when there is more than 2 imports in the cycle +- Fixes a bug with import cycle detection when there is more than 2 imports in + the cycle. ([Ameen Radwan](https://github.com/Acepie)) diff --git a/compiler-core/src/language_server/engine.rs b/compiler-core/src/language_server/engine.rs index 3ba86fcca..181c1b3dd 100644 --- a/compiler-core/src/language_server/engine.rs +++ b/compiler-core/src/language_server/engine.rs @@ -216,6 +216,9 @@ where Located::PatternSpread { .. } => None, Located::Pattern(_pattern) => None, + // Do not show completions when typing inside a string. + Located::Expression(TypedExpr::String { .. }) => None, + Located::Statement(_) | Located::Expression(_) => { Some(completer.completion_values()) } diff --git a/compiler-core/src/language_server/tests/completion.rs b/compiler-core/src/language_server/tests/completion.rs index 602ddbfef..6f51bc879 100644 --- a/compiler-core/src/language_server/tests/completion.rs +++ b/compiler-core/src/language_server/tests/completion.rs @@ -1202,3 +1202,31 @@ pub fn main() { vec![], ); } + +#[test] +fn ignore_completions_inside_empty_string() { + let code = " +pub fn main() { + \"\" +} +"; + + assert_eq!( + completion(TestProject::for_source(code), Position::new(2, 2)), + vec![], + ); +} + +#[test] +fn ignore_completions_inside_string() { + let code = " +pub fn main() { + \"Ok()\" +} +"; + + assert_eq!( + completion(TestProject::for_source(code), Position::new(2, 5)), + vec![], + ); +} From 03b7500d84b002e46566503e9e714c43cd0510ba Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 13:50:01 +0100 Subject: [PATCH 08/16] CI test --- .github/workflows/ci.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0c26a5e93..2bc0388b7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -214,6 +214,13 @@ jobs: cd lib_project glistix run --target erlang glistix test --target erlang + + # Test adding of deps + glistix add exception # No specifier + glistix add gleam_http@3 # Version specifier + glistix test --target erlang + + # Test documentation generation glistix docs build # Assert that module metadata has been written From 340846b7da48624c6edac198ffbb3e3d4e9893c3 Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 14:12:50 +0100 Subject: [PATCH 09/16] Fix version passing --- compiler-cli/src/add.rs | 29 ++++-- compiler-cli/src/dependencies.rs | 151 +++++++++++++++---------------- 2 files changed, 94 insertions(+), 86 deletions(-) diff --git a/compiler-cli/src/add.rs b/compiler-cli/src/add.rs index 9a549fe94..bd74e5f34 100644 --- a/compiler-cli/src/add.rs +++ b/compiler-cli/src/add.rs @@ -5,17 +5,26 @@ use glistix_core::{ Error, Result, }; -use crate::{cli, dependencies::UseManifest, fs}; +use crate::{ + cli, + dependencies::{parse_gleam_add_specifier, UseManifest}, + fs, +}; -pub fn command(packages: Vec, dev: bool) -> Result<()> { +pub fn command(packages_to_add: Vec, dev: bool) -> Result<()> { let paths = crate::find_project_paths()?; + let mut new_package_requirements = Vec::with_capacity(packages_to_add.len()); + for specifier in packages_to_add { + new_package_requirements.push(parse_gleam_add_specifier(&specifier)?); + } + // Insert the new packages into the manifest and perform dependency // resolution to determine suitable versions let manifest = crate::dependencies::download( &paths, cli::Reporter::new(), - Some((packages.to_vec(), dev)), + Some((new_package_requirements.clone(), dev)), UseManifest::Yes, )?; @@ -24,12 +33,14 @@ pub fn command(packages: Vec, dev: bool) -> Result<()> { let mut manifest_toml = read_toml_edit("manifest.toml")?; // Insert the new deps - for package_to_add in packages { + for (added_package, _) in new_package_requirements { + let added_package = added_package.to_string(); + // Pull the selected version out of the new manifest so we know what it is let version = &manifest .packages .iter() - .find(|package| package.name == *package_to_add) + .find(|package| package.name == *added_package) .expect("Added package not found in resolved manifest") .version; @@ -49,16 +60,16 @@ pub fn command(packages: Vec, dev: bool) -> Result<()> { #[allow(clippy::indexing_slicing)] { if dev { - gleam_toml["dev-dependencies"][&package_to_add] = toml_edit::value(range.clone()); + gleam_toml["dev-dependencies"][&added_package] = toml_edit::value(range.clone()); } else { - gleam_toml["dependencies"][&package_to_add] = toml_edit::value(range.clone()); + gleam_toml["dependencies"][&added_package] = toml_edit::value(range.clone()); }; - manifest_toml["requirements"][&package_to_add] + manifest_toml["requirements"][&added_package] .as_inline_table_mut() .expect("Invalid manifest format")["version"] = range.into(); } - cli::print_added(&format!("{package_to_add} v{version}")); + cli::print_added(&format!("{added_package} v{version}")); } // Write the updated config diff --git a/compiler-cli/src/dependencies.rs b/compiler-cli/src/dependencies.rs index 17d5c9a99..b780e83bd 100644 --- a/compiler-cli/src/dependencies.rs +++ b/compiler-cli/src/dependencies.rs @@ -119,133 +119,131 @@ pub fn update() -> Result<()> { Ok(()) } -fn parse_hex_requirement(package: &str) -> Result { - match package.find('@') { - Some(pos) => { - // Parse the major and minor from the provided semantic version. - let version = match package.get(pos + 1..) { - Some(version) => Ok(version), - None => Err(Error::InvalidVersionFormat { - input: package.to_string(), - error: "Failed to parse version from specifier".to_string(), - }), - }?; - let parts = version.split('.').collect::>(); - let major = match parts.first() { - Some(major) => Ok(major), - None => Err(Error::InvalidVersionFormat { - input: package.to_string(), - error: "Failed to parse semantic major version".to_string(), - }), - }?; - let minor = match parts.get(1) { - Some(minor) => minor, - None => "0", - }; +pub fn parse_gleam_add_specifier(package: &str) -> Result<(EcoString, Requirement)> { + let Some((package, version)) = package.split_once('@') else { + // Default to the latest version available. + return Ok((package.into(), Requirement::hex(">= 0.0.0"))); + }; - // Using the major version specifier, calculate the maximum - // allowable version (i.e., the next major version). - let max_ver = match major.parse::() { - Ok(num) => Ok([&(num + 1).to_string(), "0", "0"].join(".")), - Err(_) => Err(Error::InvalidVersionFormat { - input: version.to_string(), - error: "Failed to parse semantic major version as integer".to_string(), - }), - }?; - - // Pad the provided version specifier with zeros map to a Hex version. - match parts.len() { - 1 | 2 => { - let min_ver = [major, minor, "0"].join("."); - Ok(Requirement::hex( - &[">=", &min_ver, "and", "<", &max_ver].join(" "), - )) - } - 3 => Ok(Requirement::hex(version)), - n_parts => Err(Error::InvalidVersionFormat { - input: version.to_string(), - error: format!( - "Expected up to 3 numbers in version specifier (MAJOR.MINOR.PATCH), found {n_parts}" - ), - }), - } + // Parse the major and minor from the provided semantic version. + let parts = version.split('.').collect::>(); + let major = match parts.first() { + Some(major) => Ok(major), + None => Err(Error::InvalidVersionFormat { + input: package.to_string(), + error: "Failed to parse semantic major version".to_string(), + }), + }?; + let minor = match parts.get(1) { + Some(minor) => minor, + None => "0", + }; + + // Using the major version specifier, calculate the maximum + // allowable version (i.e., the next major version). + let Ok(num) = major.parse::() else { + return Err(Error::InvalidVersionFormat { + input: version.to_string(), + error: "Failed to parse semantic major version as integer".to_string(), + }); + }; + + let max_ver = [&(num + 1).to_string(), "0", "0"].join("."); + + // Pad the provided version specifier with zeros map to a Hex version. + let requirement = match parts.len() { + 1 | 2 => { + let min_ver = [major, minor, "0"].join("."); + Requirement::hex(&[">=", &min_ver, "and", "<", &max_ver].join(" ")) + } + 3 => Requirement::hex(version), + n => { + return Err(Error::InvalidVersionFormat { + input: version.to_string(), + error: format!( + "Expected up to 3 numbers in version specifier (MAJOR.MINOR.PATCH), found {n}" + ), + }) } + }; - // Default to the latest version available. - None => Ok(Requirement::hex(">= 0.0.0")), - } + Ok((package.into(), requirement)) } #[test] -fn parse_hex_requirement_invalid_semver() { - assert!(parse_hex_requirement("some_package@1.2.3.4").is_err()); +fn parse_gleam_add_specifier_invalid_semver() { + assert!(parse_gleam_add_specifier("some_package@1.2.3.4").is_err()); } #[test] -fn parse_hex_requirement_non_numeric_version() { - assert!(parse_hex_requirement("some_package@not_a_version").is_err()); +fn parse_gleam_add_specifier_non_numeric_version() { + assert!(parse_gleam_add_specifier("some_package@not_a_version").is_err()); } #[test] -fn parse_hex_requirement_default() { +fn parse_gleam_add_specifier_default() { let provided = "some_package"; let expected = ">= 0.0.0"; - let version = parse_hex_requirement(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); } _ => assert!(false, "failed hexpm version parse: {}", provided), } - assert_eq!(version, Requirement::hex(expected)) + assert_eq!(version, Requirement::hex(expected)); + assert_eq!("some_package", package); } #[test] -fn parse_hex_requirement_major_only() { - let provided = "some_package@1"; +fn parse_gleam_add_specifier_major_only() { + let provided = "wobble@1"; let expected = ">= 1.0.0 and < 2.0.0"; - let version = parse_hex_requirement(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); } _ => assert!(false, "failed hexpm version parse: {}", provided), } - assert_eq!(version, Requirement::hex(expected)) + assert_eq!(version, Requirement::hex(expected)); + assert_eq!("wobble", package); } #[test] -fn parse_hex_requirement_major_and_minor() { - let provided = "some_package@1.2"; +fn parse_gleam_add_specifier_major_and_minor() { + let provided = "wibble@1.2"; let expected = ">= 1.2.0 and < 2.0.0"; - let version = parse_hex_requirement(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); } _ => assert!(false, "failed hexpm version parse: {}", provided), } - assert_eq!(version, Requirement::hex(expected)) + assert_eq!(version, Requirement::hex(expected)); + assert_eq!("wibble", package); } #[test] -fn parse_hex_requirement_major_minor_and_patch() { - let provided = "some_package@1.2.3"; +fn parse_gleam_add_specifier_major_minor_and_patch() { + let provided = "bobble@1.2.3"; let expected = "1.2.3"; - let version = parse_hex_requirement(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); } _ => assert!(false, "failed hexpm version parse: {}", provided), } - assert_eq!(version, Requirement::hex(expected)) + assert_eq!(version, Requirement::hex(expected)); + assert_eq!("bobble", package); } pub fn download( paths: &ProjectPaths, telemetry: Telem, - new_package: Option<(Vec, bool)>, + new_package: Option<(Vec<(EcoString, Requirement)>, bool)>, // If true we read the manifest from disc. If not set then we ignore any // manifest which will result in the latest versions of the dependency // packages being resolved (not the locked ones). @@ -271,12 +269,11 @@ pub fn download( // Insert the new packages to add, if it exists if let Some((packages, dev)) = new_package { - for package in packages { - let version = parse_hex_requirement(&package)?; - let _ = if dev { - config.dev_dependencies.insert(package.into(), version) + for (package, requirement) in packages { + if dev { + _ = config.dev_dependencies.insert(package, requirement); } else { - config.dependencies.insert(package.into(), version) + _ = config.dependencies.insert(package, requirement); }; } } From 16cd611538fc06f6e898dcd196a146a5115aedb5 Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 14:15:11 +0100 Subject: [PATCH 10/16] Changes! --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 042c8be33..657928879 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ attributes with inconsistent spans between Erlang and JavaScript. ([Connor Szczepaniak](https://github.com/cszczepaniak)) +- Fixed a bug where `gleam add` would fail to parse version specifiers + correctly. + ([Louis Pilfold](https://github.com/lpil)) + ## v1.3.1 - 2024-07-10 ### Bug Fixes From 6e7f5a9df1c8949f09e128f23d37bd5d031d742e Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 15:04:27 +0100 Subject: [PATCH 11/16] Fix JS scope issue Closes https://github.com/gleam-lang/gleam/issues/3379 --- compiler-core/src/javascript/expression.rs | 31 +++++++++--------- compiler-core/src/javascript/tests/case.rs | 32 +++++++++++++++++++ ...ks__nested_multiexpr_blocks_with_case.snap | 5 ++- ...e__javascript__tests__bools__nil_case.snap | 5 ++- ...e__javascript__tests__case__pointless.snap | 5 ++- ..._tests__case__single_clause_variables.snap | 15 +++++++++ ...ase__single_clause_variables_assigned.snap | 17 ++++++++++ ...on_fn_with_matching_parameter_in_case.snap | 5 ++- ..._core__javascript__tests__panic__case.snap | 12 ++++++- 9 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables.snap create mode 100644 compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables_assigned.snap diff --git a/compiler-core/src/javascript/expression.rs b/compiler-core/src/javascript/expression.rs index 3210b6f7f..b5ba450e7 100644 --- a/compiler-core/src/javascript/expression.rs +++ b/compiler-core/src/javascript/expression.rs @@ -366,23 +366,10 @@ impl<'module> Generator<'module> { self.scope_position = scope_position; // Wrap in iife document - let doc = self.immediately_involked_function_expression_document(result?); + let doc = immediately_involked_function_expression_document(result?); Ok(self.wrap_return(doc)) } - /// Wrap a document in an immediately involked function expression - fn immediately_involked_function_expression_document<'a>( - &mut self, - document: Document<'a>, - ) -> Document<'a> { - docvec!( - docvec!("(() => {", break_("", " "), document).nest(INDENT), - break_("", " "), - "})()", - ) - .group() - } - fn variable<'a>( &mut self, name: &'a EcoString, @@ -631,7 +618,11 @@ impl<'module> Generator<'module> { doc = if is_only_clause { // If this is the only clause and there are no checks then we can // render just the body as the case does nothing - doc.append(body) + // A block is used as it could declare variables still. + doc.append("{") + .append(docvec!(line(), body).nest(INDENT)) + .append(line()) + .append("}") } else if is_final_clause { // If this is the final clause and there are no checks then we can // render `else` instead of `else if (...)` @@ -1602,3 +1593,13 @@ fn requires_semicolon(statement: &TypedStatement) -> bool { Statement::Use(_) => false, } } + +/// Wrap a document in an immediately involked function expression +fn immediately_involked_function_expression_document<'a>(document: Document<'a>) -> Document<'a> { + docvec!( + docvec!("(() => {", break_("", " "), document).nest(INDENT), + break_("", " "), + "})()", + ) + .group() +} diff --git a/compiler-core/src/javascript/tests/case.rs b/compiler-core/src/javascript/tests/case.rs index 89d63cf2f..03735abe5 100644 --- a/compiler-core/src/javascript/tests/case.rs +++ b/compiler-core/src/javascript/tests/case.rs @@ -235,3 +235,35 @@ pub fn main() { "# ) } + +// https://github.com/gleam-lang/gleam/issues/3379 +#[test] +fn single_clause_variables() { + assert_js!( + r#" +pub fn main() { + let text = "first defined" + case "defined again" { + text -> Nil + } + let text = "a third time" +} +"# + ) +} + +// https://github.com/gleam-lang/gleam/issues/3379 +#[test] +fn single_clause_variables_assigned() { + assert_js!( + r#" +pub fn main() { + let text = "first defined" + let other = case "defined again" { + text -> Nil + } + let text = "a third time" +} +"# + ) +} diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__blocks__nested_multiexpr_blocks_with_case.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__blocks__nested_multiexpr_blocks_with_case.snap index 1c4bcf1f7..11d557f22 100644 --- a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__blocks__nested_multiexpr_blocks_with_case.snap +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__blocks__nested_multiexpr_blocks_with_case.snap @@ -1,5 +1,6 @@ --- source: compiler-core/src/javascript/tests/blocks.rs +assertion_line: 94 expression: "\nfn go() {\n let x = {\n 1\n {\n 2\n case True {\n _ -> 3\n }\n }\n }\n x\n}\n" --- function go() { @@ -8,7 +9,9 @@ function go() { return (() => { 2; let $ = true; - return 3; + { + return 3; + } })(); })(); return x; diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__bools__nil_case.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__bools__nil_case.snap index 14302f143..1c393a15b 100644 --- a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__bools__nil_case.snap +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__bools__nil_case.snap @@ -1,7 +1,10 @@ --- source: compiler-core/src/javascript/tests/bools.rs +assertion_line: 128 expression: "\nfn go(a) {\n case a {\n Nil -> 0\n }\n}\n" --- function go(a) { - return 0; + { + return 0; + } } diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__pointless.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__pointless.snap index 1a448ae8e..eea573a27 100644 --- a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__pointless.snap +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__pointless.snap @@ -1,7 +1,10 @@ --- source: compiler-core/src/javascript/tests/case.rs +assertion_line: 6 expression: "\nfn go(x) {\n case x {\n _ -> x\n }\n}\n" --- function go(x) { - return x; + { + return x; + } } diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables.snap new file mode 100644 index 000000000..d89793107 --- /dev/null +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables.snap @@ -0,0 +1,15 @@ +--- +source: compiler-core/src/javascript/tests/case.rs +assertion_line: 242 +expression: "\npub fn main() {\n let text = \"first defined\"\n case \"defined again\" {\n text -> Nil\n }\n let text = \"a third time\"\n}\n" +--- +export function main() { + let text = "first defined"; + let $ = "defined again"; + { + let text$1 = $; + undefined + } + let text$1 = "a third time"; + return text$1; +} diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables_assigned.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables_assigned.snap new file mode 100644 index 000000000..f6f4678d4 --- /dev/null +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__case__single_clause_variables_assigned.snap @@ -0,0 +1,17 @@ +--- +source: compiler-core/src/javascript/tests/case.rs +assertion_line: 258 +expression: "\npub fn main() {\n let text = \"first defined\"\n let other = case \"defined again\" {\n text -> Nil\n }\n let text = \"a third time\"\n}\n" +--- +export function main() { + let text = "first defined"; + let other = (() => { + let $ = "defined again"; + { + let text$1 = $; + return undefined; + } + })(); + let text$1 = "a third time"; + return text$1; +} diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__functions__variable_rewriting_in_anon_fn_with_matching_parameter_in_case.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__functions__variable_rewriting_in_anon_fn_with_matching_parameter_in_case.snap index 44544eb82..970e0f630 100644 --- a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__functions__variable_rewriting_in_anon_fn_with_matching_parameter_in_case.snap +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__functions__variable_rewriting_in_anon_fn_with_matching_parameter_in_case.snap @@ -1,12 +1,15 @@ --- source: compiler-core/src/javascript/tests/functions.rs +assertion_line: 341 expression: "pub fn bad() {\n fn(state) {\n let state = case Nil {\n _ -> state\n }\n state\n }\n}\n" --- export function bad() { return (state) => { let state$1 = (() => { let $ = undefined; - return state; + { + return state; + } })(); return state$1; }; diff --git a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__panic__case.snap b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__panic__case.snap index 9dbdca683..458d94e34 100644 --- a/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__panic__case.snap +++ b/compiler-core/src/javascript/tests/snapshots/glistix_core__javascript__tests__panic__case.snap @@ -1,9 +1,19 @@ --- source: compiler-core/src/javascript/tests/panic.rs +assertion_line: 74 expression: "\nfn go(x) {\n case x {\n _ -> panic\n }\n}\n" --- import { makeError } from "../gleam.mjs"; function go(x) { - throw makeError("panic", "my/mod", 4, "go", "panic expression evaluated", {}) + { + throw makeError( + "panic", + "my/mod", + 4, + "go", + "panic expression evaluated", + {} + ) + } } From 078e2d5defefc5612172ce7cff7c7905309f5a42 Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 15:05:34 +0100 Subject: [PATCH 12/16] Changes! --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 657928879..305f5131b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ correctly. ([Louis Pilfold](https://github.com/lpil)) +- Fixed a bug where single clause case expressions could generate JavaScript + code with incorrectly rewritten JavaScript variable names. + ([Louis Pilfold](https://github.com/lpil)) + ## v1.3.1 - 2024-07-10 ### Bug Fixes From 5f8287b0715f552fd3b8f4e49b7ec0cbe8dfafa5 Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 15:08:54 +0100 Subject: [PATCH 13/16] Clippy! --- compiler-cli/src/dependencies.rs | 8 ++++---- compiler-core/src/javascript/expression.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler-cli/src/dependencies.rs b/compiler-cli/src/dependencies.rs index b780e83bd..2ddf4ff42 100644 --- a/compiler-cli/src/dependencies.rs +++ b/compiler-cli/src/dependencies.rs @@ -184,7 +184,7 @@ fn parse_gleam_add_specifier_non_numeric_version() { fn parse_gleam_add_specifier_default() { let provided = "some_package"; let expected = ">= 0.0.0"; - let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); @@ -199,7 +199,7 @@ fn parse_gleam_add_specifier_default() { fn parse_gleam_add_specifier_major_only() { let provided = "wobble@1"; let expected = ">= 1.0.0 and < 2.0.0"; - let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); @@ -214,7 +214,7 @@ fn parse_gleam_add_specifier_major_only() { fn parse_gleam_add_specifier_major_and_minor() { let provided = "wibble@1.2"; let expected = ">= 1.2.0 and < 2.0.0"; - let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); @@ -229,7 +229,7 @@ fn parse_gleam_add_specifier_major_and_minor() { fn parse_gleam_add_specifier_major_minor_and_patch() { let provided = "bobble@1.2.3"; let expected = "1.2.3"; - let (package, version) = parse_gleam_add_specifier(&provided).unwrap(); + let (package, version) = parse_gleam_add_specifier(provided).unwrap(); match &version { Requirement::Hex { version: v } => { assert!(v.to_pubgrub().is_ok(), "failed pubgrub parse: {}", v); diff --git a/compiler-core/src/javascript/expression.rs b/compiler-core/src/javascript/expression.rs index b5ba450e7..fab75bba7 100644 --- a/compiler-core/src/javascript/expression.rs +++ b/compiler-core/src/javascript/expression.rs @@ -1595,7 +1595,7 @@ fn requires_semicolon(statement: &TypedStatement) -> bool { } /// Wrap a document in an immediately involked function expression -fn immediately_involked_function_expression_document<'a>(document: Document<'a>) -> Document<'a> { +fn immediately_involked_function_expression_document(document: Document<'_>) -> Document<'_> { docvec!( docvec!("(() => {", break_("", " "), document).nest(INDENT), break_("", " "), From 90671d5f88770a6038b7e6f77084a38e927e93cb Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Thu, 11 Jul 2024 15:13:45 +0100 Subject: [PATCH 14/16] v1.3.2 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 305f5131b..be6b050d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ ### Language Server +### Bug Fixes + +## v1.3.2 - 2024-07-11 + +### Language Server + - The language server no longer shows completions when inside a literal string. ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) From 56f42eb1df52d51b02d0d286ed207d675ebb722e Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Sun, 28 Jul 2024 15:44:02 -0300 Subject: [PATCH 15/16] workaround for failing gleam docs build --- .github/workflows/ci.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2bc0388b7..27f82d4af 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -221,6 +221,10 @@ jobs: glistix test --target erlang # Test documentation generation + # Workaround for lack of --target option for docs build + # TODO: Remove this workaround after Gleam 1.4 + # (See https://github.com/gleam-lang/gleam/pull/3333) + sed -i 's/target = "nix"/target = "erlang"/' gleam.toml glistix docs build # Assert that module metadata has been written From 429575199e6ddec40f731ac1221fd5d607a51241 Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Sun, 28 Jul 2024 16:35:08 -0300 Subject: [PATCH 16/16] ci: fix picky sed --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 27f82d4af..f18a16b09 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -224,7 +224,7 @@ jobs: # Workaround for lack of --target option for docs build # TODO: Remove this workaround after Gleam 1.4 # (See https://github.com/gleam-lang/gleam/pull/3333) - sed -i 's/target = "nix"/target = "erlang"/' gleam.toml + sed -i -e 's/target = "nix"/target = "erlang"/' gleam.toml glistix docs build # Assert that module metadata has been written