From 96b4eb4a51d59a3a7131745245fa6f7dd97e94ef Mon Sep 17 00:00:00 2001 From: Tin Svagelj Date: Mon, 30 Oct 2023 21:19:34 +0100 Subject: [PATCH] Fix a bug in class file reader Fix indentation writer Improve testing harness to produce a much more helpful development environment Signed-off-by: Tin Svagelj --- Cargo.toml | 4 ++ README.md | 4 ++ class_format/src/lib.rs | 42 +++++------- src/gen/indent.rs | 48 ++++++++----- src/gen/java/class.rs | 54 ++++++++++----- src/gen/java/code.rs | 7 +- src/gen/java/mod.rs | 2 +- src/ir/expression.rs | 6 +- tests/{units => }/.gitignore | 0 tests/hello_world.rs | 51 -------------- tests/run_units.rs | 118 ++++++++++++++++++++++++++++++++ tests/units/00_empty.java | 1 + tests/units/01_hello_world.java | 5 -- tests/units/01_return_lit.java | 5 ++ tests/units/02_hello_world.java | 5 ++ 15 files changed, 233 insertions(+), 119 deletions(-) rename tests/{units => }/.gitignore (100%) delete mode 100644 tests/hello_world.rs create mode 100644 tests/run_units.rs create mode 100644 tests/units/00_empty.java delete mode 100644 tests/units/01_hello_world.java create mode 100644 tests/units/01_return_lit.java create mode 100644 tests/units/02_hello_world.java diff --git a/Cargo.toml b/Cargo.toml index c3a32bc..bcc5bca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,3 +29,7 @@ zip = "0.6" serde = { version = "1.0", features = ["derive"] } clap = { version = "4.0", features = ["derive"], optional = true } + +[dev-dependencies] +pretty_assertions = "1.4" +tracing-subscriber = { version = "0.3" } diff --git a/README.md b/README.md index e65a609..525db1a 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,10 @@ Pattern matching doesn't cover all instructions yet so a lot of the output is co Most command line arguments aren't properly handled yet. +### Testing + +In order to run the test suite `JAVA_HOME` environment variable must be set or `javac` must be in path. + ## JVM support status Latest version is currently being written to support any semi-recent Java version. diff --git a/class_format/src/lib.rs b/class_format/src/lib.rs index 654aa7d..270f5f1 100644 --- a/class_format/src/lib.rs +++ b/class_format/src/lib.rs @@ -141,6 +141,14 @@ impl ClassPath { }); } + pub fn from_class_index( + pool: &ConstantPool, + class_index: usize, + ) -> Result { + let utf8_index = constant_match!(pool.get(class_index), Constant::Class { name_index } => { *name_index as usize })?; + ClassPath::try_from(pool.get(utf8_index)) + } + pub fn parse(name: impl AsRef) -> Result { let mut cursor = std::io::Cursor::new(name.as_ref()); Self::read_from(&mut cursor) @@ -230,26 +238,6 @@ pub struct Class { pub attributes: HashMap, } -fn name_from_class_index( - index: usize, - constant_pool: &ConstantPool, -) -> Result { - log::trace!("enter name_from_class_index({}, &constant_pool)", index); - let constant = constant_pool.try_get(index)?; - - match constant { - Constant::Class { name_index } => match constant_pool.try_get(*name_index as usize)? { - Constant::Utf8 { value } => Ok(ClassPath::parse(value)?), - _ => Err(ClassReadError::InvalidClassNameReference), - }, - other => Err(ConstantPoolError::UnexpectedType { - found: other.tag(), - expected: ConstantTag::Class, - } - .into()), - } -} - impl Class { pub fn open(path: impl AsRef) -> Result { let mut file = File::open(path)?; @@ -306,12 +294,18 @@ impl Class { "Class::try_from(impl Read)::class_name#{}", class_const_index ); - let class_name = ClassPath::try_from(constant_pool.get(class_const_index))?; + let class_name = ClassPath::from_class_index(&constant_pool, class_const_index)?; - log::trace!("Class::read_from(impl Read)::super_name"); let super_const_index = r.read_u16::()? as usize; + log::trace!( + "Class::read_from(impl Read)::super_name#{}", + super_const_index + ); let super_name = if super_const_index != 0 { - Some(ClassPath::try_from(constant_pool.get(class_const_index))?) + Some(ClassPath::from_class_index( + &constant_pool, + super_const_index, + )?) } else { None }; @@ -322,7 +316,7 @@ impl Class { for _ in 0..interface_count { let interface_index = r.read_u16::()? as usize; - let interface_name = name_from_class_index(interface_index, &constant_pool)?; + let interface_name = ClassPath::from_class_index(&constant_pool, interface_index)?; interfaces.push(interface_name); } diff --git a/src/gen/indent.rs b/src/gen/indent.rs index 7f1586d..7473498 100644 --- a/src/gen/indent.rs +++ b/src/gen/indent.rs @@ -1,4 +1,4 @@ -use std::{io::Write, collections::HashSet}; +use std::{collections::HashSet, io::Write}; #[derive(Debug, Copy, Clone, Eq, PartialEq)] #[repr(u8)] @@ -20,23 +20,31 @@ pub struct Indented { inner: W, pub level: usize, pub indent: IndentKind, + pub pending: bool, pub enter_block_on: HashSet, pub exit_block_on: HashSet, } impl Indented { - pub fn new(writer: W, indent: IndentKind, level: usize, enter_block_on: impl AsRef<[u8]>, exit_block_on: impl AsRef<[u8]>) -> Indented { + pub fn new( + writer: W, + indent: IndentKind, + level: usize, + enter_block_on: impl AsRef<[u8]>, + exit_block_on: impl AsRef<[u8]>, + ) -> Indented { Indented { inner: writer, level, indent, + pending: true, enter_block_on: HashSet::from_iter(enter_block_on.as_ref().iter().cloned()), exit_block_on: HashSet::from_iter(exit_block_on.as_ref().iter().cloned()), } } - + #[inline] pub fn enter_block(&mut self) { self.level += 1; @@ -68,36 +76,44 @@ impl Indented { impl Write for Indented { fn write(&mut self, buf: &[u8]) -> std::io::Result { let mut total = 0; + for byte in buf { - let count = match *byte as char { + total += match *byte as char { '\n' => { let nl_len = self.inner.write(b"\n")?; - let indent_len = self.inner.write(self.indent_string().as_bytes())?; - nl_len + indent_len - } - _ if self.enter_block_on.contains(byte) => { - self.enter_block(); - self.inner.write(&[*byte])? + self.pending = true; + nl_len } - _ if self.exit_block_on.contains(byte) => { - self.exit_block(); + _ => { + if self.enter_block_on.contains(byte) { + self.enter_block(); + } else if self.exit_block_on.contains(byte) { + self.exit_block(); + } + + if self.pending { + total += self.inner.write(self.indent_string().as_bytes())?; + self.pending = false; + } + self.inner.write(&[*byte])? } - _ => self.inner.write(&[*byte])?, }; - total += count; } Ok(total) } - + fn write_all(&mut self, buf: &[u8]) -> std::io::Result<()> { let written = self.write(buf)?; // TODO: Do a proper check // needs to handle indentation levels // rope science - https://xi-editor.io/docs/rope_science_04.html if written < buf.len() { - return Err(std::io::Error::new(std::io::ErrorKind::Other, "didn't write enough code")) + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "didn't write enough code", + )); } Ok(()) } diff --git a/src/gen/java/class.rs b/src/gen/java/class.rs index c7c1650..d2b4424 100644 --- a/src/gen/java/class.rs +++ b/src/gen/java/class.rs @@ -116,44 +116,60 @@ impl GenerateCode for JavaBackend { } } } - w.write_all(b" {\n")?; + w.write_all(b" {")?; // TODO: generate enum entries - tracing::debug!("- Generating fields for {}", class_name); + let contents = { + let mut content_buffer = Vec::with_capacity(512); + let mut w: Cursor<&mut Vec> = Cursor::new(&mut content_buffer); - let mut class_indent = Indented::new(&mut w, lang.indentation, 1, b"{", b"}"); + tracing::debug!("- Generating fields for {}", class_name); - for field in &class.fields { - let field_requirements = - self.write_value(&lang, &FieldContext, field, &mut class_indent)?; - req.add_import(field_requirements.imports); - } + let mut class_indent = Indented::new(&mut w, lang.indentation, 1, b"{", b"}"); - tracing::debug!("- Generating methods for {}", class_name); + for field in &class.fields { + let field_requirements = + self.write_value(&lang, &FieldContext, field, &mut class_indent)?; + req.add_import(field_requirements.imports); + } - for method in &class.methods { - let method_ctx = ClassContext { - class_name: class.class_name.clone(), - ..Default::default() - }; - let method_requirements = - self.write_value(&lang, &method_ctx, method, &mut class_indent)?; - req.add_import(method_requirements.imports); - } + tracing::debug!("- Generating methods for {}", class_name); + + for method in &class.methods { + let method_ctx = ClassContext { + class_name: class.class_name.clone(), + ..Default::default() + }; + let method_requirements = + self.write_value(&lang, &method_ctx, method, &mut class_indent)?; + req.add_import(method_requirements.imports); + } + content_buffer + }; + + if !contents.is_empty() { + w.write_all(b"\n")?; + } + w.write_all(&contents)?; w.write_all(b"}\n")?; w.flush()?; result }; + let mut has_imports = false; for import in req.imports.drain() { + has_imports = true; w.write_all(b"import ")?; w.write_all(import.full_path().as_bytes())?; w.write_all(b";\n")?; } - w.write_all(b"\n")?; + + if has_imports { + w.write_all(b"\n")?; + } w.write_all(&delayed)?; diff --git a/src/gen/java/code.rs b/src/gen/java/code.rs index dd54513..af325ba 100644 --- a/src/gen/java/code.rs +++ b/src/gen/java/code.rs @@ -21,7 +21,8 @@ impl<'m, 'data> GenerateCode> for JavaBack match input { Expression::Comment(it) => self.write_value(lang, ctx, it, w), Expression::Super(it) => self.write_value(lang, ctx, it, w), - _ => todo!("unimplemented expression"), + Expression::EmptyConstructor(_) => Ok(Default::default()), + expr => todo!("unimplemented expression: {:?}", expr), } } } @@ -40,7 +41,9 @@ impl<'m, 'data, B: GeneratorBackend> GenerateCode GenerateCode> for B { +impl<'m, 'data, B: GeneratorBackend> GenerateCode> + for B +{ fn write_value( &self, _: &Self::LanguageContext, diff --git a/src/gen/java/mod.rs b/src/gen/java/mod.rs index e448c1a..c8c5c9e 100644 --- a/src/gen/java/mod.rs +++ b/src/gen/java/mod.rs @@ -172,7 +172,7 @@ impl Default for JavaContext { header_message: Some( "Generated file - do not edit, your changes will be lost.".to_string(), ), - indentation: IndentKind::Space(4), + indentation: IndentKind::Space(2), constant_pool: None, } } diff --git a/src/ir/expression.rs b/src/ir/expression.rs index dac251a..f1a5902 100644 --- a/src/ir/expression.rs +++ b/src/ir/expression.rs @@ -37,6 +37,7 @@ pub trait CheckExpression { ) -> Option<(usize, Expression)>; } +#[derive(Debug)] pub enum Expression { EmptyConstructor(EmptyConstructor), ReturnStatement(ReturnStatement), @@ -64,6 +65,7 @@ impl CheckExpression for InstructionComment { } } +#[derive(Debug)] pub struct EmptyConstructor; impl CheckExpression for EmptyConstructor { @@ -91,7 +93,8 @@ impl CheckExpression for EmptyConstructor { } } -pub struct ReturnStatement; +#[derive(Debug)] +pub struct ReturnStatement; impl CheckExpression for ReturnStatement { fn test<'cp, 'code>( @@ -112,6 +115,7 @@ impl CheckExpression for ReturnStatement { } } +#[derive(Debug)] pub struct EmptySuperCall; impl CheckExpression for EmptySuperCall { diff --git a/tests/units/.gitignore b/tests/.gitignore similarity index 100% rename from tests/units/.gitignore rename to tests/.gitignore diff --git a/tests/hello_world.rs b/tests/hello_world.rs deleted file mode 100644 index a8bff87..0000000 --- a/tests/hello_world.rs +++ /dev/null @@ -1,51 +0,0 @@ -use std::fs::File; -use std::io::{BufWriter, Write}; -use std::process::Command; - -use jaded::gen::java::JavaBackend; -use jaded::gen::{GenerateCode, GeneratorBuilder}; -use jvm_class_format::error::ClassReadError; -use jvm_class_format::Class; - -// root structure of a java file is a class -pub fn compile(source: impl AsRef) -> Result, std::io::Error> { - let mut javac = Command::new("javac") - .args(["-nowarn", source.as_ref()]) - .spawn()?; - - javac.wait().unwrap(); - - let result = std::fs::read("tests/units/Unit.class"); - let _ = std::fs::remove_file("tests/units/Unit.class"); // it will be overriden - result -} - -const TEST_NAME: &str = "01_hello_world"; -#[test] -fn hello_world() -> Result<(), ClassReadError> { - #[cfg(feature = "tracing-subscriber")] - tracing_subscriber::fmt() - .with_max_level(tracing::Level::TRACE) - .init(); - #[cfg(not(feature = "tracing-subscriber"))] - println!("Running without logging"); - #[cfg(feature = "tracing-subscriber")] - tracing::info!("- {}", TEST_NAME); - - let src = compile(format!("tests/units/{}.java", TEST_NAME)).unwrap(); - let hello_world = Class::read(src)?; - - let lang = GeneratorBuilder::java().no_header().build(); - - let result = JavaBackend - .generate(&lang, &(), &hello_world) - .expect("unable to generate class code") - .0; - - let out = File::create("./sample_output.java").expect("unable to create output file"); - let mut w = BufWriter::new(out); - w.write(result.as_bytes()).unwrap(); - w.flush().unwrap(); - - Ok(()) -} diff --git a/tests/run_units.rs b/tests/run_units.rs new file mode 100644 index 0000000..99f4138 --- /dev/null +++ b/tests/run_units.rs @@ -0,0 +1,118 @@ +use std::cell::OnceCell; +use std::fs::{DirEntry, File}; +use std::io::{BufWriter, Read, Write}; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::str::FromStr; + +use jaded::gen::java::JavaBackend; +use jaded::gen::{GenerateCode, GeneratorBuilder}; +use jvm_class_format::error::ClassReadError; +use jvm_class_format::Class; + +fn javac(source: impl AsRef) -> Command { + static mut JAVA_HOME: OnceCell = OnceCell::new(); + let javac = unsafe { + JAVA_HOME.get_or_init(|| match std::env::var("JAVA_HOME") { + Ok(it) => PathBuf::from_str((it + "/bin/javac").as_str()).unwrap(), + Err(_) => PathBuf::from_str("javac").unwrap(), + }) + }; + + let mut c = Command::new(javac); + c.args([ + "-nowarn", + source.as_ref().to_str().expect("invalid source path"), + ]); + c +} + +// root structure of a java file is a class +pub fn compile(source: impl AsRef) -> Result, std::io::Error> { + let mut javac_command = javac(source) + .stdout(Stdio::null()) + .stderr(Stdio::piped()) + .spawn()?; + + let compilation_result = javac_command.wait()?; + + if !compilation_result.success() { + // If compilation fails, capture the error output and include it in the error message. + let mut error_output = String::new(); + if let Some(mut stderr) = javac_command.stderr { + stderr.read_to_string(&mut error_output)?; + } + + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + format!("compile error:\n{}", error_output), + )); + } + + let result = std::fs::read("tests/units/Unit.class")?; + let _ = std::fs::remove_file("tests/units/Unit.class"); // it will be overriden + Ok(result) +} + +fn entry_num(entry: &DirEntry) -> usize { + entry + .file_name() + .to_string_lossy() + .chars() + .take_while(|it| it.is_numeric()) + .collect::() + .parse::() + .unwrap() +} + +#[test] +fn run_units() -> Result<(), ClassReadError> { + tracing_subscriber::fmt() + .with_max_level(tracing::Level::INFO) + .init(); + + let lang = GeneratorBuilder::java().no_header().build(); + + let units = std::fs::read_dir("tests/units").expect("can't iterate test units"); + let mut units = units + .into_iter() + .filter_map(|it| it.ok()) + .collect::>(); + units.sort_by_key(entry_num); + + let mut any_failed = false; + for unit in units.into_iter() { + let filename = unit.file_name().to_string_lossy().to_string(); + tracing::info!("Testing unit: {}", filename); + let binary = match compile(unit.path()) { + Ok(it) => it, + Err(err) => { + tracing::error!("{}", err); + continue; + } + }; + + let hello_world = Class::read(binary)?; + + let result = JavaBackend + .generate(&lang, &(), &hello_world) + .expect("unable to generate class code") + .0; + + let source = std::fs::read_to_string(unit.path()).unwrap(); + + if source != result { + tracing::error!( + "Unit {} failed\n{}", + filename, + pretty_assertions::StrComparison::new(&source, &result).to_string() + ); + any_failed = true; + } else { + tracing::info!("Unit {} passed", filename); + } + } + assert!(!any_failed, "some tests failed; check the logs"); + + Ok(()) +} diff --git a/tests/units/00_empty.java b/tests/units/00_empty.java new file mode 100644 index 0000000..12ab187 --- /dev/null +++ b/tests/units/00_empty.java @@ -0,0 +1 @@ +class Unit {} diff --git a/tests/units/01_hello_world.java b/tests/units/01_hello_world.java deleted file mode 100644 index f55b114..0000000 --- a/tests/units/01_hello_world.java +++ /dev/null @@ -1,5 +0,0 @@ -class Unit { - public static void main(String[] args) { - System.out.println("Hello, World!"); - } -} \ No newline at end of file diff --git a/tests/units/01_return_lit.java b/tests/units/01_return_lit.java new file mode 100644 index 0000000..a105bb5 --- /dev/null +++ b/tests/units/01_return_lit.java @@ -0,0 +1,5 @@ +class Unit { + public static int literal_return() { + return 20; + } +} diff --git a/tests/units/02_hello_world.java b/tests/units/02_hello_world.java new file mode 100644 index 0000000..3d076eb --- /dev/null +++ b/tests/units/02_hello_world.java @@ -0,0 +1,5 @@ +class Unit { + public static void main(String[] arg_0) { + System.out.println("Hello, World!"); + } +}