Some more comments and gated testing infra

teofr · teofr · commit 77c89fff64ad · 2026-02-20T16:23:00.000Z
diff --git a/crates/solidity-v2/outputs/cargo/parser/Cargo.toml b/crates/solidity-v2/outputs/cargo/parser/Cargo.toml
@@ -18,12 +18,15 @@ license = "MIT"
 keywords = ["parser"]
 categories = ["compilers", "parsing", "parser-implementations"]
 
+[features]
+__private_testing_utils = ["anyhow", "infra_utils"]
+
 [build-dependencies]
 lalrpop = { workspace = true }
 
 [dependencies]
-anyhow = { workspace = true }
-infra_utils = { workspace = true }
+anyhow = { workspace = true, optional = true }
+infra_utils = { workspace = true, optional = true }
 lalrpop-util = { workspace = true }
 logos = { workspace = true }
 semver = { workspace = true }
diff --git a/crates/solidity-v2/outputs/cargo/parser/src/parser/mod.rs b/crates/solidity-v2/outputs/cargo/parser/src/parser/mod.rs
@@ -28,6 +28,8 @@ pub trait Parser {
     // TODO(v2): Errors should be something other than `String`
     fn parse(input: &str, version: LanguageVersion) -> Result<Self::NonTerminal, String>;
 
+    // TODO(v2): This is temporary, once the language definition is restricted to only supported versions
+    // it won't be needed
     fn check_version(version: LanguageVersion) -> Result<(), String> {
         if version == LanguageVersion::V0_8_30 {
             Ok(())
diff --git a/crates/solidity-v2/outputs/cargo/parser/src/parser/nodes.rs b/crates/solidity-v2/outputs/cargo/parser/src/parser/nodes.rs
@@ -9,9 +9,9 @@ use slang_solidity_v2_cst::structured_cst::nodes::{
     new_expression_index_access_expression, new_expression_member_access_expression,
     new_function_type, new_function_type_attributes, new_index_access_expression,
     new_member_access_expression, new_type_name_array_type_name, new_type_name_elementary_type,
-    new_type_name_identifier_path, CloseBracket, ConstantKeyword, ElementaryType, Expression,
-    FunctionType, FunctionTypeAttribute, FunctionTypeStruct, Identifier, IdentifierPath,
-    IdentifierPathElement, IndexAccessEnd, OpenBracket, Period, StateVariableAttribute, TypeName,
+    new_type_name_identifier_path, CloseBracket, ElementaryType, Expression, FunctionType,
+    FunctionTypeAttribute, FunctionTypeStruct, Identifier, IdentifierPath, IdentifierPathElement,
+    IndexAccessEnd, OpenBracket, Period, StateVariableAttribute, TypeName,
 };
 
 /// An `IndexAccessPath` represents a path or elementary type followed by
diff --git a/crates/solidity-v2/outputs/cargo/parser/src/parser/temp_testing/mod.rs b/crates/solidity-v2/outputs/cargo/parser/src/parser/temp_testing/mod.rs
@@ -1,249 +1,15 @@
 #[path = "node_checker.generated.rs"]
-pub mod node_checker;
+mod node_checker;
 
-use std::fmt::{Debug, Write};
-use std::path::Path;
-use std::rc::Rc;
-
-use anyhow::Result;
-use infra_utils::codegen::CodegenFileSystem;
-use semver::Version;
-use slang_solidity::cst::{Cursor, Node, TextRange};
-use slang_solidity::parser::ParseOutput;
+#[cfg(feature = "__private_testing_utils")]
+pub mod testing;
+use slang_solidity::cst::{Cursor, TextRange};
 use slang_solidity_v2_common::versions::LanguageVersion;
-use slang_solidity_v2_cst::structured_cst::nodes::{ContractDefinition, Expression, SourceUnit};
+#[cfg(feature = "__private_testing_utils")]
+pub use testing::V2TesterConstructor;
 
-use crate::parser::{ContractDefinitionParser, ExpressionParser, SourceUnitParser};
+use crate::parser::{Parser, SourceUnitParser};
 use crate::temp_testing::node_checker::{NodeChecker, NodeCheckerError};
-use crate::Parser as ParserV2;
-
-/// A Tester for V2 parser that compares against V1 outputs.
-///
-/// It generates snapshots for V2 outputs and diffs against V1 outputs.
-pub trait V2Tester {
-    fn test_next(
-        &mut self,
-        test_dir: &Path,
-        fs: &mut CodegenFileSystem,
-        source_id: &str,
-        source: &str,
-        version: &Version,
-        v1_output: &ParseOutput,
-    ) -> Result<()>;
-}
-
-pub struct V2TesterConstructor;
-
-impl V2TesterConstructor {
-    /// Return a new `V2Tester` instance for the given parser name.
-    ///
-    /// Only `SourceUnit`, `Expression`, and `ContractDefinition` are supported for now.
-    pub fn new_tester(parser_name: &str) -> Box<dyn V2Tester> {
-        match parser_name {
-            "SourceUnit" => Box::new(V2TesterImpl::<SourceUnit, SourceUnitParser>::new()),
-            "Expression" => Box::new(V2TesterImpl::<Expression, ExpressionParser>::new()),
-            "ContractDefinition" => {
-                Box::new(V2TesterImpl::<ContractDefinition, ContractDefinitionParser>::new())
-            }
-            // TODO(v2): We should consolidate all tests to the supported non terminals
-            _ => Box::new(NonSupportedParserTester),
-        }
-    }
-}
-
-/// A dummy tester that does nothing, used for unsupported parsers.
-struct NonSupportedParserTester;
-
-impl V2Tester for NonSupportedParserTester {
-    fn test_next(
-        &mut self,
-        _test_dir: &Path,
-        _fs: &mut CodegenFileSystem,
-        _source_id: &str,
-        _source: &str,
-        _version: &Version,
-        _v1_output: &ParseOutput,
-    ) -> Result<()> {
-        Ok(())
-    }
-}
-
-struct V2TesterImpl<NT, T: ParserV2<NonTerminal = NT>> {
-    last_output: Option<Result<NT, String>>,
-    last_diff: Option<(bool, Option<String>, String)>,
-    phantom: std::marker::PhantomData<T>,
-}
-
-impl<NT, T: ParserV2<NonTerminal = NT>> V2TesterImpl<NT, T> {
-    pub fn new() -> Self {
-        Self {
-            last_output: None,
-            last_diff: None,
-            phantom: std::marker::PhantomData,
-        }
-    }
-}
-
-impl<NT: NodeChecker + Debug + PartialEq, T: ParserV2<NonTerminal = NT>> V2Tester
-    for V2TesterImpl<NT, T>
-{
-    fn test_next(
-        &mut self,
-        test_dir: &Path,
-        fs: &mut CodegenFileSystem,
-        source_id: &str,
-        source: &str,
-        version: &Version,
-        v1_output: &ParseOutput,
-    ) -> Result<()> {
-        // We check version 0.8.30
-        // TODO(v2) check all  versions
-        // _SLANG_V2_PARSER_VERSION_ (keep in sync)
-        if *version != Version::new(0, 8, 30) {
-            return Ok(());
-        }
-
-        // Get the output for v2
-        let lang_version = LanguageVersion::try_from(version.clone())
-            .unwrap_or_else(|_| panic!("Unsupported version: {version}"));
-
-        let v2_output: Result<NT, _> = T::parse(source, lang_version);
-
-        let v2_output = match self.last_output {
-            // Skip this version if it produces the same output.
-            // Note: comparing objects cheaply before expensive serialization.
-            Some(ref last) if last == &v2_output => last,
-            _ => {
-                let status = if v2_output.is_ok() {
-                    "success"
-                } else {
-                    "failure"
-                };
-
-                let snapshot_path = test_dir
-                    .join("v2/generated")
-                    .join(format!("{version}-{status}.txt"));
-
-                let mut s = String::new();
-
-                match &v2_output {
-                    Ok(parsed_checker) => {
-                        // Print AST
-                        writeln!(s, "{parsed_checker:#?}")?;
-                    }
-                    Err(err) => {
-                        // We don't care about the errors for now, we just write them
-                        writeln!(s, "{err:#?}")?;
-                    }
-                }
-
-                fs.write_file_raw(&snapshot_path, s)?;
-                self.last_output.insert(v2_output)
-            }
-        };
-
-        // Now check the diff between V1 and V2
-
-        let diff = Self::diff_report(v1_output, v2_output, source_id, source);
-
-        match &self.last_diff {
-            // Skip if the diff is the same as last time
-            Some(ref last) if last == &diff => (),
-            _ => {
-                let (is_same, should_panic, diff_report) = &diff;
-
-                let diff_path = test_dir.join("diff/generated").join(format!(
-                    "{version}-{diff}.txt",
-                    diff = if *is_same { "same" } else { "diff" }
-                ));
-
-                fs.write_file_raw(&diff_path, diff_report)?;
-
-                if let Some(panic_message) = should_panic {
-                    panic!("{panic_message}");
-                }
-                self.last_diff = Some(diff);
-            }
-        }
-
-        Ok(())
-    }
-}
-
-impl<NT: NodeChecker + Debug + PartialEq, T: ParserV2<NonTerminal = NT>> V2TesterImpl<NT, T> {
-    fn diff_report(
-        v1_output: &ParseOutput,
-        v2_output: &Result<NT, String>,
-        source_id: &str,
-        source: &str,
-    ) -> (bool, Option<String>, String) {
-        match v2_output {
-            Ok(parsed_checker) => {
-                // check V1 validity
-                if v1_output.is_valid() {
-                    // Check for errors
-                    let checked =
-                        parsed_checker.check_node(&Node::Nonterminal(Rc::clone(v1_output.tree())));
-
-                    if checked.is_empty() {
-                        (
-                            true,
-                            None,
-                            "V2 parser produced the same output as V1 output.".to_string(),
-                        )
-                    } else {
-                        let errors = write_errors(&checked, source_id, source);
-
-                        // TODO(v2): This is forced not to panic since some tests in V1 produce different outputs,
-                        // in particular `state_variable_function`
-                        (false, None, errors)
-                    }
-                } else {
-                    // TODO(v2): This is forced not to panic, since V2 has no validation yet, but we
-                    // do want to be aware of these cases, so we write them in the diff report and we can review them later.
-                    (
-                        false,
-                        None,
-                        "V1 Parser: Invalid\nV2 Parser: Valid\n".to_string(),
-                    )
-                }
-            }
-            // TODO(v2): We force this not to panic, since we need lexical context switching to work for some
-            // tests to pass
-            Err(_) if v1_output.is_valid() => (
-                false,
-                None,
-                "V1 Parser: Valid\nV2 Parser: Invalid\n".to_string(),
-            ),
-            Err(_) => {
-                // TODO(v2): Both are invalid, compare the errors
-                (
-                    true,
-                    None,
-                    "Both V1 and V2 produced invalid output.\n".to_string(),
-                )
-            }
-        }
-    }
-}
-
-fn write_errors(errors: &Vec<NodeCheckerError>, source_id: &str, source: &str) -> String {
-    if errors.is_empty() {
-        return String::new();
-    }
-
-    let mut s = String::new();
-    writeln!(s, "Errors: # {count} total", count = errors.len()).unwrap();
-
-    for error in errors {
-        writeln!(s, "  - >").unwrap();
-        for line in slang_solidity::diagnostic::render(error, source_id, source, false).lines() {
-            writeln!(s, "    {line}").unwrap();
-        }
-    }
-
-    s
-}
 
 pub fn compare_with_v1_cursor(source: &str, root_cursor: &Cursor) -> Vec<NodeCheckerError> {
     let v2_output = SourceUnitParser::parse(source, LanguageVersion::V0_8_30);
diff --git a/crates/solidity-v2/outputs/cargo/parser/src/parser/temp_testing/testing.rs b/crates/solidity-v2/outputs/cargo/parser/src/parser/temp_testing/testing.rs
diff --git a/crates/solidity/outputs/cargo/tests/Cargo.toml b/crates/solidity/outputs/cargo/tests/Cargo.toml
