Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cranelift: ISLE printer #8263

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+353 −5
Open

cranelift: ISLE printer #8263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29b3581
initial version
mmcloughlin Mar 29, 2024
4d84d7a
parse_without_pos
mmcloughlin Mar 29, 2024
c17bcd7
generated printer tests
mmcloughlin Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions cranelift/isle/isle/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,17 @@ readme = "../README.md"
repository = "https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/isle"
version = "0.107.0"

[[test]]
name = "printer_tests"
required-features = ["printer"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under this implementation, I believe the printer tests would not be run by default in CI.

It would be nice if cargo supported some kind of "default test features" configuration, but as far as I can tell this isn't possible. Nor is there much appetite for adding it according to rust-lang/cargo#2911.

Perhaps if we cared enough we could edit the Github actions workflows to invoke these tests with the right --feature argument. But having poked around the CI config a little, it's not clear how to do that without causing a mess.

Another option is to have the printer tests in a different crate that can depend on this one with the right feature set.

All that said, I'm not sure it's worth it just for these tests, but I wanted to mention it in case you had a simple suggestion for how to run them?


[lints]
workspace = true

[dependencies]
codespan-reporting = { version = "0.11.1", optional = true }
log = { workspace = true, optional = true }
pretty = { version = "0.12", optional = true }

[dev-dependencies]
tempfile = "3"
Expand All @@ -23,3 +28,4 @@ default = []

logging = ["log"]
fancy-errors = ["codespan-reporting"]
printer = ["pretty"]
14 changes: 14 additions & 0 deletions cranelift/isle/isle/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ fn main() {
std::env::var_os("OUT_DIR").expect("The OUT_DIR environment variable must be set"),
);

isle_tests(&out_dir);
isle_printer_tests(&out_dir);
}

fn isle_tests(out_dir: &std::path::PathBuf) {
let mut out = String::new();

emit_tests(&mut out, "isle_examples/pass", "run_pass");
Expand All @@ -19,6 +24,15 @@ fn main() {
std::fs::write(output, out).unwrap();
}

fn isle_printer_tests(out_dir: &std::path::PathBuf) {
let mut out = String::new();

emit_tests(&mut out, "isle_examples/pass", "run_print");

let output = out_dir.join("isle_printer_tests.rs");
std::fs::write(output, out).unwrap();
}

fn emit_tests(out: &mut String, dir_name: &str, runner_func: &str) {
for test_file in std::fs::read_dir(dir_name).unwrap() {
let test_file = test_file.unwrap().file_name().into_string().unwrap();
Expand Down
2 changes: 2 additions & 0 deletions cranelift/isle/isle/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,8 @@ pub mod lexer;
mod log;
pub mod overlap;
pub mod parser;
#[cfg(feature = "printer")]
pub mod printer;
pub mod sema;
pub mod serialize;
pub mod trie_again;
28 changes: 24 additions & 4 deletions cranelift/isle/isle/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,21 @@ pub fn parse(lexer: Lexer) -> Result<Defs> {
parser.parse_defs()
}

/// Parse without positional information. Provided mainly to support testing, to
/// enable equality testing on structure alone.
pub fn parse_without_pos(lexer: Lexer) -> Result<Defs> {
let mut parser = Parser::new(lexer);
parser.disable_pos();
parser.parse_defs()
}

Comment on lines +15 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like this, but I also couldn't think of a better way. Please let me know if you have an alternative suggestion.

  • Having parse_without_pos as a public function is a bit distasteful. Previously I had parse_without_pos in the test binary instead, but that requires you to export Parser itself, and that seems worse.
  • We can't guard parse_without_pos with #[cfg(test)] because then it's not visible to an integration test binary.
  • Potentially the printer tests could be structured as unit tests instead of modeled off the run_tests.rs binary, which would allow use of conditional compilation for test only.
  • We could have an eq_without_pos on AST types instead. However, I assumed that would require a lot of hand- or macro-generated boilerplate.

If parse_without_pos really is the best option, I also wondered if it would be nicer to instead augment the parse method with a Positions enum argument to configure whether positions are populated or dropped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A final option: a helper that maps through all AST nodes and sets positions to e.g. zero? That seems easiest to keep up to date and least likely to be subtly incorrect (vs. hand-written equality predicates), especially given compile errors on new AST node kinds.

/// The ISLE parser.
///
/// Takes in a lexer and creates an AST.
#[derive(Clone, Debug)]
struct Parser<'a> {
lexer: Lexer<'a>,
disable_pos: bool,
}

/// Used during parsing a `(rule ...)` to encapsulate some form that
Expand All @@ -31,7 +40,14 @@ enum IfLetOrExpr {
impl<'a> Parser<'a> {
/// Construct a new parser from the given lexer.
pub fn new(lexer: Lexer<'a>) -> Parser<'a> {
Parser { lexer }
Parser {
lexer,
disable_pos: false,
}
}

pub fn disable_pos(&mut self) {
self.disable_pos = true;
}

fn error(&self, pos: Pos, msg: String) -> Errors {
Expand Down Expand Up @@ -76,9 +92,13 @@ impl<'a> Parser<'a> {
}

fn pos(&self) -> Pos {
self.lexer
.peek()
.map_or_else(|| self.lexer.pos(), |(pos, _)| *pos)
if !self.disable_pos {
self.lexer
.peek()
.map_or_else(|| self.lexer.pos(), |(pos, _)| *pos)
} else {
Pos::default()
}
}

fn is_lparen(&self) -> bool {
Expand Down
Loading
Loading