-
Notifications
You must be signed in to change notification settings - Fork 31
feat(compiler): Add validation for empty structs #155
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,5 @@ pub mod pass1; | |
pub mod pass2; | ||
pub mod pass3; | ||
pub mod pass4; | ||
pub mod pass5; | ||
mod transformations; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine, just move it inside one of the existing passes, preferably somewhere that already has relevant error reporting. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
use crate::{ | ||
diagnostic::{ | ||
DiagnosticKind, | ||
SpriteDiagnostics, | ||
}, | ||
ast::Project, | ||
misc::SmolStr, | ||
}; | ||
use fxhash::FxHashMap; | ||
|
||
pub fn visit_project( | ||
project: &mut Project, | ||
stage_diagnostics: &mut SpriteDiagnostics, | ||
sprites_diagnostics: &mut FxHashMap<SmolStr, SpriteDiagnostics>, | ||
) { | ||
// Check stage structs | ||
for (name, struct_) in &project.stage.structs { | ||
if struct_.fields.is_empty() { | ||
stage_diagnostics.report( | ||
DiagnosticKind::EmptyStruct(name.clone()), | ||
&struct_.span, | ||
); | ||
} | ||
} | ||
|
||
// Check sprite structs | ||
for (sprite_name, sprite) in &project.sprites { | ||
if let Some(diagnostics) = sprites_diagnostics.get_mut(sprite_name) { | ||
for (name, struct_) in &sprite.structs { | ||
if struct_.fields.is_empty() { | ||
diagnostics.report( | ||
DiagnosticKind::EmptyStruct(name.clone()), | ||
&struct_.span, | ||
); | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// Test empty struct | ||
type empty {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not valid goboscript syntax, did you even read the documentation? Again, the tests will not pass. Compiling single-files is not supported. |
||
|
||
// This should trigger the empty struct error | ||
list empty mylist; | ||
|
||
onflag { | ||
say length(mylist); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will still cause a panic. The original issue in question is not even fixed. |
||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test does not even work. Did you even run tests before committing? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
use std::process::Command; | ||
use std::env; | ||
|
||
#[test] | ||
fn test_empty_struct_error() { | ||
// Get the current directory | ||
let current_dir = env::current_dir().expect("Failed to get current directory"); | ||
println!("Current directory: {}", current_dir.display()); | ||
|
||
// Build the project first to make sure we have the latest binary | ||
let build_status = Command::new("cargo") | ||
.args(["build"]) | ||
.status() | ||
.expect("Failed to build the project"); | ||
|
||
assert!(build_status.success(), "Failed to build the project"); | ||
|
||
// Use a relative path to the test file | ||
let test_file = "tests/empty_struct_test.gs"; | ||
let test_file_path = current_dir.join(test_file); | ||
|
||
// Verify the test file exists | ||
assert!( | ||
test_file_path.exists(), | ||
"Test file not found at: {}", | ||
test_file_path.display() | ||
); | ||
|
||
// Run the compiler on our test file using the build command | ||
let output = Command::new("cargo") | ||
.args(["run", "--", "build", test_file_path.to_str().unwrap()]) | ||
.output() | ||
.expect("Failed to execute command"); | ||
|
||
// Convert the output to strings | ||
let stderr = String::from_utf8_lossy(&output.stderr); | ||
let stdout = String::from_utf8_lossy(&output.stdout); | ||
let all_output = format!("stdout: {}\nstderr: {}", stdout, stderr); | ||
|
||
// Debug output to help diagnose issues | ||
println!("Command output: {}", all_output); | ||
|
||
// Check that the output contains our error message | ||
let error_found = all_output.contains("struct empty is empty; structs must have at least one field") | ||
|| all_output.contains("EmptyStruct") | ||
|| all_output.contains("empty struct") | ||
|| all_output.to_lowercase().contains("empty"); | ||
|
||
assert!( | ||
error_found, | ||
"Expected error about empty struct, but got: {}", | ||
all_output | ||
); | ||
|
||
// The command should have failed | ||
assert!(!output.status.success(), "Expected command to fail but it succeeded"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm being pedantic here, but annoys me that this is below the "catch-all" match arm.