Fix unbound variable error when matching on const record subjects#5302
Fix unbound variable error when matching on const record subjects#5302IgorCastejon wants to merge 4 commits intogleam-lang:mainfrom
Conversation
| ValueConstructorVariant::ModuleConstant { .. } => { | ||
| // If the variable is a constant we shadow it with the refined type. | ||
| // We cannot convert it to a local variable as that would cause the | ||
| // code generator to look for a runtime variable that doesn't exist. | ||
| let mut refined_constant = variable.clone(); | ||
| refined_constant.type_ = type_; | ||
| let _ = self.environment.scope.insert(name, refined_constant); | ||
| } |
There was a problem hiding this comment.
I added this specific block to allow field access of const records (like user.name) to work inside the branch, matching the current behavior for local variables.
However, I noticed that Gleam doesn't do this type refinement for constants globally:
Gleam playground
Since the fix for the runtime crash doesn't strictly require this part (we could just ignore ModuleConstant entirely here), I have two questions:
- Should I keep or remove this code?
- Should I open a separate issue about global type refinement for constants (or is the current behavior deliberate)?
There was a problem hiding this comment.
Yeah I'd open a second separate issue for type refinements of constants and not add any code related to refinement in this PR. Makes it easier to review!
There was a problem hiding this comment.
This worked in the JavaScript target, but for the Erlang target it would generate an invalid local variable:
Gleam playground
So removing this code now results in a compiler error that would break existing code. Should I still remove it?
There was a problem hiding this comment.
I've talked with @giacomocavalieri and we agreed the code should be kept! Otherwise, removing it will break variant inference for consts, which is currently possible in the JavaScript target.
| source: compiler-core/src/javascript/tests/case.rs | ||
| expression: "\npub type Wibble {\n Wibble\n Wobble(int: Int)\n}\n\nconst wibble = Wibble\n\npub fn main() {\n echo wibble\n // This 'wibble' shadows the const\n let wibble = Wobble(42)\n case wibble {\n // This matches the local variable, not the const\n Wobble(_) -> wibble\n }\n}\n" | ||
| --- | ||
| ----- SOURCE CODE | ||
|
|
||
| pub type Wibble { | ||
| Wibble | ||
| Wobble(int: Int) | ||
| } | ||
|
|
||
| const wibble = Wibble | ||
|
|
||
| pub fn main() { | ||
| echo wibble | ||
| // This 'wibble' shadows the const | ||
| let wibble = Wobble(42) | ||
| case wibble { | ||
| // This matches the local variable, not the const | ||
| Wobble(_) -> wibble | ||
| } | ||
| } |
There was a problem hiding this comment.
Just something of interest I noticed while testing: if a const is defined but unused, it is properly optimized out of the generated JavaScript, but it still seems to have its name reserved in the variable generator.
Gleam playground
90fd708 to
a071320
Compare
...ots/gleam_core__javascript__tests__case__local_variable_record_access_variant_inference.snap
Show resolved
Hide resolved
| ValueConstructorVariant::ModuleConstant { .. } => { | ||
| // If the variable is a constant we shadow it with the refined type. | ||
| // We cannot convert it to a local variable as that would cause the | ||
| // code generator to look for a runtime variable that doesn't exist. | ||
| let mut refined_constant = variable.clone(); | ||
| refined_constant.type_ = type_; | ||
| let _ = self.environment.scope.insert(name, refined_constant); | ||
| } |
There was a problem hiding this comment.
Yeah I'd open a second separate issue for type refinements of constants and not add any code related to refinement in this PR. Makes it easier to review!
a071320 to
f70a809
Compare
lpil
left a comment
There was a problem hiding this comment.
Thank you!! I've left some questions inline 🙏
compiler-core/src/type_/pattern.rs
Outdated
| // code generator to look for a runtime variable that doesn't exist. | ||
| let mut refined_constant = variable.clone(); | ||
| refined_constant.type_ = type_; | ||
| let _ = self.environment.scope.insert(name, refined_constant); |
There was a problem hiding this comment.
Remove the references to refinement types please
| ValueConstructorVariant::ModuleConstant { .. } => { | ||
| // If the variable is a constant we shadow it with the refined type. | ||
| // We cannot convert it to a local variable as that would cause the | ||
| // code generator to look for a runtime variable that doesn't exist. | ||
| let mut refined_constant = variable.clone(); | ||
| refined_constant.type_ = type_; | ||
| let _ = self.environment.scope.insert(name, refined_constant); | ||
| } |
...ots/gleam_core__javascript__tests__case__local_variable_record_access_variant_inference.snap
Show resolved
Hide resolved
| } else { | ||
| return wibble.int; | ||
| } | ||
| } |
There was a problem hiding this comment.
How come this is generating flow control instead of returning 24 unconditionally like it would with let wibble = Wobble(42)?
There was a problem hiding this comment.
Sounds like a separate issue, the code was already generated like this before this PR:
Gleam Playground
f70a809 to
b4a9b70
Compare
Closes #5261.