diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 7b1becdd2..c08a2ea8e 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -271,6 +271,7 @@ checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" name = "ruby-rbs" version = "0.3.0" dependencies = [ + "indexmap", "ruby-rbs-sys", "serde", "serde_yaml", diff --git a/rust/ruby-rbs/Cargo.toml b/rust/ruby-rbs/Cargo.toml index 973162288..d09b9a86b 100644 --- a/rust/ruby-rbs/Cargo.toml +++ b/rust/ruby-rbs/Cargo.toml @@ -17,6 +17,7 @@ include = [ [dependencies] ruby-rbs-sys = { version = "0.3", path = "../ruby-rbs-sys" } xxhash-rust = { version = "0.8", features = ["xxh3"] } +indexmap = "2" [build-dependencies] serde = { version = "1.0", features = ["derive"] } diff --git a/rust/ruby-rbs/src/ast/declarations.rs b/rust/ruby-rbs/src/ast/declarations.rs index 5e392bc32..dcca1798f 100644 --- a/rust/ruby-rbs/src/ast/declarations.rs +++ b/rust/ruby-rbs/src/ast/declarations.rs @@ -3,7 +3,7 @@ use crate::ast::comment::Comment; use crate::ast::location::{ AliasDeclarationLocation, ClassDeclarationLocation, ClassSuperLocation, ConstantDeclarationLocation, GlobalDeclarationLocation, InterfaceDeclarationLocation, - ModuleDeclarationLocation, ModuleSelfLocation, TypeAliasDeclarationLocation, + LocationRange, ModuleDeclarationLocation, ModuleSelfLocation, TypeAliasDeclarationLocation, }; use crate::ast::members::Member; use crate::ast::type_param::TypeParam; @@ -22,6 +22,22 @@ pub enum Declaration { ModuleAlias(ModuleAliasDeclaration), } +impl Declaration { + #[must_use] + pub(crate) fn location_range(&self) -> Option { + match self { + Declaration::Class(d) => d.location.as_ref().map(|l| l.range), + Declaration::Module(d) => d.location.as_ref().map(|l| l.range), + Declaration::Interface(d) => d.location.as_ref().map(|l| l.range), + Declaration::Constant(d) => d.location.as_ref().map(|l| l.range), + Declaration::Global(d) => d.location.as_ref().map(|l| l.range), + Declaration::TypeAlias(d) => d.location.as_ref().map(|l| l.range), + Declaration::ClassAlias(d) => d.location.as_ref().map(|l| l.range), + Declaration::ModuleAlias(d) => d.location.as_ref().map(|l| l.range), + } + } +} + #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub enum ClassMember { Member(Member), diff --git a/rust/ruby-rbs/src/environment/decl.rs b/rust/ruby-rbs/src/environment/decl.rs new file mode 100644 index 000000000..820b8a3a4 --- /dev/null +++ b/rust/ruby-rbs/src/environment/decl.rs @@ -0,0 +1,23 @@ +use crate::ast::Declaration; + +use super::Environment; + +/// Only valid against the [`Environment`] that issued it. +/// +/// Only top-level declarations are registered for now, so an id is just the +/// declaration's position in `sources`. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub struct DeclId { + pub(super) source: u32, + pub(super) index: u32, +} + +impl Environment { + /// # Panics + /// + /// May panic if `id` was issued by a different `Environment`. + #[must_use] + pub fn decl(&self, id: DeclId) -> &Declaration { + &self.sources[id.source as usize].declarations[id.index as usize] + } +} diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs new file mode 100644 index 000000000..ee307a589 --- /dev/null +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -0,0 +1,29 @@ +use crate::ids::TypeName; + +use super::{DeclId, Environment}; + +/// `name` repeats the table key, as in Ruby, so iterating entries alone +/// yields their names. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +#[non_exhaustive] +pub struct SingleEntry { + pub name: TypeName, + pub decl: DeclId, +} + +impl Environment { + #[must_use] + pub fn is_interface_name(&self, name: TypeName) -> bool { + self.interface_decls.contains_key(&name) + } + + #[must_use] + pub fn interface_entry(&self, name: TypeName) -> Option<&SingleEntry> { + self.interface_decls.get(&name) + } + + /// In insertion order, like iterating Ruby's `interface_decls`. + pub fn interface_entries(&self) -> impl Iterator { + self.interface_decls.values() + } +} diff --git a/rust/ruby-rbs/src/environment/error.rs b/rust/ruby-rbs/src/environment/error.rs new file mode 100644 index 000000000..81d6f9f78 --- /dev/null +++ b/rust/ruby-rbs/src/environment/error.rs @@ -0,0 +1,52 @@ +use std::fmt; +use std::path::PathBuf; + +use crate::ast::location::LocationRange; + +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct DuplicatedDecl { + pub path: PathBuf, + pub location: Option, +} + +/// Owns rendered data instead of `DeclId`s because it outlives the +/// `Environment` (`Environment::from_loader` drops it on error). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DuplicatedDeclarationError { + name: String, + decls: Vec, +} + +impl DuplicatedDeclarationError { + pub(crate) fn new(name: String, inserted: DuplicatedDecl, existing: DuplicatedDecl) -> Self { + Self { + name, + decls: vec![inserted, existing], + } + } + + #[must_use] + pub fn name(&self) -> &str { + &self.name + } + + /// The newly inserted declaration first, then the existing ones, as in + /// Ruby. Always at least two; a slice because Ruby's error takes + /// `*decls`, so class entries will report every reopening. + #[must_use] + pub fn decls(&self) -> &[DuplicatedDecl] { + &self.decls + } +} + +impl fmt::Display for DuplicatedDeclarationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // Ruby's nil-location fallback: line:col needs the source text, which + // `Source` does not keep yet, and path alone matches no Ruby format. + // TODO: render `decls.last()` as `path:line:col...line:col`. + write!(f, "*:*:*...*:*: Duplicated declaration: {}", self.name) + } +} + +impl std::error::Error for DuplicatedDeclarationError {} diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs new file mode 100644 index 000000000..cf6b8b13d --- /dev/null +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -0,0 +1,196 @@ +use crate::ast::Declaration; +use crate::ids::TypeName; + +use super::entry::SingleEntry; +use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; +use super::source::Source; +use super::{DeclId, Environment}; + +impl Environment { + /// Like Ruby, the source and the entries inserted before a duplication + /// error are not rolled back. The rejected declaration itself is not + /// registered. + pub(crate) fn add_source(&mut self, source: Source) -> Result<(), DuplicatedDeclarationError> { + let source_index = u32::try_from(self.sources.len()).expect("too many sources"); + let decl_count = + u32::try_from(source.declarations.len()).expect("too many declarations in one source"); + self.sources.push(source); + + for index in 0..decl_count { + self.insert_decl(DeclId { + source: source_index, + index, + })?; + } + Ok(()) + } + + fn insert_decl(&mut self, id: DeclId) -> Result<(), DuplicatedDeclarationError> { + let decl_name = match self.decl(id) { + Declaration::Interface(d) => d.name, + // TODO: register the other declaration kinds. + _ => return Ok(()), + }; + + let name = self.interners.type_names.to_absolute(decl_name); + if let Some(existing) = self.interface_decls.get(&name) { + return Err(self.duplicated_declaration(name, id, existing.decl)); + } + self.interface_decls + .insert(name, SingleEntry { name, decl: id }); + + Ok(()) + } + + fn duplicated_declaration( + &self, + name: TypeName, + inserted: DeclId, + existing: DeclId, + ) -> DuplicatedDeclarationError { + let interners = &self.interners; + DuplicatedDeclarationError::new( + interners.type_names.display(name, &interners.strings), + self.duplicated_decl(inserted), + self.duplicated_decl(existing), + ) + } + + fn duplicated_decl(&self, id: DeclId) -> DuplicatedDecl { + DuplicatedDecl { + path: self.sources[id.source as usize].path.clone(), + location: self.decl(id).location_range(), + } + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use crate::ast::{AstConverter, Declaration}; + use crate::environment::{DuplicatedDeclarationError, Environment, Source, SourceKind}; + use crate::ids::TypeName; + use crate::node; + + fn parse_decls(env: &mut Environment, src: &str) -> Vec { + let signature = node::parse(src).expect("valid RBS source"); + let interners = env.interners_mut(); + let mut converter = AstConverter::new(&mut interners.strings, &mut interners.type_names); + signature + .declarations() + .iter() + .map(|node| converter.convert_declaration(&node)) + .collect() + } + + fn add_rbs_source( + env: &mut Environment, + path: &str, + declarations: Vec, + ) -> Result<(), DuplicatedDeclarationError> { + env.add_source(Source { + path: PathBuf::from(path), + directives: Vec::new(), + declarations, + kind: SourceKind::Dir { + path: PathBuf::from("."), + }, + }) + } + + fn type_name(env: &mut Environment, name: &str) -> TypeName { + let interners = env.interners_mut(); + interners.type_names.parse(&mut interners.strings, name) + } + + // environment_test.rb:169 + #[test] + fn interface_twice_duplication_error() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "interface _I\nend\ninterface _I\nend\n"); + + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + assert_eq!(err.name(), "::_I"); + let [inserted, existing] = err.decls() else { + panic!("expected two decls, got {:?}", err.decls()); + }; + assert_eq!(inserted.path, PathBuf::from("b.rbs")); + assert_eq!(inserted.location, decls[1].location_range()); + assert_eq!(existing.path, PathBuf::from("a.rbs")); + assert_eq!(existing.location, decls[0].location_range()); + assert_eq!(err.to_string(), "*:*:*...*:*: Duplicated declaration: ::_I"); + + // The rejected declaration is not registered. + assert_eq!(env.interface_entries().count(), 1); + let name = type_name(&mut env, "::_I"); + let entry = env.interface_entry(name).unwrap(); + assert_eq!(env.decl(entry.decl), &decls[0]); + } + + #[test] + fn absolute_and_relative_names_collide() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "interface _I\nend\ninterface ::_I\nend\n"); + + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + assert_eq!(err.name(), "::_I"); + } + + #[test] + fn duplication_within_a_source_keeps_earlier_entries() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "interface _A\nend\ninterface _I\nend\ninterface _I\nend\ninterface _B\nend\n", + ); + + let err = add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap_err(); + + let [inserted, existing] = err.decls() else { + panic!("expected two decls, got {:?}", err.decls()); + }; + assert_eq!(inserted.path, PathBuf::from("a.rbs")); + assert_eq!(inserted.location, decls[2].location_range()); + assert_eq!(existing.path, PathBuf::from("a.rbs")); + assert_eq!(existing.location, decls[1].location_range()); + + // Not rolled back: the source and the entries before the duplicate stay. + assert_eq!(env.sources().len(), 1); + let a = type_name(&mut env, "::_A"); + let i = type_name(&mut env, "::_I"); + let b = type_name(&mut env, "::_B"); + let names: Vec<_> = env.interface_entries().map(|e| e.name).collect(); + assert_eq!(names, [a, i]); + assert_eq!(env.decl(env.interface_entry(i).unwrap().decl), &decls[1]); + // Declarations after the duplicate are never reached. + assert!(!env.is_interface_name(b)); + } + + #[test] + fn distinct_interfaces_are_registered_in_order() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "interface _A\nend\ninterface _B\nend\n"); + + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap(); + + let a = type_name(&mut env, "::_A"); + let b = type_name(&mut env, "::_B"); + let unknown = type_name(&mut env, "::_C"); + assert!(env.is_interface_name(a)); + assert!(env.is_interface_name(b)); + assert!(!env.is_interface_name(unknown)); + assert!(env.interface_entry(unknown).is_none()); + + let names: Vec<_> = env.interface_entries().map(|e| e.name).collect(); + assert_eq!(names, [a, b]); + for (entry, decl) in env.interface_entries().zip(&decls) { + assert_eq!(env.decl(entry.decl), decl); + } + } +} diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index fa2cedf10..12525c5f7 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -1,7 +1,15 @@ +mod decl; +mod entry; +mod error; +mod insert; pub mod source; +pub use decl::DeclId; +pub use entry::SingleEntry; +pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; +use crate::ids::{IdIndexMap, TypeNameTag}; use crate::interners::Interners; use crate::loader::{EnvironmentLoader, LoadError}; @@ -12,6 +20,7 @@ use crate::loader::{EnvironmentLoader, LoadError}; pub struct Environment { interners: Interners, sources: Vec, + interface_decls: IdIndexMap, } impl Environment { @@ -32,10 +41,6 @@ impl Environment { &mut self.interners } - pub(crate) fn add_source(&mut self, source: Source) { - self.sources.push(source); - } - pub fn from_loader(loader: &EnvironmentLoader) -> Result { let mut env = Environment::new(); loader.load(&mut env)?; diff --git a/rust/ruby-rbs/src/ids.rs b/rust/ruby-rbs/src/ids.rs index a72678b4e..50a6aaf68 100644 --- a/rust/ruby-rbs/src/ids.rs +++ b/rust/ruby-rbs/src/ids.rs @@ -21,7 +21,7 @@ //! ``` use std::cmp::Ordering; -use std::hash::{Hash, Hasher}; +use std::hash::{BuildHasherDefault, Hash, Hasher}; use std::marker::PhantomData; use std::num::NonZeroU64; @@ -132,6 +132,32 @@ pub enum TypeNameTag {} /// the parent type name's hash and the last segment's hash. pub type TypeName = Id; +/// `Id` is already an xxh3 hash, so this passes it through instead of +/// re-hashing with `SipHash`. +#[derive(Default)] +pub(crate) struct IdHasher(u64); + +impl Hasher for IdHasher { + fn finish(&self) -> u64 { + self.0 + } + + fn write_u64(&mut self, n: u64) { + self.0 = n; + } + + fn write(&mut self, _bytes: &[u8]) { + unreachable!("IdHasher only hashes Id values") + } +} + +/// Insertion-ordered like Ruby's `Hash`; `Environment` consumers depend on +/// that order for deterministic results. +/// +/// Keyed by `Id` (named by its tag, e.g. `IdIndexMap`) so +/// that `IdHasher` only ever sees the single `write_u64` of an `Id`. +pub(crate) type IdIndexMap = indexmap::IndexMap, V, BuildHasherDefault>; + #[cfg(test)] mod tests { use super::*; diff --git a/rust/ruby-rbs/src/loader/mod.rs b/rust/ruby-rbs/src/loader/mod.rs index c436f8e5c..4fde53e4d 100644 --- a/rust/ruby-rbs/src/loader/mod.rs +++ b/rust/ruby-rbs/src/loader/mod.rs @@ -4,7 +4,7 @@ use std::io; use std::path::{Path, PathBuf}; use crate::ast::AstConverter; -use crate::environment::{Environment, Source, SourceKind}; +use crate::environment::{DuplicatedDeclarationError, Environment, Source, SourceKind}; use crate::file_finder; use crate::interners::Interners; use crate::node; @@ -14,6 +14,7 @@ use crate::node; pub enum LoadError { Io { path: PathBuf, source: io::Error }, Parse { path: PathBuf, message: String }, + DuplicatedDeclaration(DuplicatedDeclarationError), } impl fmt::Display for LoadError { @@ -25,6 +26,7 @@ impl fmt::Display for LoadError { LoadError::Parse { path, message } => { write!(f, "Syntax error in {}: {}", path.display(), message) } + LoadError::DuplicatedDeclaration(err) => fmt::Display::fmt(err, f), } } } @@ -33,7 +35,10 @@ impl std::error::Error for LoadError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { LoadError::Io { source, .. } => Some(source), - _ => None, + // Transparent: `Display` already delegates to `err`, so returning + // it here would print the same message twice in an error chain. + LoadError::DuplicatedDeclaration(err) => std::error::Error::source(err), + LoadError::Parse { .. } => None, } } } @@ -82,7 +87,9 @@ impl EnvironmentLoader { /// Returns what this call read, in the order it read it, so a caller /// appending to a non-empty `env` still learns what it added. On `Err` the /// sources read before the failure are already in `env`, same as the Ruby - /// implementation adding sources as it walks the directories. + /// implementation adding sources as it walks the directories. On + /// `LoadError::DuplicatedDeclaration` that includes the failing source and + /// the declarations registered from it before the duplicate. pub fn load(&self, env: &mut Environment) -> Result, LoadError> { let mut loaded = Vec::new(); let mut seen_files: HashSet = HashSet::new(); @@ -100,7 +107,8 @@ impl EnvironmentLoader { continue; } let source = parse_one(&path, &kind, env.interners_mut())?; - env.add_source(source); + env.add_source(source) + .map_err(LoadError::DuplicatedDeclaration)?; loaded.push(LoadedFile { path, kind: kind.clone(), diff --git a/rust/ruby-rbs/tests/loader.rs b/rust/ruby-rbs/tests/loader.rs index 3246dbec1..33378a674 100644 --- a/rust/ruby-rbs/tests/loader.rs +++ b/rust/ruby-rbs/tests/loader.rs @@ -127,6 +127,26 @@ fn parse_errors_carry_the_file_path() { )); } +#[test] +fn duplicated_declarations_are_reported_as_load_errors() { + let dir = tree(&[ + ("a.rbs", "interface _I\nend\n"), + ("b.rbs", "interface _I\nend\n"), + ]); + + let loader = EnvironmentLoader::new(None).add_dir(dir.path().to_path_buf()); + let Err(error) = Environment::from_loader(&loader) else { + panic!("expected a duplicated-declaration error"); + }; + + let LoadError::DuplicatedDeclaration(inner) = &error else { + panic!("expected LoadError::DuplicatedDeclaration, got {error:?}"); + }; + assert_eq!(error.to_string(), inner.to_string()); + // Transparent wrapper: the message is not repeated as a source. + assert!(std::error::Error::source(&error).is_none()); +} + #[test] fn loaded_sources_carry_converted_declarations_and_directives() { let dir = tree(&[(