From b6cbb4fc8a98fa9b2250f38b156bd5b6f1037c12 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Sun, 27 Sep 2026 23:24:59 +0900 Subject: [PATCH] Detect duplicated interface declarations in Environment Add DeclId, SingleEntry, and DuplicatedDeclarationError types, and insert.rs to track interface declarations per source and report duplicates as load errors instead of silently overwriting. Co-Authored-By: Claude Sonnet 5 --- rust/Cargo.lock | 1 + rust/ruby-rbs/Cargo.toml | 1 + rust/ruby-rbs/src/ast/declarations.rs | 18 ++- rust/ruby-rbs/src/environment/decl.rs | 23 +++ rust/ruby-rbs/src/environment/entry.rs | 29 ++++ rust/ruby-rbs/src/environment/error.rs | 52 +++++++ rust/ruby-rbs/src/environment/insert.rs | 196 ++++++++++++++++++++++++ rust/ruby-rbs/src/environment/mod.rs | 13 +- rust/ruby-rbs/src/ids.rs | 28 +++- rust/ruby-rbs/src/loader/mod.rs | 16 +- rust/ruby-rbs/tests/loader.rs | 20 +++ 11 files changed, 387 insertions(+), 10 deletions(-) create mode 100644 rust/ruby-rbs/src/environment/decl.rs create mode 100644 rust/ruby-rbs/src/environment/entry.rs create mode 100644 rust/ruby-rbs/src/environment/error.rs create mode 100644 rust/ruby-rbs/src/environment/insert.rs diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 7b1becdd29..c08a2ea8e7 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 973162288b..d09b9a86b5 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 5e392bc321..dcca1798f8 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 0000000000..820b8a3a4c --- /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 0000000000..ee307a5892 --- /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 0000000000..81d6f9f789 --- /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 0000000000..cf6b8b13d1 --- /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 fa2cedf103..12525c5f77 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 a72678b4e9..50a6aaf688 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 c436f8e5cd..4fde53e4d9 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 3246dbec11..33378a6749 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(&[(