From c1cf0459fdf4ac428c981530630fec845a1ab733 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 01/10] 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 6646c2536f..425c90d98c 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -278,6 +278,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(&[( From 532544604ebff7bb7ba5d37298adb66c8f8189fe Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:14:08 +0900 Subject: [PATCH 02/10] Detect duplicated type alias declarations in Environment Register type aliases in their own table so duplicates are reported as load errors, as is already done for interfaces. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 14 ++++ rust/ruby-rbs/src/environment/insert.rs | 101 ++++++++++++++++++------ rust/ruby-rbs/src/environment/mod.rs | 1 + 3 files changed, 92 insertions(+), 24 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index ee307a5892..d19300a938 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -26,4 +26,18 @@ impl Environment { pub fn interface_entries(&self) -> impl Iterator { self.interface_decls.values() } + + #[must_use] + pub fn is_type_alias_name(&self, name: TypeName) -> bool { + self.type_alias_decls.contains_key(&name) + } + + #[must_use] + pub fn type_alias_entry(&self, name: TypeName) -> Option<&SingleEntry> { + self.type_alias_decls.get(&name) + } + + pub fn type_alias_entries(&self) -> impl Iterator { + self.type_alias_decls.values() + } } diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index cf6b8b13d1..c165c72891 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -1,11 +1,13 @@ use crate::ast::Declaration; -use crate::ids::TypeName; +use crate::ids::{IdIndexMap, TypeName, TypeNameTag}; use super::entry::SingleEntry; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; +type SingleTable = IdIndexMap; + impl Environment { /// Like Ruby, the source and the entries inserted before a duplication /// error are not rolled back. The rejected declaration itself is not @@ -26,18 +28,33 @@ impl Environment { } fn insert_decl(&mut self, id: DeclId) -> Result<(), DuplicatedDeclarationError> { - let decl_name = match self.decl(id) { - Declaration::Interface(d) => d.name, + match self.decl(id) { + Declaration::Interface(d) => { + let name = d.name; + self.insert_single(|env| &mut env.interface_decls, name, id) + } + Declaration::TypeAlias(d) => { + let name = d.name; + self.insert_single(|env| &mut env.type_alias_decls, name, id) + } // TODO: register the other declaration kinds. - _ => return Ok(()), - }; + _ => 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)); + fn insert_single( + &mut self, + table: fn(&mut Self) -> &mut SingleTable, + name: TypeName, + id: DeclId, + ) -> Result<(), DuplicatedDeclarationError> { + let name = self.interners.type_names.to_absolute(name); + let decls = table(self); + if let Some(entry) = decls.get(&name) { + let existing = entry.decl; + return Err(self.duplicated_declaration(name, id, existing)); } - self.interface_decls - .insert(name, SingleEntry { name, decl: id }); + decls.insert(name, SingleEntry { name, decl: id }); Ok(()) } @@ -104,6 +121,20 @@ mod tests { interners.type_names.parse(&mut interners.strings, name) } + fn assert_duplicate( + err: &DuplicatedDeclarationError, + inserted: (&str, &Declaration), + existing: (&str, &Declaration), + ) { + let [got_inserted, got_existing] = err.decls() else { + panic!("expected two decls, got {:?}", err.decls()); + }; + assert_eq!(got_inserted.path, PathBuf::from(inserted.0)); + assert_eq!(got_inserted.location, inserted.1.location_range()); + assert_eq!(got_existing.path, PathBuf::from(existing.0)); + assert_eq!(got_existing.location, existing.1.location_range()); + } + // environment_test.rb:169 #[test] fn interface_twice_duplication_error() { @@ -114,13 +145,7 @@ mod tests { 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_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); assert_eq!(err.to_string(), "*:*:*...*:*: Duplicated declaration: ::_I"); // The rejected declaration is not registered. @@ -151,13 +176,7 @@ mod tests { 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()); + assert_duplicate(&err, ("a.rbs", &decls[2]), ("a.rbs", &decls[1])); // Not rolled back: the source and the entries before the duplicate stay. assert_eq!(env.sources().len(), 1); @@ -193,4 +212,38 @@ mod tests { assert_eq!(env.decl(entry.decl), decl); } } + + #[test] + fn type_alias_twice_duplication_error() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "type foo = String\ntype foo = Integer\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(), "::foo"); + assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + + assert_eq!(env.type_alias_entries().count(), 1); + let name = type_name(&mut env, "::foo"); + let entry = env.type_alias_entry(name).unwrap(); + assert_eq!(env.decl(entry.decl), &decls[0]); + } + + #[test] + fn interface_and_type_alias_tables_are_independent() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "interface _I\nend\ntype i = String\n"); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let interface = type_name(&mut env, "::_I"); + let alias = type_name(&mut env, "::i"); + assert!(env.is_interface_name(interface)); + assert!(!env.is_type_alias_name(interface)); + assert!(env.is_type_alias_name(alias)); + assert!(!env.is_interface_name(alias)); + assert_eq!(env.interface_entries().count(), 1); + assert_eq!(env.type_alias_entries().count(), 1); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index 12525c5f77..eb89f38063 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -21,6 +21,7 @@ pub struct Environment { interners: Interners, sources: Vec, interface_decls: IdIndexMap, + type_alias_decls: IdIndexMap, } impl Environment { From 40a1d4c004b505e09608270425b8e3ad524598eb Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:57:48 +0900 Subject: [PATCH 03/10] Detect duplicated global declarations in Environment Report them as load errors instead of silently overwriting, as is already done for interfaces and type aliases. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 22 +++++- rust/ruby-rbs/src/environment/insert.rs | 101 ++++++++++++++++++++---- rust/ruby-rbs/src/environment/mod.rs | 3 +- 3 files changed, 108 insertions(+), 18 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index d19300a938..2280b93aea 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -1,4 +1,4 @@ -use crate::ids::TypeName; +use crate::ids::{SymbolId, TypeName}; use super::{DeclId, Environment}; @@ -6,8 +6,8 @@ use super::{DeclId, Environment}; /// yields their names. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] #[non_exhaustive] -pub struct SingleEntry { - pub name: TypeName, +pub struct SingleEntry { + pub name: N, pub decl: DeclId, } @@ -41,3 +41,19 @@ impl Environment { self.type_alias_decls.values() } } + +impl Environment { + #[must_use] + pub fn is_global_name(&self, name: SymbolId) -> bool { + self.global_decls.contains_key(&name) + } + + #[must_use] + pub fn global_entry(&self, name: SymbolId) -> Option<&SingleEntry> { + self.global_decls.get(&name) + } + + pub fn global_entries(&self) -> impl Iterator> { + self.global_decls.values() + } +} diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index c165c72891..6185cb4f46 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -1,12 +1,13 @@ use crate::ast::Declaration; -use crate::ids::{IdIndexMap, TypeName, TypeNameTag}; +use crate::ids::{Id, IdIndexMap}; use super::entry::SingleEntry; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; +use crate::interners::Interners; -type SingleTable = IdIndexMap; +type SingleTable = IdIndexMap>>; impl Environment { /// Like Ruby, the source and the entries inserted before a duplication @@ -30,28 +31,48 @@ impl Environment { fn insert_decl(&mut self, id: DeclId) -> Result<(), DuplicatedDeclarationError> { match self.decl(id) { Declaration::Interface(d) => { - let name = d.name; - self.insert_single(|env| &mut env.interface_decls, name, id) + let name = self.interners.type_names.to_absolute(d.name); + self.insert_single( + |env| &mut env.interface_decls, + name, + id, + |i, n| i.type_names.display(n, &i.strings), + ) } Declaration::TypeAlias(d) => { + let name = self.interners.type_names.to_absolute(d.name); + self.insert_single( + |env| &mut env.type_alias_decls, + name, + id, + |i, n| i.type_names.display(n, &i.strings), + ) + } + Declaration::Global(d) => { let name = d.name; - self.insert_single(|env| &mut env.type_alias_decls, name, id) + self.insert_single( + |env| &mut env.global_decls, + name, + id, + |i, n| i.strings.resolve(n).to_owned(), + ) } // TODO: register the other declaration kinds. _ => Ok(()), } } - fn insert_single( + fn insert_single( &mut self, - table: fn(&mut Self) -> &mut SingleTable, - name: TypeName, + table: fn(&mut Self) -> &mut SingleTable, + name: Id, id: DeclId, + render: fn(&Interners, Id) -> String, ) -> Result<(), DuplicatedDeclarationError> { - let name = self.interners.type_names.to_absolute(name); let decls = table(self); if let Some(entry) = decls.get(&name) { let existing = entry.decl; + let name = render(&self.interners, name); return Err(self.duplicated_declaration(name, id, existing)); } decls.insert(name, SingleEntry { name, decl: id }); @@ -61,13 +82,12 @@ impl Environment { fn duplicated_declaration( &self, - name: TypeName, + name: String, inserted: DeclId, existing: DeclId, ) -> DuplicatedDeclarationError { - let interners = &self.interners; DuplicatedDeclarationError::new( - interners.type_names.display(name, &interners.strings), + name, self.duplicated_decl(inserted), self.duplicated_decl(existing), ) @@ -87,7 +107,7 @@ mod tests { use crate::ast::{AstConverter, Declaration}; use crate::environment::{DuplicatedDeclarationError, Environment, Source, SourceKind}; - use crate::ids::TypeName; + use crate::ids::{SymbolId, TypeName}; use crate::node; fn parse_decls(env: &mut Environment, src: &str) -> Vec { @@ -121,6 +141,10 @@ mod tests { interners.type_names.parse(&mut interners.strings, name) } + fn intern(env: &mut Environment, name: &str) -> SymbolId { + env.interners_mut().strings.intern(name) + } + fn assert_duplicate( err: &DuplicatedDeclarationError, inserted: (&str, &Declaration), @@ -233,7 +257,7 @@ mod tests { #[test] fn interface_and_type_alias_tables_are_independent() { let mut env = Environment::new(); - let decls = parse_decls(&mut env, "interface _I\nend\ntype i = String\n"); + let decls = parse_decls(&mut env, "interface _I\nend\ntype i = String\n$g: String\n"); add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); @@ -245,5 +269,54 @@ mod tests { assert!(!env.is_interface_name(alias)); assert_eq!(env.interface_entries().count(), 1); assert_eq!(env.type_alias_entries().count(), 1); + assert_eq!(env.global_entries().count(), 1); + } + + #[test] + fn insert_global() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "$VERSION: String\n"); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let name = intern(&mut env, "$VERSION"); + assert!(env.is_global_name(name)); + let entry = env.global_entry(name).unwrap(); + assert_eq!(entry.name, name); + assert_eq!(env.decl(entry.decl), &decls[0]); + + let other = intern(&mut env, "$OTHER"); + assert!(!env.is_global_name(other)); + assert!(env.global_entry(other).is_none()); + } + + #[test] + fn global_twice_duplication_error() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "$VERSION: String\n$VERSION: Integer\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(), "$VERSION"); + assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + + assert_eq!(env.global_entries().count(), 1); + let name = intern(&mut env, "$VERSION"); + let entry = env.global_entry(name).unwrap(); + assert_eq!(env.decl(entry.decl), &decls[0]); + } + + #[test] + fn globals_are_registered_in_order() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "$B: String\n$A: String\n"); + + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let a = intern(&mut env, "$A"); + let b = intern(&mut env, "$B"); + let names: Vec<_> = env.global_entries().map(|e| e.name).collect(); + assert_eq!(names, [b, a]); } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index eb89f38063..2482676d0f 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -9,7 +9,7 @@ pub use entry::SingleEntry; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; -use crate::ids::{IdIndexMap, TypeNameTag}; +use crate::ids::{IdIndexMap, SymbolId, SymbolTag, TypeNameTag}; use crate::interners::Interners; use crate::loader::{EnvironmentLoader, LoadError}; @@ -22,6 +22,7 @@ pub struct Environment { sources: Vec, interface_decls: IdIndexMap, type_alias_decls: IdIndexMap, + global_decls: IdIndexMap>, } impl Environment { From 73902f1f4aacfa6de2fb11111a97b61a8e854b5a Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:28:15 +0900 Subject: [PATCH 04/10] Register modules and their nested declarations in Environment Declarations inside a module were never reached, so a duplicate such as two `interface Foo::_I` went undetected. Register modules as one entry per name, recurse into their bodies with the enclosing namespace, and record each declaration's context. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/ast/declarations.rs | 20 ++ rust/ruby-rbs/src/environment/decl.rs | 78 +++++- rust/ruby-rbs/src/environment/entry.rs | 60 +++++ rust/ruby-rbs/src/environment/insert.rs | 343 +++++++++++++++++++++--- rust/ruby-rbs/src/environment/mod.rs | 11 +- 5 files changed, 462 insertions(+), 50 deletions(-) diff --git a/rust/ruby-rbs/src/ast/declarations.rs b/rust/ruby-rbs/src/ast/declarations.rs index dcca1798f8..c35afda218 100644 --- a/rust/ruby-rbs/src/ast/declarations.rs +++ b/rust/ruby-rbs/src/ast/declarations.rs @@ -44,12 +44,32 @@ pub enum ClassMember { Declaration(Declaration), } +impl ClassMember { + #[must_use] + pub fn as_declaration(&self) -> Option<&Declaration> { + match self { + Self::Declaration(d) => Some(d), + Self::Member(_) => None, + } + } +} + #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub enum ModuleMember { Member(Member), Declaration(Declaration), } +impl ModuleMember { + #[must_use] + pub fn as_declaration(&self) -> Option<&Declaration> { + match self { + Self::Declaration(d) => Some(d), + Self::Member(_) => None, + } + } +} + #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub struct ClassSuper { pub name: TypeName, diff --git a/rust/ruby-rbs/src/environment/decl.rs b/rust/ruby-rbs/src/environment/decl.rs index 820b8a3a4c..c03f67c79e 100644 --- a/rust/ruby-rbs/src/environment/decl.rs +++ b/rust/ruby-rbs/src/environment/decl.rs @@ -1,15 +1,37 @@ use crate::ast::Declaration; +use crate::ids::TypeName; 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 struct DeclId(u32); + +#[derive(Copy, Clone, Debug)] +pub(super) struct DeclNode { pub(super) source: u32, + pub(super) parent: Option, pub(super) index: u32, + pub(super) context: Context, +} + +/// Ruby's `Resolver::context` (a cons list of `[context, name]`), interned by +/// its chain of names. Equal across reopenings of the same module. Unlike +/// Ruby's, the chain holds absolute names rather than the written ones. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub struct Context(Option); + +impl Context { + #[must_use] + pub const fn root() -> Self { + Self(None) + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub(super) struct ContextNode { + parent: Context, + name: TypeName, } impl Environment { @@ -18,6 +40,52 @@ impl Environment { /// 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] + self.member_decl(self.node(id)) + } + + /// # Panics + /// + /// May panic if `id` was issued by a different `Environment`. + #[must_use] + pub fn context_of(&self, id: DeclId) -> Context { + self.node(id).context + } + + /// Innermost first. + /// + /// # Panics + /// + /// May panic if `context` was issued by a different `Environment`. + pub fn context_names(&self, context: Context) -> impl Iterator + '_ { + let node = move |i: u32| &self.contexts[i as usize]; + std::iter::successors(context.0, move |&i| node(i).parent.0).map(move |i| node(i).name) + } + + pub(super) fn intern_context(&mut self, parent: Context, name: TypeName) -> Context { + let (index, _) = self.contexts.insert_full(ContextNode { parent, name }); + Context(Some(u32::try_from(index).expect("too many contexts"))) + } + + pub(super) fn node(&self, id: DeclId) -> &DeclNode { + &self.decl_nodes[id.0 as usize] + } + + pub(super) fn push_node(&mut self, node: DeclNode) -> DeclId { + let id = DeclId(u32::try_from(self.decl_nodes.len()).expect("too many declarations")); + self.decl_nodes.push(node); + id + } + + pub(super) fn member_decl(&self, node: &DeclNode) -> &Declaration { + let index = node.index as usize; + let Some(parent) = node.parent else { + return &self.sources[node.source as usize].declarations[index]; + }; + let member = match self.decl(parent) { + Declaration::Class(c) => c.members[index].as_declaration(), + Declaration::Module(m) => m.members[index].as_declaration(), + _ => unreachable!("only class/module declarations have child declarations"), + }; + member.expect("DeclNode must point at a Declaration") } } diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index 2280b93aea..bac8b56f11 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -11,7 +11,67 @@ pub struct SingleEntry { pub decl: DeclId, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ModuleEntry { + name: TypeName, + decls: Vec, +} + +impl ModuleEntry { + pub(super) fn new(name: TypeName) -> Self { + Self { + name, + decls: Vec::new(), + } + } + + pub(super) fn push(&mut self, decl: DeclId) { + self.decls.push(decl); + } + + #[must_use] + pub fn name(&self) -> TypeName { + self.name + } + + pub fn each_decl(&self) -> impl Iterator + '_ { + self.decls.iter().copied() + } + + /// # Panics + /// + /// Panics if the entry is empty, which registration never leaves behind. + #[must_use] + // No `&Environment`: only a class needs to read the declarations to pick + // the one with a superclass, and that lives on `ClassEntry`, as in Ruby. + pub fn primary_decl(&self) -> DeclId { + *self + .decls + .first() + .expect("an entry always has at least one declaration") + } +} + impl Environment { + #[must_use] + pub fn is_module_decl(&self, name: TypeName) -> bool { + self.class_decls.contains_key(&name) + } + + #[must_use] + pub fn is_module_name(&self, name: TypeName) -> bool { + self.class_decls.contains_key(&name) + } + + #[must_use] + pub fn module_entry(&self, name: TypeName) -> Option<&ModuleEntry> { + self.class_decls.get(&name) + } + + pub fn class_entries(&self) -> impl Iterator { + self.class_decls.values() + } + #[must_use] pub fn is_interface_name(&self, name: TypeName) -> bool { self.interface_decls.contains_key(&name) diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index 6185cb4f46..9654888de4 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -1,81 +1,143 @@ -use crate::ast::Declaration; -use crate::ids::{Id, IdIndexMap}; +use crate::ast::{Declaration, ModuleMember}; +use crate::ids::{Id, IdIndexMap, SymbolId, TypeName}; +use crate::interners::Interners; -use super::entry::SingleEntry; +use super::decl::{Context, DeclNode}; +use super::entry::{ModuleEntry, SingleEntry}; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; -use crate::interners::Interners; type SingleTable = IdIndexMap>>; 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. + /// Like Ruby, nothing is rolled back on a duplication error: the source, + /// the entries inserted before the duplicate and the reopening of + /// enclosing modules all stay. The rejected declaration and everything + /// after it are 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); + let root = self.interners.type_names.absolute_root(); for index in 0..decl_count { - self.insert_decl(DeclId { - source: source_index, - index, - })?; + self.insert_decl(source_index, None, index, root, Context::root())?; } Ok(()) } - fn insert_decl(&mut self, id: DeclId) -> Result<(), DuplicatedDeclarationError> { - match self.decl(id) { - Declaration::Interface(d) => { - let name = self.interners.type_names.to_absolute(d.name); + fn insert_decl( + &mut self, + source: u32, + parent: Option, + index: u32, + namespace: TypeName, + context: Context, + ) -> Result<(), DuplicatedDeclarationError> { + enum Kind { + Module { + name: TypeName, + decl_members: Vec, + }, + Interface(TypeName), + TypeAlias(TypeName), + Global(SymbolId), + Other, + } + + let node = DeclNode { + source, + parent, + index, + context, + }; + let kind = match self.member_decl(&node) { + Declaration::Module(m) => Kind::Module { + name: m.name, + decl_members: decl_member_indices(&m.members, ModuleMember::as_declaration), + }, + Declaration::Interface(d) => Kind::Interface(d.name), + Declaration::TypeAlias(d) => Kind::TypeAlias(d.name), + Declaration::Global(d) => Kind::Global(d.name), + // TODO: classes, constants, class aliases and module aliases are + // skipped. Declarations in a class body are not registered, a + // class reopening a module is not rejected, and a module name + // clashing with a constant is not detected. + _ => Kind::Other, + }; + + match kind { + Kind::Module { name, decl_members } => { + let name = self.interners.type_names.concat(namespace, name); + let id = self.push_node(node); + self.class_decls + .entry(name) + .or_insert_with(|| ModuleEntry::new(name)) + .push(id); + self.insert_members(node.source, node.context, id, name, &decl_members) + } + Kind::Interface(name) => { + let name = self.interners.type_names.concat(namespace, name); self.insert_single( |env| &mut env.interface_decls, name, - id, + node, |i, n| i.type_names.display(n, &i.strings), ) } - Declaration::TypeAlias(d) => { - let name = self.interners.type_names.to_absolute(d.name); + Kind::TypeAlias(name) => { + let name = self.interners.type_names.concat(namespace, name); self.insert_single( |env| &mut env.type_alias_decls, name, - id, + node, |i, n| i.type_names.display(n, &i.strings), ) } - Declaration::Global(d) => { - let name = d.name; - self.insert_single( - |env| &mut env.global_decls, - name, - id, - |i, n| i.strings.resolve(n).to_owned(), - ) - } - // TODO: register the other declaration kinds. - _ => Ok(()), + Kind::Global(name) => self.insert_single( + |env| &mut env.global_decls, + name, + node, + |i, n| i.strings.resolve(n).to_owned(), + ), + Kind::Other => Ok(()), } } + fn insert_members( + &mut self, + source: u32, + context: Context, + id: DeclId, + name: TypeName, + decl_members: &[u32], + ) -> Result<(), DuplicatedDeclarationError> { + if decl_members.is_empty() { + return Ok(()); + } + let inner = self.intern_context(context, name); + for &member in decl_members { + self.insert_decl(source, Some(id), member, name, inner)?; + } + Ok(()) + } + fn insert_single( &mut self, table: fn(&mut Self) -> &mut SingleTable, name: Id, - id: DeclId, + node: DeclNode, render: fn(&Interners, Id) -> String, ) -> Result<(), DuplicatedDeclarationError> { - let decls = table(self); - if let Some(entry) = decls.get(&name) { + if let Some(entry) = table(self).get(&name) { let existing = entry.decl; let name = render(&self.interners, name); - return Err(self.duplicated_declaration(name, id, existing)); + return Err(self.duplicated_declaration(name, &node, existing)); } - decls.insert(name, SingleEntry { name, decl: id }); + let decl = self.push_node(node); + table(self).insert(name, SingleEntry { name, decl }); Ok(()) } @@ -83,30 +145,44 @@ impl Environment { fn duplicated_declaration( &self, name: String, - inserted: DeclId, + inserted: &DeclNode, existing: DeclId, ) -> DuplicatedDeclarationError { DuplicatedDeclarationError::new( name, self.duplicated_decl(inserted), - self.duplicated_decl(existing), + self.duplicated_decl(self.node(existing)), ) } - fn duplicated_decl(&self, id: DeclId) -> DuplicatedDecl { + fn duplicated_decl(&self, node: &DeclNode) -> DuplicatedDecl { DuplicatedDecl { - path: self.sources[id.source as usize].path.clone(), - location: self.decl(id).location_range(), + path: self.sources[node.source as usize].path.clone(), + location: self.member_decl(node).location_range(), } } } +fn decl_member_indices( + members: &[M], + as_declaration: impl Fn(&M) -> Option<&Declaration>, +) -> Vec { + members + .iter() + .enumerate() + .filter(|(_, m)| as_declaration(m).is_some()) + .map(|(i, _)| u32::try_from(i).expect("too many members")) + .collect() +} + #[cfg(test)] mod tests { use std::path::PathBuf; - use crate::ast::{AstConverter, Declaration}; - use crate::environment::{DuplicatedDeclarationError, Environment, Source, SourceKind}; + use crate::ast::{AstConverter, Declaration, ModuleMember}; + use crate::environment::{ + Context, DuplicatedDeclarationError, Environment, ModuleEntry, Source, SourceKind, + }; use crate::ids::{SymbolId, TypeName}; use crate::node; @@ -145,6 +221,16 @@ mod tests { env.interners_mut().strings.intern(name) } + fn nested_decl(module: &Declaration, index: usize) -> &Declaration { + let Declaration::Module(m) = module else { + panic!("expected a module declaration"); + }; + let ModuleMember::Declaration(d) = &m.members[index] else { + panic!("expected member {index} to be a declaration"); + }; + d + } + fn assert_duplicate( err: &DuplicatedDeclarationError, inserted: (&str, &Declaration), @@ -172,7 +258,6 @@ mod tests { assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); 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(); @@ -209,6 +294,7 @@ mod tests { let b = type_name(&mut env, "::_B"); let names: Vec<_> = env.interface_entries().map(|e| e.name).collect(); assert_eq!(names, [a, i]); + // The rejected declaration is not registered. 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)); @@ -319,4 +405,175 @@ mod tests { let names: Vec<_> = env.global_entries().map(|e| e.name).collect(); assert_eq!(names, [b, a]); } + + #[test] + fn primary_decl_is_first_declaration() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "interface _Animal\nend\nmodule Foo : _Animal\nend\nmodule Foo : Object\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let foo = type_name(&mut env, "::Foo"); + let entry = env.module_entry(foo).unwrap(); + assert_eq!(env.decl(entry.primary_decl()), &decls[1]); + } + + #[test] + fn insert_nested_modules() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\n module Bar\n end\n module ::Baz\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let names: Vec<_> = env.class_entries().map(ModuleEntry::name).collect(); + let expected: Vec<_> = ["::Foo", "::Foo::Bar", "::Baz"] + .into_iter() + .map(|n| type_name(&mut env, n)) + .collect(); + assert_eq!(names, expected); + for name in expected { + assert!(env.is_module_decl(name)); + assert!(env.is_module_name(name)); + } + let unknown = type_name(&mut env, "::Foo::Baz"); + assert!(!env.is_module_decl(unknown)); + assert!(!env.is_module_name(unknown)); + } + + #[test] + fn reopened_module_is_one_entry() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo\nend\nmodule Foo\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(); + + assert_eq!(env.class_entries().count(), 1); + let foo = type_name(&mut env, "::Foo"); + let entry = env.module_entry(foo).unwrap(); + let registered: Vec<_> = entry.each_decl().map(|id| env.decl(id)).collect(); + assert_eq!(registered, [&decls[0], &decls[1]]); + } + + #[test] + fn context_of_nested_declarations() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\n module ::Baz\n module Qux\n end\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [foo, baz, qux] = ["::Foo", "::Baz", "::Baz::Qux"].map(|n| type_name(&mut env, n)); + let id = |name| env.module_entry(name).unwrap().each_decl().next().unwrap(); + + assert_eq!(env.context_names(env.context_of(id(foo))).count(), 0); + let names: Vec<_> = env.context_names(env.context_of(id(baz))).collect(); + assert_eq!(names, [foo]); + let names: Vec<_> = env.context_names(env.context_of(id(qux))).collect(); + assert_eq!(names, [baz, foo]); + + assert_eq!(env.decl(id(qux)), nested_decl(nested_decl(&decls[0], 0), 0)); + } + + #[test] + fn context_is_equal_across_reopenings() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\n module Bar\n end\nend\nmodule Foo\n module Bar\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let bar = type_name(&mut env, "::Foo::Bar"); + let ids: Vec<_> = env.module_entry(bar).unwrap().each_decl().collect(); + assert_eq!(ids.len(), 2); + assert_eq!(env.context_of(ids[0]), env.context_of(ids[1])); + assert_ne!(env.context_of(ids[0]), Context::root()); + } + + #[test] + fn context_is_per_declaration_within_an_entry() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo::Bar\nend\nmodule Foo\n module Bar\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [foo, bar] = ["::Foo", "::Foo::Bar"].map(|n| type_name(&mut env, n)); + let ids: Vec<_> = env.module_entry(bar).unwrap().each_decl().collect(); + assert_eq!(ids.len(), 2); + assert_eq!(env.decl(ids[0]), &decls[0]); + assert_eq!(env.decl(ids[1]), nested_decl(&decls[1], 0)); + assert_eq!(env.context_of(ids[0]), Context::root()); + let names: Vec<_> = env.context_names(env.context_of(ids[1])).collect(); + assert_eq!(names, [foo]); + } + + #[test] + fn declarations_in_a_module_use_its_namespace() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\n interface _I\n end\n type t = String\n def foo: () -> void\n $g: String\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let interface = type_name(&mut env, "::Foo::_I"); + let alias = type_name(&mut env, "::Foo::t"); + let relative = type_name(&mut env, "::_I"); + assert!(env.is_interface_name(interface)); + assert!(env.is_type_alias_name(alias)); + assert!(!env.is_interface_name(relative)); + let global = intern(&mut env, "$g"); + assert!(env.is_global_name(global)); + + let module = &decls[0]; + let entry = env.interface_entry(interface).unwrap(); + assert_eq!(env.decl(entry.decl), nested_decl(module, 0)); + let entry = env.type_alias_entry(alias).unwrap(); + assert_eq!(env.decl(entry.decl), nested_decl(module, 1)); + let entry = env.global_entry(global).unwrap(); + assert_eq!(env.decl(entry.decl), nested_decl(module, 3)); + + let entry = env.interface_entry(interface).unwrap(); + let names: Vec<_> = env.context_names(env.context_of(entry.decl)).collect(); + let foo = type_name(&mut env, "::Foo"); + assert_eq!(names, [foo]); + } + + #[test] + fn nested_duplicate_is_reported_without_leaving_a_node() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\n interface _I\n end\nend\nmodule Foo\n interface _I\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + let nodes = env.decl_nodes.len(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + assert_eq!(err.name(), "::Foo::_I"); + assert_duplicate( + &err, + ("b.rbs", nested_decl(&decls[1], 0)), + ("a.rbs", nested_decl(&decls[0], 0)), + ); + assert_eq!(env.decl_nodes.len(), nodes + 1); + assert_eq!(env.interface_entries().count(), 1); + let foo = type_name(&mut env, "::Foo"); + assert_eq!(env.module_entry(foo).unwrap().each_decl().count(), 2); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index 2482676d0f..fe33ec3950 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -4,11 +4,15 @@ mod error; mod insert; pub mod source; -pub use decl::DeclId; -pub use entry::SingleEntry; +pub use decl::{Context, DeclId}; +pub use entry::{ModuleEntry, SingleEntry}; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; +use indexmap::IndexSet; + +use decl::{ContextNode, DeclNode}; + use crate::ids::{IdIndexMap, SymbolId, SymbolTag, TypeNameTag}; use crate::interners::Interners; use crate::loader::{EnvironmentLoader, LoadError}; @@ -20,6 +24,9 @@ use crate::loader::{EnvironmentLoader, LoadError}; pub struct Environment { interners: Interners, sources: Vec, + decl_nodes: Vec, + contexts: IndexSet, + class_decls: IdIndexMap, interface_decls: IdIndexMap, type_alias_decls: IdIndexMap, global_decls: IdIndexMap>, From bff3a5846a8e18d90973fa5f0b18ce2dcb02f099 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:02:16 +0900 Subject: [PATCH 05/10] Register classes in Environment and reject class/module name clashes Ruby keeps classes and modules in one `class_decls` table, so a name is either a class or a module. Split the entry into `ClassEntry` and `ModuleEntry` over a shared layout, and report a duplicated declaration when a name is reopened as the other kind. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 134 ++++++++++++++- rust/ruby-rbs/src/environment/insert.rs | 209 +++++++++++++++++++++--- rust/ruby-rbs/src/environment/mod.rs | 4 +- 3 files changed, 320 insertions(+), 27 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index bac8b56f11..f0a080b4a3 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -1,3 +1,6 @@ +use std::marker::PhantomData; + +use crate::ast::Declaration; use crate::ids::{SymbolId, TypeName}; use super::{DeclId, Environment}; @@ -11,17 +14,37 @@ pub struct SingleEntry { pub decl: DeclId, } +/// Whether a `class_decls` name is a class or a module. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub enum ClassDeclKind { + Class, + Module, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ClassTag; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ModuleTag; + +/// Ruby's `ClassEntry` and `ModuleEntry` differ only in `primary_decl`, so +/// they share one layout and the tag selects which `primary_decl` exists. #[derive(Clone, Debug, PartialEq, Eq)] -pub struct ModuleEntry { +pub struct Entry { name: TypeName, decls: Vec, + kind: PhantomData, } -impl ModuleEntry { +pub type ClassEntry = Entry; +pub type ModuleEntry = Entry; + +impl Entry { pub(super) fn new(name: TypeName) -> Self { Self { name, decls: Vec::new(), + kind: PhantomData, } } @@ -37,7 +60,28 @@ impl ModuleEntry { pub fn each_decl(&self) -> impl Iterator + '_ { self.decls.iter().copied() } +} +impl Entry { + /// The first declaration with a superclass, else the first one. Not + /// memoized as in Ruby: the result is the same either way. + /// + /// # Panics + /// + /// Panics if the entry is empty, which registration never leaves behind. + /// May also panic if `env` is not the one that issued the entry's ids. + #[must_use] + pub fn primary_decl(&self, env: &Environment) -> DeclId { + self.decls + .iter() + .copied() + .find(|&id| matches!(env.decl(id), Declaration::Class(c) if c.super_class.is_some())) + .or_else(|| self.decls.first().copied()) + .expect("an entry always has at least one declaration") + } +} + +impl Entry { /// # Panics /// /// Panics if the entry is empty, which registration never leaves behind. @@ -52,23 +96,103 @@ impl ModuleEntry { } } +/// Ruby's `class_decls` holds both kinds under one name space, so a name is +/// either a class or a module, never both. The name follows `class_decls`, +/// so it covers modules too. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ClassDeclEntry { + Class(ClassEntry), + Module(ModuleEntry), +} + +impl ClassDeclEntry { + pub(super) fn new(name: TypeName, kind: ClassDeclKind) -> Self { + match kind { + ClassDeclKind::Class => Self::Class(Entry::new(name)), + ClassDeclKind::Module => Self::Module(Entry::new(name)), + } + } + + pub(super) fn push(&mut self, decl: DeclId) { + match self { + Self::Class(e) => e.push(decl), + Self::Module(e) => e.push(decl), + } + } + + #[must_use] + pub fn kind(&self) -> ClassDeclKind { + match self { + Self::Class(_) => ClassDeclKind::Class, + Self::Module(_) => ClassDeclKind::Module, + } + } + + #[must_use] + pub fn name(&self) -> TypeName { + match self { + Self::Class(e) => e.name(), + Self::Module(e) => e.name(), + } + } + + pub fn each_decl(&self) -> impl Iterator + '_ { + let decls = match self { + Self::Class(e) => &e.decls, + Self::Module(e) => &e.decls, + }; + decls.iter().copied() + } + + /// # Panics + /// + /// Panics if the entry is empty, which registration never leaves behind. + /// May also panic if `env` is not the one that issued the entry's ids. + #[must_use] + pub fn primary_decl(&self, env: &Environment) -> DeclId { + match self { + Self::Class(e) => e.primary_decl(env), + Self::Module(e) => e.primary_decl(), + } + } +} + impl Environment { + #[must_use] + pub fn is_class_decl(&self, name: TypeName) -> bool { + matches!(self.class_decls.get(&name), Some(ClassDeclEntry::Class(_))) + } + #[must_use] pub fn is_module_decl(&self, name: TypeName) -> bool { - self.class_decls.contains_key(&name) + matches!(self.class_decls.get(&name), Some(ClassDeclEntry::Module(_))) } + /// TODO: class and module aliases are not consulted yet. Ruby also + /// checks `class_alias_decls`, and `class_entry` / `module_entry` + /// return alias entries; add this when the alias tables land. #[must_use] pub fn is_module_name(&self, name: TypeName) -> bool { self.class_decls.contains_key(&name) } + #[must_use] + pub fn class_entry(&self, name: TypeName) -> Option<&ClassEntry> { + match self.class_decls.get(&name) { + Some(ClassDeclEntry::Class(e)) => Some(e), + _ => None, + } + } + #[must_use] pub fn module_entry(&self, name: TypeName) -> Option<&ModuleEntry> { - self.class_decls.get(&name) + match self.class_decls.get(&name) { + Some(ClassDeclEntry::Module(e)) => Some(e), + _ => None, + } } - pub fn class_entries(&self) -> impl Iterator { + pub fn class_entries(&self) -> impl Iterator { self.class_decls.values() } diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index 9654888de4..db516dd9c1 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -1,9 +1,9 @@ -use crate::ast::{Declaration, ModuleMember}; +use crate::ast::{ClassMember, Declaration, ModuleMember}; use crate::ids::{Id, IdIndexMap, SymbolId, TypeName}; use crate::interners::Interners; use super::decl::{Context, DeclNode}; -use super::entry::{ModuleEntry, SingleEntry}; +use super::entry::{ClassDeclEntry, ClassDeclKind, SingleEntry}; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; @@ -37,7 +37,8 @@ impl Environment { context: Context, ) -> Result<(), DuplicatedDeclarationError> { enum Kind { - Module { + ClassOrModule { + kind: ClassDeclKind, name: TypeName, decl_members: Vec, }, @@ -54,27 +55,47 @@ impl Environment { context, }; let kind = match self.member_decl(&node) { - Declaration::Module(m) => Kind::Module { + Declaration::Class(c) => Kind::ClassOrModule { + kind: ClassDeclKind::Class, + name: c.name, + decl_members: decl_member_indices(&c.members, ClassMember::as_declaration), + }, + Declaration::Module(m) => Kind::ClassOrModule { + kind: ClassDeclKind::Module, name: m.name, decl_members: decl_member_indices(&m.members, ModuleMember::as_declaration), }, Declaration::Interface(d) => Kind::Interface(d.name), Declaration::TypeAlias(d) => Kind::TypeAlias(d.name), Declaration::Global(d) => Kind::Global(d.name), - // TODO: classes, constants, class aliases and module aliases are - // skipped. Declarations in a class body are not registered, a - // class reopening a module is not rejected, and a module name - // clashing with a constant is not detected. + // TODO: constants, class aliases and module aliases are skipped. + // A class or module name clashing with one of them is not + // detected, and the query side (`is_module_name`, + // `class_entry`, `module_entry`) ignores alias entries too. _ => Kind::Other, }; match kind { - Kind::Module { name, decl_members } => { + Kind::ClassOrModule { + kind, + name, + decl_members, + } => { let name = self.interners.type_names.concat(namespace, name); + if let Some(existing) = self.class_decls.get(&name) + && existing.kind() != kind + { + let existing = existing.primary_decl(self); + let name = self + .interners + .type_names + .display(name, &self.interners.strings); + return Err(self.duplicated_declaration(name, &node, existing)); + } let id = self.push_node(node); self.class_decls .entry(name) - .or_insert_with(|| ModuleEntry::new(name)) + .or_insert_with(|| ClassDeclEntry::new(name, kind)) .push(id); self.insert_members(node.source, node.context, id, name, &decl_members) } @@ -179,9 +200,9 @@ fn decl_member_indices( mod tests { use std::path::PathBuf; - use crate::ast::{AstConverter, Declaration, ModuleMember}; + use crate::ast::{AstConverter, Declaration}; use crate::environment::{ - Context, DuplicatedDeclarationError, Environment, ModuleEntry, Source, SourceKind, + ClassDeclEntry, Context, DuplicatedDeclarationError, Environment, Source, SourceKind, }; use crate::ids::{SymbolId, TypeName}; use crate::node; @@ -221,14 +242,13 @@ mod tests { env.interners_mut().strings.intern(name) } - fn nested_decl(module: &Declaration, index: usize) -> &Declaration { - let Declaration::Module(m) = module else { - panic!("expected a module declaration"); - }; - let ModuleMember::Declaration(d) = &m.members[index] else { - panic!("expected member {index} to be a declaration"); + fn nested_decl(parent: &Declaration, index: usize) -> &Declaration { + let member = match parent { + Declaration::Class(c) => c.members[index].as_declaration(), + Declaration::Module(m) => m.members[index].as_declaration(), + _ => panic!("expected a class or module declaration"), }; - d + member.unwrap_or_else(|| panic!("expected member {index} to be a declaration")) } fn assert_duplicate( @@ -431,7 +451,7 @@ mod tests { add_rbs_source(&mut env, "a.rbs", decls).unwrap(); - let names: Vec<_> = env.class_entries().map(ModuleEntry::name).collect(); + let names: Vec<_> = env.class_entries().map(ClassDeclEntry::name).collect(); let expected: Vec<_> = ["::Foo", "::Foo::Bar", "::Baz"] .into_iter() .map(|n| type_name(&mut env, n)) @@ -576,4 +596,153 @@ mod tests { let foo = type_name(&mut env, "::Foo"); assert_eq!(env.module_entry(foo).unwrap().each_decl().count(), 2); } + + #[test] + fn insert_nested_modules_in_a_class() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class Foo\n module Bar\n end\n module ::Baz\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let [foo, bar, baz] = ["::Foo", "::Foo::Bar", "::Baz"].map(|n| type_name(&mut env, n)); + assert!(env.is_class_decl(foo)); + assert!(!env.is_module_decl(foo)); + assert!(env.is_module_decl(bar)); + assert!(env.is_module_decl(baz)); + assert!(env.is_module_name(foo)); + assert!(env.class_entry(bar).is_none()); + assert!(env.module_entry(foo).is_none()); + let names: Vec<_> = env.class_entries().map(ClassDeclEntry::name).collect(); + assert_eq!(names, [foo, bar, baz]); + let names: Vec<_> = env + .context_names(env.context_of(env.module_entry(bar).unwrap().primary_decl())) + .collect(); + assert_eq!(names, [foo]); + } + + #[test] + fn insert_open_class() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class Foo\n module Bar\n end\nend\nclass Foo < String\n module Bar\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [foo, bar] = ["::Foo", "::Foo::Bar"].map(|n| type_name(&mut env, n)); + let string = type_name(&mut env, "String"); + let entry = env.class_entry(foo).unwrap(); + assert_eq!(entry.each_decl().count(), 2); + let primary = env.decl(entry.primary_decl(&env)); + assert_eq!(primary, &decls[1]); + let Declaration::Class(primary) = primary else { + panic!("expected a class declaration"); + }; + assert_eq!(primary.super_class.as_ref().unwrap().name, string); + + assert_eq!(env.module_entry(bar).unwrap().each_decl().count(), 2); + } + + #[test] + fn class_primary_decl_falls_back_to_first() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "class Foo\nend\nclass Foo\nend\n"); + + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let foo = type_name(&mut env, "::Foo"); + let entry = env.class_entry(foo).unwrap(); + assert_eq!(env.decl(entry.primary_decl(&env)), &decls[0]); + } + + #[test] + fn module_then_class_is_rejected() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo\nend\nclass Foo < String\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(), "::Foo"); + assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + let foo = type_name(&mut env, "::Foo"); + assert!(env.is_module_decl(foo)); + assert!(!env.is_class_decl(foo)); + } + + #[test] + fn class_then_module_reports_the_primary_decl() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class Foo\nend\nclass Foo < String\nend\nmodule Foo\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + + assert_duplicate(&err, ("b.rbs", &decls[2]), ("a.rbs", &decls[1])); + let foo = type_name(&mut env, "::Foo"); + assert!(env.is_class_decl(foo)); + assert_eq!(env.class_entry(foo).unwrap().each_decl().count(), 2); + } + + #[test] + fn nested_kind_mismatch_is_rejected() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class Foo\n module Bar\n end\nend\nclass Foo\n class Bar\n end\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(), "::Foo::Bar"); + assert_duplicate( + &err, + ("b.rbs", nested_decl(&decls[1], 0)), + ("a.rbs", nested_decl(&decls[0], 0)), + ); + } + + #[test] + fn members_of_a_rejected_declaration_are_not_registered() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\nend\nclass Foo\n module Baz\n end\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + let nodes = env.decl_nodes.len(); + add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + let baz = type_name(&mut env, "::Foo::Baz"); + assert!(!env.is_module_name(baz)); + assert_eq!(env.class_entries().count(), 1); + assert_eq!(env.decl_nodes.len(), nodes); + } + + #[test] + fn insert_class_in_a_module() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo\n class Bar\n end\nend\n"); + + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let [foo, bar] = ["::Foo", "::Foo::Bar"].map(|n| type_name(&mut env, n)); + assert!(env.is_module_decl(foo)); + assert!(env.is_class_decl(bar)); + assert!(env.class_entry(bar).is_some()); + assert!(env.module_entry(bar).is_none()); + let names: Vec<_> = env + .context_names(env.context_of(env.class_entry(bar).unwrap().primary_decl(&env))) + .collect(); + assert_eq!(names, [foo]); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index fe33ec3950..9dcfa7ff57 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -5,7 +5,7 @@ mod insert; pub mod source; pub use decl::{Context, DeclId}; -pub use entry::{ModuleEntry, SingleEntry}; +pub use entry::{ClassDeclEntry, ClassDeclKind, ClassEntry, ModuleEntry, SingleEntry}; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; @@ -26,7 +26,7 @@ pub struct Environment { sources: Vec, decl_nodes: Vec, contexts: IndexSet, - class_decls: IdIndexMap, + class_decls: IdIndexMap, interface_decls: IdIndexMap, type_alias_decls: IdIndexMap, global_decls: IdIndexMap>, From 605b251c46f480c5c114f38617f3a548b278b5b0 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:33:42 +0900 Subject: [PATCH 06/10] Register constants in Environment and reject clashes with classes Constant lookup needs constant entries, and Ruby's Environment rejects a constant that shares a name with a class or module. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 54 ++++- rust/ruby-rbs/src/environment/error.rs | 15 +- rust/ruby-rbs/src/environment/insert.rs | 257 +++++++++++++++++++++--- rust/ruby-rbs/src/environment/mod.rs | 5 +- 4 files changed, 288 insertions(+), 43 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index f0a080b4a3..4360cb8358 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -137,11 +137,14 @@ impl ClassDeclEntry { } pub fn each_decl(&self) -> impl Iterator + '_ { - let decls = match self { + self.decls().iter().copied() + } + + fn decls(&self) -> &[DeclId] { + match self { Self::Class(e) => &e.decls, Self::Module(e) => &e.decls, - }; - decls.iter().copied() + } } /// # Panics @@ -157,6 +160,26 @@ impl ClassDeclEntry { } } +/// What `constant_entry` finds under a name: Ruby's `ClassEntry | +/// ModuleEntry | ConstantEntry`. Class and module stay split inside +/// `ClassDeclEntry`. Named `*Lookup` to stay apart from Ruby's +/// `ConstantEntry`, which is the `Constant` arm here. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ConstantLookup<'a> { + ClassDecl(&'a ClassDeclEntry), + Constant(&'a SingleEntry), +} + +impl ConstantLookup<'_> { + pub fn each_decl(&self) -> impl Iterator + '_ { + let decls = match self { + Self::ClassDecl(e) => e.decls(), + Self::Constant(e) => std::slice::from_ref(&e.decl), + }; + decls.iter().copied() + } +} + impl Environment { #[must_use] pub fn is_class_decl(&self, name: TypeName) -> bool { @@ -192,6 +215,31 @@ impl Environment { } } + /// TODO: class and module aliases are not consulted yet. Ruby checks + /// `class_alias_decls` between `class_decls` and `constant_decls`; add + /// this when the alias tables land. + #[must_use] + pub fn constant_entry(&self, name: TypeName) -> Option> { + match self.class_decls.get(&name) { + Some(e) => Some(ConstantLookup::ClassDecl(e)), + None => self.constant_decls.get(&name).map(ConstantLookup::Constant), + } + } + + #[must_use] + pub fn is_constant_decl(&self, name: TypeName) -> bool { + self.constant_decls.contains_key(&name) + } + + #[must_use] + pub fn is_constant_name(&self, name: TypeName) -> bool { + self.is_constant_decl(name) || self.is_module_name(name) + } + + pub fn constant_entries(&self) -> impl Iterator { + self.constant_decls.values() + } + pub fn class_entries(&self) -> impl Iterator { self.class_decls.values() } diff --git a/rust/ruby-rbs/src/environment/error.rs b/rust/ruby-rbs/src/environment/error.rs index 81d6f9f789..0d7bf96cf0 100644 --- a/rust/ruby-rbs/src/environment/error.rs +++ b/rust/ruby-rbs/src/environment/error.rs @@ -19,11 +19,14 @@ pub struct DuplicatedDeclarationError { } impl DuplicatedDeclarationError { - pub(crate) fn new(name: String, inserted: DuplicatedDecl, existing: DuplicatedDecl) -> Self { - Self { - name, - decls: vec![inserted, existing], - } + pub(crate) fn new( + name: String, + inserted: DuplicatedDecl, + first_existing: DuplicatedDecl, + rest: impl IntoIterator, + ) -> Self { + let decls = [inserted, first_existing].into_iter().chain(rest).collect(); + Self { name, decls } } #[must_use] @@ -33,7 +36,7 @@ impl DuplicatedDeclarationError { /// 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. + /// `*decls`, so a constant clashing with a class reports every reopening. #[must_use] pub fn decls(&self) -> &[DuplicatedDecl] { &self.decls diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index db516dd9c1..3eb4043ed5 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -3,7 +3,7 @@ use crate::ids::{Id, IdIndexMap, SymbolId, TypeName}; use crate::interners::Interners; use super::decl::{Context, DeclNode}; -use super::entry::{ClassDeclEntry, ClassDeclKind, SingleEntry}; +use super::entry::{ClassDeclEntry, ClassDeclKind, ConstantLookup, SingleEntry}; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; @@ -44,6 +44,7 @@ impl Environment { }, Interface(TypeName), TypeAlias(TypeName), + Constant(TypeName), Global(SymbolId), Other, } @@ -67,11 +68,13 @@ impl Environment { }, Declaration::Interface(d) => Kind::Interface(d.name), Declaration::TypeAlias(d) => Kind::TypeAlias(d.name), + Declaration::Constant(d) => Kind::Constant(d.name), Declaration::Global(d) => Kind::Global(d.name), - // TODO: constants, class aliases and module aliases are skipped. - // A class or module name clashing with one of them is not - // detected, and the query side (`is_module_name`, - // `class_entry`, `module_entry`) ignores alias entries too. + // TODO: class aliases and module aliases are skipped. A class, + // module or constant name clashing with one of them is not + // detected, and the query side (`is_module_name`, `class_entry`, + // `module_entry`, `constant_entry`, `is_constant_name`) ignores + // alias entries too. _ => Kind::Other, }; @@ -82,15 +85,8 @@ impl Environment { decl_members, } => { let name = self.interners.type_names.concat(namespace, name); - if let Some(existing) = self.class_decls.get(&name) - && existing.kind() != kind - { - let existing = existing.primary_decl(self); - let name = self - .interners - .type_names - .display(name, &self.interners.strings); - return Err(self.duplicated_declaration(name, &node, existing)); + if let Some((first, rest)) = self.constant_clash(name, Some(kind)) { + return Err(self.duplicated_type_name(name, &node, first, rest)); } let id = self.push_node(node); self.class_decls @@ -117,6 +113,14 @@ impl Environment { |i, n| i.type_names.display(n, &i.strings), ) } + Kind::Constant(name) => { + let name = self.interners.type_names.concat(namespace, name); + if let Some((first, rest)) = self.constant_clash(name, None) { + return Err(self.duplicated_type_name(name, &node, first, rest)); + } + self.register_single(|env| &mut env.constant_decls, name, node); + Ok(()) + } Kind::Global(name) => self.insert_single( |env| &mut env.global_decls, name, @@ -155,27 +159,73 @@ impl Environment { if let Some(entry) = table(self).get(&name) { let existing = entry.decl; let name = render(&self.interners, name); - return Err(self.duplicated_declaration(name, &node, existing)); + return Err(self.duplicated_declaration(name, &node, existing, [])); } + self.register_single(table, name, node); + Ok(()) + } + + fn register_single( + &mut self, + table: fn(&mut Self) -> &mut SingleTable, + name: Id, + node: DeclNode, + ) { let decl = self.push_node(node); table(self).insert(name, SingleEntry { name, decl }); + } - Ok(()) + fn duplicated_type_name( + &self, + name: TypeName, + inserted: &DeclNode, + first: DeclId, + rest: impl IntoIterator, + ) -> DuplicatedDeclarationError { + let name = self + .interners + .type_names + .display(name, &self.interners.strings); + self.duplicated_declaration(name, inserted, first, rest) } fn duplicated_declaration( &self, name: String, inserted: &DeclNode, - existing: DeclId, + first: DeclId, + rest: impl IntoIterator, ) -> DuplicatedDeclarationError { DuplicatedDeclarationError::new( name, self.duplicated_decl(inserted), - self.duplicated_decl(self.node(existing)), + self.duplicated_decl(self.node(first)), + rest.into_iter() + .map(|id| self.duplicated_decl(self.node(id))), ) } + /// The declarations a new constant-namespace entry would clash with. + /// `class_kind` is the kind of the class/module being inserted, `None` + /// for a constant declaration. Same-kind reopening is not a clash, and + /// a class/module kind mismatch reports only the primary decl. + fn constant_clash( + &self, + name: TypeName, + class_kind: Option, + ) -> Option<(DeclId, Vec)> { + match (self.constant_entry(name)?, class_kind) { + (ConstantLookup::ClassDecl(e), Some(kind)) => { + (e.kind() != kind).then(|| (e.primary_decl(self), Vec::new())) + } + (entry, _) => { + let mut ids = entry.each_decl(); + let first = ids.next()?; + Some((first, ids.collect())) + } + } + } + fn duplicated_decl(&self, node: &DeclNode) -> DuplicatedDecl { DuplicatedDecl { path: self.sources[node.source as usize].path.clone(), @@ -202,7 +252,8 @@ mod tests { use crate::ast::{AstConverter, Declaration}; use crate::environment::{ - ClassDeclEntry, Context, DuplicatedDeclarationError, Environment, Source, SourceKind, + ClassDeclEntry, ConstantLookup, Context, DuplicatedDeclarationError, Environment, Source, + SourceKind, }; use crate::ids::{SymbolId, TypeName}; use crate::node; @@ -254,15 +305,16 @@ mod tests { fn assert_duplicate( err: &DuplicatedDeclarationError, inserted: (&str, &Declaration), - existing: (&str, &Declaration), + existing: &[(&str, &Declaration)], ) { - let [got_inserted, got_existing] = err.decls() else { - panic!("expected two decls, got {:?}", err.decls()); - }; + let (got_inserted, got_existing) = err.decls().split_first().expect("decls is not empty"); assert_eq!(got_inserted.path, PathBuf::from(inserted.0)); assert_eq!(got_inserted.location, inserted.1.location_range()); - assert_eq!(got_existing.path, PathBuf::from(existing.0)); - assert_eq!(got_existing.location, existing.1.location_range()); + assert_eq!(got_existing.len(), existing.len(), "{:?}", err.decls()); + for (got, want) in got_existing.iter().zip(existing) { + assert_eq!(got.path, PathBuf::from(want.0)); + assert_eq!(got.location, want.1.location_range()); + } } // environment_test.rb:169 @@ -275,7 +327,7 @@ mod tests { let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); assert_eq!(err.name(), "::_I"); - assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); assert_eq!(err.to_string(), "*:*:*...*:*: Duplicated declaration: ::_I"); assert_eq!(env.interface_entries().count(), 1); @@ -305,7 +357,7 @@ mod tests { let err = add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap_err(); - assert_duplicate(&err, ("a.rbs", &decls[2]), ("a.rbs", &decls[1])); + assert_duplicate(&err, ("a.rbs", &decls[2]), &[("a.rbs", &decls[1])]); // Not rolled back: the source and the entries before the duplicate stay. assert_eq!(env.sources().len(), 1); @@ -352,7 +404,7 @@ mod tests { let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); assert_eq!(err.name(), "::foo"); - assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); assert_eq!(env.type_alias_entries().count(), 1); let name = type_name(&mut env, "::foo"); @@ -405,7 +457,7 @@ mod tests { let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); assert_eq!(err.name(), "$VERSION"); - assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); assert_eq!(env.global_entries().count(), 1); let name = intern(&mut env, "$VERSION"); @@ -589,7 +641,7 @@ mod tests { assert_duplicate( &err, ("b.rbs", nested_decl(&decls[1], 0)), - ("a.rbs", nested_decl(&decls[0], 0)), + &[("a.rbs", nested_decl(&decls[0], 0))], ); assert_eq!(env.decl_nodes.len(), nodes + 1); assert_eq!(env.interface_entries().count(), 1); @@ -668,7 +720,7 @@ mod tests { let err = add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); assert_eq!(err.name(), "::Foo"); - assert_duplicate(&err, ("b.rbs", &decls[1]), ("a.rbs", &decls[0])); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); let foo = type_name(&mut env, "::Foo"); assert!(env.is_module_decl(foo)); assert!(!env.is_class_decl(foo)); @@ -685,7 +737,7 @@ mod tests { add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); - assert_duplicate(&err, ("b.rbs", &decls[2]), ("a.rbs", &decls[1])); + assert_duplicate(&err, ("b.rbs", &decls[2]), &[("a.rbs", &decls[1])]); let foo = type_name(&mut env, "::Foo"); assert!(env.is_class_decl(foo)); assert_eq!(env.class_entry(foo).unwrap().each_decl().count(), 2); @@ -706,7 +758,7 @@ mod tests { assert_duplicate( &err, ("b.rbs", nested_decl(&decls[1], 0)), - ("a.rbs", nested_decl(&decls[0], 0)), + &[("a.rbs", nested_decl(&decls[0], 0))], ); } @@ -745,4 +797,143 @@ mod tests { .collect(); assert_eq!(names, [foo]); } + + #[test] + fn insert_const_duplication_error() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\nend\nBar: ::Integer\nFoo: String\nclass Bar\nend\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + assert_eq!(err.name(), "::Foo"); + assert_duplicate(&err, ("b.rbs", &decls[2]), &[("a.rbs", &decls[0])]); + + let err = add_rbs_source(&mut env, "c.rbs", vec![decls[3].clone()]).unwrap_err(); + assert_eq!(err.name(), "::Bar"); + assert_duplicate(&err, ("c.rbs", &decls[3]), &[("a.rbs", &decls[1])]); + } + + #[test] + fn const_twice_duplication_error() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "Foo: String\nFoo: String\n"); + + let err = add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap_err(); + + assert_eq!(err.name(), "::Foo"); + assert_duplicate(&err, ("a.rbs", &decls[1]), &[("a.rbs", &decls[0])]); + } + + #[test] + fn const_clashing_with_a_reopened_class_reports_every_declaration() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "class Foo\nend\nclass Foo\nend\nFoo: String\n"); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + + assert_duplicate( + &err, + ("b.rbs", &decls[2]), + &[("a.rbs", &decls[0]), ("a.rbs", &decls[1])], + ); + } + + #[test] + fn const_clashing_with_a_reopened_module_reports_every_declaration() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo\nend\nmodule Foo\nend\nFoo: String\n"); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + + assert_duplicate( + &err, + ("b.rbs", &decls[2]), + &[("a.rbs", &decls[0]), ("a.rbs", &decls[1])], + ); + } + + #[test] + fn nested_class_clashing_with_a_const_reports_the_qualified_name() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module M\n X: Integer\n class X\n end\nend\n"); + + let err = add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap_err(); + + assert_eq!(err.name(), "::M::X"); + assert_duplicate( + &err, + ("a.rbs", nested_decl(&decls[0], 1)), + &[("a.rbs", nested_decl(&decls[0], 0))], + ); + } + + #[test] + fn clash_between_const_and_class_leaves_the_environment_unchanged() { + for (first, second) in [ + ("Foo: String\n", "class Foo\nend\n"), + ("class Foo\nend\n", "Foo: String\n"), + ] { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, &format!("{first}{second}")); + add_rbs_source(&mut env, "a.rbs", decls[..1].to_vec()).unwrap(); + let nodes = env.decl_nodes.len(); + let consts = env.constant_decls.len(); + let classes = env.class_decls.len(); + + add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + assert_eq!(env.decl_nodes.len(), nodes); + assert_eq!(env.constant_decls.len(), consts); + assert_eq!(env.class_decls.len(), classes); + } + } + + #[test] + fn constant_entry_finds_class_module_and_constant() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "class A\nend\nmodule B\nend\nC: String\n"); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [a, b, c, d] = ["::A", "::B", "::C", "::D"].map(|n| type_name(&mut env, n)); + assert!(matches!( + env.constant_entry(a), + Some(ConstantLookup::ClassDecl(ClassDeclEntry::Class(_))) + )); + assert!(matches!( + env.constant_entry(b), + Some(ConstantLookup::ClassDecl(ClassDeclEntry::Module(_))) + )); + let Some(ConstantLookup::Constant(entry)) = env.constant_entry(c) else { + panic!("expected a constant entry"); + }; + assert_eq!(env.decl(entry.decl), &decls[2]); + assert!(env.constant_entry(d).is_none()); + + assert!(env.is_constant_decl(c)); + assert!(!env.is_constant_decl(a)); + assert!(env.is_constant_name(a)); + assert!(env.is_constant_name(b)); + assert!(env.is_constant_name(c)); + assert!(!env.is_constant_name(d)); + } + + #[test] + fn constants_are_registered_in_order_with_their_namespace() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module M\n X: Integer\nend\nB: String\nA: String\n", + ); + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let [x, b, a] = ["::M::X", "::B", "::A"].map(|n| type_name(&mut env, n)); + let names: Vec<_> = env.constant_entries().map(|e| e.name).collect(); + assert_eq!(names, [x, b, a]); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index 9dcfa7ff57..40bd3a0568 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -5,7 +5,9 @@ mod insert; pub mod source; pub use decl::{Context, DeclId}; -pub use entry::{ClassDeclEntry, ClassDeclKind, ClassEntry, ModuleEntry, SingleEntry}; +pub use entry::{ + ClassDeclEntry, ClassDeclKind, ClassEntry, ConstantLookup, ModuleEntry, SingleEntry, +}; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; @@ -27,6 +29,7 @@ pub struct Environment { decl_nodes: Vec, contexts: IndexSet, class_decls: IdIndexMap, + constant_decls: IdIndexMap, interface_decls: IdIndexMap, type_alias_decls: IdIndexMap, global_decls: IdIndexMap>, From 1a9e33017e0d6f81268c338493ef34ecfbfe7a15 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:59:57 +0900 Subject: [PATCH 07/10] Register module aliases in Environment and reject clashes with them Ruby keeps module aliases in the same constant name space as classes, modules and constants, so `module Foo = Kernel` followed by `module Foo` or `Foo: String` must raise a duplication error. module_entry now returns ModuleLookup since an alias has no ModuleEntry. class_entry is unchanged until class aliases are registered. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 102 +++++++++--- rust/ruby-rbs/src/environment/insert.rs | 196 +++++++++++++++++++++--- rust/ruby-rbs/src/environment/mod.rs | 4 +- 3 files changed, 264 insertions(+), 38 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index 4360cb8358..0b334d7499 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -58,7 +58,11 @@ impl Entry { } pub fn each_decl(&self) -> impl Iterator + '_ { - self.decls.iter().copied() + self.decls().iter().copied() + } + + fn decls(&self) -> &[DeclId] { + &self.decls } } @@ -142,8 +146,8 @@ impl ClassDeclEntry { fn decls(&self) -> &[DeclId] { match self { - Self::Class(e) => &e.decls, - Self::Module(e) => &e.decls, + Self::Class(e) => e.decls(), + Self::Module(e) => e.decls(), } } @@ -160,13 +164,21 @@ impl ClassDeclEntry { } } +/// Ruby's `class_alias_decls` values: `ModuleAliasEntry | ClassAliasEntry`. +/// Only the module arm exists until class aliases are registered. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum AliasEntry { + Module(SingleEntry), +} + /// What `constant_entry` finds under a name: Ruby's `ClassEntry | -/// ModuleEntry | ConstantEntry`. Class and module stay split inside -/// `ClassDeclEntry`. Named `*Lookup` to stay apart from Ruby's +/// ModuleEntry | ModuleAliasEntry | ConstantEntry`. Class and module stay +/// split inside `ClassDeclEntry`. Named `*Lookup` to stay apart from Ruby's /// `ConstantEntry`, which is the `Constant` arm here. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ConstantLookup<'a> { ClassDecl(&'a ClassDeclEntry), + ModuleAlias(&'a SingleEntry), Constant(&'a SingleEntry), } @@ -174,12 +186,49 @@ impl ConstantLookup<'_> { pub fn each_decl(&self) -> impl Iterator + '_ { let decls = match self { Self::ClassDecl(e) => e.decls(), - Self::Constant(e) => std::slice::from_ref(&e.decl), + Self::ModuleAlias(e) | Self::Constant(e) => std::slice::from_ref(&e.decl), }; decls.iter().copied() } } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ModuleLookup<'a> { + Module(&'a ModuleEntry), + ModuleAlias(&'a SingleEntry), +} + +impl ModuleLookup<'_> { + #[must_use] + pub fn name(&self) -> TypeName { + match self { + Self::Module(e) => e.name(), + Self::ModuleAlias(e) => e.name, + } + } + + pub fn each_decl(&self) -> impl Iterator + '_ { + let decls = match self { + Self::Module(e) => e.decls(), + Self::ModuleAlias(e) => std::slice::from_ref(&e.decl), + }; + decls.iter().copied() + } + + /// An alias has the one declaration it was registered with. + /// + /// # Panics + /// + /// Panics if a module entry is empty, which registration never leaves behind. + #[must_use] + pub fn primary_decl(&self) -> DeclId { + match self { + Self::Module(e) => e.primary_decl(), + Self::ModuleAlias(e) => e.decl, + } + } +} + impl Environment { #[must_use] pub fn is_class_decl(&self, name: TypeName) -> bool { @@ -191,14 +240,27 @@ impl Environment { matches!(self.class_decls.get(&name), Some(ClassDeclEntry::Module(_))) } - /// TODO: class and module aliases are not consulted yet. Ruby also - /// checks `class_alias_decls`, and `class_entry` / `module_entry` - /// return alias entries; add this when the alias tables land. + #[must_use] + pub fn is_module_alias(&self, name: TypeName) -> bool { + matches!( + self.class_alias_decls.get(&name), + Some(AliasEntry::Module(_)) + ) + } + #[must_use] pub fn is_module_name(&self, name: TypeName) -> bool { - self.class_decls.contains_key(&name) + self.class_decls.contains_key(&name) || self.class_alias_decls.contains_key(&name) + } + + #[must_use] + pub fn is_type_name(&self, name: TypeName) -> bool { + self.is_interface_name(name) || self.is_type_alias_name(name) || self.is_module_name(name) } + /// TODO: class aliases are not consulted yet. Ruby's `class_entry` + /// returns `ClassEntry | ClassAliasEntry`; return a lookup enum like + /// `module_entry` does when class aliases are registered. #[must_use] pub fn class_entry(&self, name: TypeName) -> Option<&ClassEntry> { match self.class_decls.get(&name) { @@ -208,22 +270,24 @@ impl Environment { } #[must_use] - pub fn module_entry(&self, name: TypeName) -> Option<&ModuleEntry> { - match self.class_decls.get(&name) { - Some(ClassDeclEntry::Module(e)) => Some(e), + pub fn module_entry(&self, name: TypeName) -> Option> { + match self.constant_entry(name)? { + ConstantLookup::ClassDecl(ClassDeclEntry::Module(e)) => Some(ModuleLookup::Module(e)), + ConstantLookup::ModuleAlias(e) => Some(ModuleLookup::ModuleAlias(e)), _ => None, } } - /// TODO: class and module aliases are not consulted yet. Ruby checks - /// `class_alias_decls` between `class_decls` and `constant_decls`; add - /// this when the alias tables land. #[must_use] pub fn constant_entry(&self, name: TypeName) -> Option> { - match self.class_decls.get(&name) { - Some(e) => Some(ConstantLookup::ClassDecl(e)), - None => self.constant_decls.get(&name).map(ConstantLookup::Constant), + if let Some(e) = self.class_decls.get(&name) { + return Some(ConstantLookup::ClassDecl(e)); + } + match self.class_alias_decls.get(&name) { + Some(AliasEntry::Module(e)) => return Some(ConstantLookup::ModuleAlias(e)), + None => {} } + self.constant_decls.get(&name).map(ConstantLookup::Constant) } #[must_use] diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index 3eb4043ed5..5eabc69ec0 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -3,7 +3,7 @@ use crate::ids::{Id, IdIndexMap, SymbolId, TypeName}; use crate::interners::Interners; use super::decl::{Context, DeclNode}; -use super::entry::{ClassDeclEntry, ClassDeclKind, ConstantLookup, SingleEntry}; +use super::entry::{AliasEntry, ClassDeclEntry, ClassDeclKind, ConstantLookup, SingleEntry}; use super::error::{DuplicatedDecl, DuplicatedDeclarationError}; use super::source::Source; use super::{DeclId, Environment}; @@ -45,6 +45,7 @@ impl Environment { Interface(TypeName), TypeAlias(TypeName), Constant(TypeName), + ModuleAlias(TypeName), Global(SymbolId), Other, } @@ -70,12 +71,12 @@ impl Environment { Declaration::TypeAlias(d) => Kind::TypeAlias(d.name), Declaration::Constant(d) => Kind::Constant(d.name), Declaration::Global(d) => Kind::Global(d.name), - // TODO: class aliases and module aliases are skipped. A class, - // module or constant name clashing with one of them is not - // detected, and the query side (`is_module_name`, `class_entry`, - // `module_entry`, `constant_entry`, `is_constant_name`) ignores - // alias entries too. - _ => Kind::Other, + Declaration::ModuleAlias(d) => Kind::ModuleAlias(d.new_name), + // TODO: class aliases are skipped. A class, module, constant or + // module alias name clashing with one of them is not detected, + // and `constant_entry` and `class_entry` ignore class alias + // entries too. + Declaration::ClassAlias(_) => Kind::Other, }; match kind { @@ -113,13 +114,13 @@ impl Environment { |i, n| i.type_names.display(n, &i.strings), ) } - Kind::Constant(name) => { - let name = self.interners.type_names.concat(namespace, name); - if let Some((first, rest)) = self.constant_clash(name, None) { - return Err(self.duplicated_type_name(name, &node, first, rest)); - } - self.register_single(|env| &mut env.constant_decls, name, node); - Ok(()) + Kind::Constant(name) => self.insert_constant_like(namespace, name, node, |env, e| { + env.constant_decls.insert(e.name, e); + }), + Kind::ModuleAlias(name) => { + self.insert_constant_like(namespace, name, node, |env, e| { + env.class_alias_decls.insert(e.name, AliasEntry::Module(e)); + }) } Kind::Global(name) => self.insert_single( |env| &mut env.global_decls, @@ -149,6 +150,22 @@ impl Environment { Ok(()) } + fn insert_constant_like( + &mut self, + namespace: TypeName, + name: TypeName, + node: DeclNode, + store: fn(&mut Self, SingleEntry), + ) -> Result<(), DuplicatedDeclarationError> { + let name = self.interners.type_names.concat(namespace, name); + if let Some((first, rest)) = self.constant_clash(name, None) { + return Err(self.duplicated_type_name(name, &node, first, rest)); + } + let decl = self.push_node(node); + store(self, SingleEntry { name, decl }); + Ok(()) + } + fn insert_single( &mut self, table: fn(&mut Self) -> &mut SingleTable, @@ -252,8 +269,8 @@ mod tests { use crate::ast::{AstConverter, Declaration}; use crate::environment::{ - ClassDeclEntry, ConstantLookup, Context, DuplicatedDeclarationError, Environment, Source, - SourceKind, + ClassDeclEntry, ConstantLookup, Context, DuplicatedDeclarationError, Environment, + ModuleLookup, Source, SourceKind, }; use crate::ids::{SymbolId, TypeName}; use crate::node; @@ -489,7 +506,9 @@ mod tests { add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); let foo = type_name(&mut env, "::Foo"); - let entry = env.module_entry(foo).unwrap(); + let ModuleLookup::Module(entry) = env.module_entry(foo).unwrap() else { + panic!("expected a module entry"); + }; assert_eq!(env.decl(entry.primary_decl()), &decls[1]); } @@ -669,8 +688,11 @@ mod tests { assert!(env.module_entry(foo).is_none()); let names: Vec<_> = env.class_entries().map(ClassDeclEntry::name).collect(); assert_eq!(names, [foo, bar, baz]); + let ModuleLookup::Module(entry) = env.module_entry(bar).unwrap() else { + panic!("expected a module entry"); + }; let names: Vec<_> = env - .context_names(env.context_of(env.module_entry(bar).unwrap().primary_decl())) + .context_names(env.context_of(entry.primary_decl())) .collect(); assert_eq!(names, [foo]); } @@ -936,4 +958,142 @@ mod tests { let names: Vec<_> = env.constant_entries().map(|e| e.name).collect(); assert_eq!(names, [x, b, a]); } + + #[test] + fn clash_with_a_module_alias_is_rejected_in_both_directions() { + for (first, second) in [ + ("module Foo = Kernel\n", "Foo: String\n"), + ("module Foo = Kernel\n", "module Foo\nend\n"), + ("module Foo = Kernel\n", "class Foo\nend\n"), + ("Foo: String\n", "module Foo = Kernel\n"), + ("module Foo = Kernel\n", "module Foo = Comparable\n"), + ("module Foo\nend\n", "module Foo = Kernel\n"), + ("class Foo\nend\n", "module Foo = Kernel\n"), + ] { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, &format!("{first}{second}")); + 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(), "::Foo", "{first}{second}"); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); + } + } + + #[test] + fn module_alias_clashing_with_a_reopened_module_reports_every_declaration() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module Foo\nend\nmodule Foo\nend\nmodule Foo = Kernel\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + + assert_duplicate( + &err, + ("b.rbs", &decls[2]), + &[("a.rbs", &decls[0]), ("a.rbs", &decls[1])], + ); + } + + #[test] + fn nested_module_alias_uses_the_namespace() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module M\n module A = Kernel\nend\n"); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [a, top_a] = ["::M::A", "::A"].map(|n| type_name(&mut env, n)); + let Some(ModuleLookup::ModuleAlias(entry)) = env.module_entry(a) else { + panic!("expected a module alias entry"); + }; + assert_eq!(entry.name, a); + assert_eq!(env.decl(entry.decl), nested_decl(&decls[0], 0)); + assert!(env.module_entry(top_a).is_none()); + } + + #[test] + fn module_entry_finds_module_and_module_alias_but_not_constant() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module A\nend\nmodule B = Kernel\nC: String\nclass D\nend\n", + ); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [a, b, c, d, e] = ["::A", "::B", "::C", "::D", "::E"].map(|n| type_name(&mut env, n)); + assert!(matches!(env.module_entry(a), Some(ModuleLookup::Module(_)))); + let Some(ModuleLookup::ModuleAlias(entry)) = env.module_entry(b) else { + panic!("expected a module alias entry"); + }; + assert_eq!(env.decl(entry.decl), &decls[1]); + assert!(env.module_entry(c).is_none()); + assert!(env.module_entry(d).is_none()); + assert!(env.module_entry(e).is_none()); + + let alias = env.module_entry(b).unwrap(); + assert_eq!(alias.name(), b); + assert_eq!(alias.primary_decl(), entry.decl); + assert_eq!(alias.each_decl().collect::>(), [entry.decl]); + + let module = env.module_entry(a).unwrap(); + assert_eq!(module.name(), a); + assert_eq!(env.decl(module.primary_decl()), &decls[0]); + } + + #[test] + fn constant_entry_finds_module_alias() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo = Kernel\n"); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let foo = type_name(&mut env, "::Foo"); + let Some(ConstantLookup::ModuleAlias(entry)) = env.constant_entry(foo) else { + panic!("expected a module alias entry"); + }; + assert_eq!(env.decl(entry.decl), &decls[0]); + assert!(env.is_constant_name(foo)); + assert!(!env.is_constant_decl(foo)); + } + + #[test] + fn module_alias_predicates() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module A = Kernel\nmodule B\nend\ninterface _I\nend\ntype t = Integer\nC: String\n", + ); + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let [a, b, i, t, c, d] = + ["::A", "::B", "::_I", "::t", "::C", "::D"].map(|n| type_name(&mut env, n)); + assert!(env.is_module_alias(a)); + assert!(!env.is_module_alias(b)); + assert!(env.is_module_name(a)); + assert!(env.is_module_name(b)); + assert!(!env.is_module_decl(a)); + for name in [a, b, i, t] { + assert!(env.is_type_name(name)); + } + assert!(!env.is_type_name(c)); + assert!(!env.is_type_name(d)); + } + + #[test] + fn clash_with_a_module_alias_leaves_the_environment_unchanged() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "module Foo = Kernel\nFoo: String\n"); + add_rbs_source(&mut env, "a.rbs", vec![decls[0].clone()]).unwrap(); + let nodes = env.decl_nodes.len(); + let aliases = env.class_alias_decls.len(); + let consts = env.constant_decls.len(); + + add_rbs_source(&mut env, "b.rbs", vec![decls[1].clone()]).unwrap_err(); + + assert_eq!(env.decl_nodes.len(), nodes); + assert_eq!(env.class_alias_decls.len(), aliases); + assert_eq!(env.constant_decls.len(), consts); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index 40bd3a0568..ef4ad0e331 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -6,7 +6,8 @@ pub mod source; pub use decl::{Context, DeclId}; pub use entry::{ - ClassDeclEntry, ClassDeclKind, ClassEntry, ConstantLookup, ModuleEntry, SingleEntry, + AliasEntry, ClassDeclEntry, ClassDeclKind, ClassEntry, ConstantLookup, ModuleEntry, + ModuleLookup, SingleEntry, }; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; @@ -29,6 +30,7 @@ pub struct Environment { decl_nodes: Vec, contexts: IndexSet, class_decls: IdIndexMap, + class_alias_decls: IdIndexMap, constant_decls: IdIndexMap, interface_decls: IdIndexMap, type_alias_decls: IdIndexMap, From 423468025ec64adfd969813f9617ef166ebb6824 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:09:39 +0900 Subject: [PATCH 08/10] Register class aliases in Environment and reject clashes with them Skipping them let a class, module, constant or module alias reuse an alias name undetected, and class_entry could not see an alias, unlike Ruby. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 146 ++++++++++++++++++--- rust/ruby-rbs/src/environment/insert.rs | 160 +++++++++++++++++++++--- rust/ruby-rbs/src/environment/mod.rs | 4 +- 3 files changed, 274 insertions(+), 36 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index 0b334d7499..1725eebfcc 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -165,31 +165,116 @@ impl ClassDeclEntry { } /// Ruby's `class_alias_decls` values: `ModuleAliasEntry | ClassAliasEntry`. -/// Only the module arm exists until class aliases are registered. #[derive(Clone, Debug, PartialEq, Eq)] pub enum AliasEntry { Module(SingleEntry), + Class(SingleEntry), +} + +impl SingleEntry { + fn decls(&self) -> &[DeclId] { + std::slice::from_ref(&self.decl) + } } /// What `constant_entry` finds under a name: Ruby's `ClassEntry | -/// ModuleEntry | ModuleAliasEntry | ConstantEntry`. Class and module stay -/// split inside `ClassDeclEntry`. Named `*Lookup` to stay apart from Ruby's -/// `ConstantEntry`, which is the `Constant` arm here. +/// ModuleEntry | ClassAliasEntry | ModuleAliasEntry | ConstantEntry`. Class +/// and module stay split inside `ClassDeclEntry`. Named `*Lookup` to stay +/// apart from Ruby's `ConstantEntry`, which is the `Constant` arm here. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ConstantLookup<'a> { ClassDecl(&'a ClassDeclEntry), + ClassAlias(&'a SingleEntry), ModuleAlias(&'a SingleEntry), Constant(&'a SingleEntry), } -impl ConstantLookup<'_> { - pub fn each_decl(&self) -> impl Iterator + '_ { - let decls = match self { +impl<'a> ConstantLookup<'a> { + pub fn each_decl(&self) -> impl Iterator + 'a { + let decls = match *self { + Self::ClassDecl(e) => e.decls(), + Self::ClassAlias(e) | Self::ModuleAlias(e) | Self::Constant(e) => e.decls(), + }; + decls.iter().copied() + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ClassLookup<'a> { + Class(&'a ClassEntry), + ClassAlias(&'a SingleEntry), +} + +impl<'a> ClassLookup<'a> { + #[must_use] + pub fn name(&self) -> TypeName { + match self { + Self::Class(e) => e.name(), + Self::ClassAlias(e) => e.name, + } + } + + pub fn each_decl(&self) -> impl Iterator + 'a { + let decls = match *self { + Self::Class(e) => e.decls(), + Self::ClassAlias(e) => e.decls(), + }; + decls.iter().copied() + } + + /// An alias has the one declaration it was registered with. + /// + /// # Panics + /// + /// Panics if a class entry is empty, which registration never leaves behind. + /// May also panic if `env` is not the one that issued the entry's ids. + #[must_use] + pub fn primary_decl(&self, env: &Environment) -> DeclId { + match self { + Self::Class(e) => e.primary_decl(env), + Self::ClassAlias(e) => e.decl, + } + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ModuleClassLookup<'a> { + ClassDecl(&'a ClassDeclEntry), + ClassAlias(&'a SingleEntry), + ModuleAlias(&'a SingleEntry), +} + +impl<'a> ModuleClassLookup<'a> { + #[must_use] + pub fn name(&self) -> TypeName { + match self { + Self::ClassDecl(e) => e.name(), + Self::ClassAlias(e) | Self::ModuleAlias(e) => e.name, + } + } + + pub fn each_decl(&self) -> impl Iterator + 'a { + let decls = match *self { Self::ClassDecl(e) => e.decls(), - Self::ModuleAlias(e) | Self::Constant(e) => std::slice::from_ref(&e.decl), + Self::ClassAlias(e) | Self::ModuleAlias(e) => e.decls(), }; decls.iter().copied() } + + /// An alias has the one declaration it was registered with. + /// + /// # Panics + /// + /// Panics if a class or module entry is empty, which registration never + /// leaves behind. + /// May also panic if `env` is not the one that issued the entry's ids. + #[must_use] + pub fn primary_decl(&self, env: &Environment) -> DeclId { + match self { + Self::ClassDecl(e) => e.primary_decl(env), + Self::ClassAlias(e) | Self::ModuleAlias(e) => e.decl, + } + } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -198,7 +283,7 @@ pub enum ModuleLookup<'a> { ModuleAlias(&'a SingleEntry), } -impl ModuleLookup<'_> { +impl<'a> ModuleLookup<'a> { #[must_use] pub fn name(&self) -> TypeName { match self { @@ -207,10 +292,10 @@ impl ModuleLookup<'_> { } } - pub fn each_decl(&self) -> impl Iterator + '_ { - let decls = match self { + pub fn each_decl(&self) -> impl Iterator + 'a { + let decls = match *self { Self::Module(e) => e.decls(), - Self::ModuleAlias(e) => std::slice::from_ref(&e.decl), + Self::ModuleAlias(e) => e.decls(), }; decls.iter().copied() } @@ -248,6 +333,14 @@ impl Environment { ) } + #[must_use] + pub fn is_class_alias(&self, name: TypeName) -> bool { + matches!( + self.class_alias_decls.get(&name), + Some(AliasEntry::Class(_)) + ) + } + #[must_use] pub fn is_module_name(&self, name: TypeName) -> bool { self.class_decls.contains_key(&name) || self.class_alias_decls.contains_key(&name) @@ -258,14 +351,14 @@ impl Environment { self.is_interface_name(name) || self.is_type_alias_name(name) || self.is_module_name(name) } - /// TODO: class aliases are not consulted yet. Ruby's `class_entry` - /// returns `ClassEntry | ClassAliasEntry`; return a lookup enum like - /// `module_entry` does when class aliases are registered. #[must_use] - pub fn class_entry(&self, name: TypeName) -> Option<&ClassEntry> { - match self.class_decls.get(&name) { - Some(ClassDeclEntry::Class(e)) => Some(e), - _ => None, + pub fn class_entry(&self, name: TypeName) -> Option> { + match self.constant_entry(name)? { + ConstantLookup::ClassDecl(ClassDeclEntry::Class(e)) => Some(ClassLookup::Class(e)), + ConstantLookup::ClassAlias(e) => Some(ClassLookup::ClassAlias(e)), + ConstantLookup::ClassDecl(ClassDeclEntry::Module(_)) + | ConstantLookup::ModuleAlias(_) + | ConstantLookup::Constant(_) => None, } } @@ -274,7 +367,19 @@ impl Environment { match self.constant_entry(name)? { ConstantLookup::ClassDecl(ClassDeclEntry::Module(e)) => Some(ModuleLookup::Module(e)), ConstantLookup::ModuleAlias(e) => Some(ModuleLookup::ModuleAlias(e)), - _ => None, + ConstantLookup::ClassDecl(ClassDeclEntry::Class(_)) + | ConstantLookup::ClassAlias(_) + | ConstantLookup::Constant(_) => None, + } + } + + #[must_use] + pub fn module_class_entry(&self, name: TypeName) -> Option> { + match self.constant_entry(name)? { + ConstantLookup::ClassDecl(e) => Some(ModuleClassLookup::ClassDecl(e)), + ConstantLookup::ClassAlias(e) => Some(ModuleClassLookup::ClassAlias(e)), + ConstantLookup::ModuleAlias(e) => Some(ModuleClassLookup::ModuleAlias(e)), + ConstantLookup::Constant(_) => None, } } @@ -284,6 +389,7 @@ impl Environment { return Some(ConstantLookup::ClassDecl(e)); } match self.class_alias_decls.get(&name) { + Some(AliasEntry::Class(e)) => return Some(ConstantLookup::ClassAlias(e)), Some(AliasEntry::Module(e)) => return Some(ConstantLookup::ModuleAlias(e)), None => {} } diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index 5eabc69ec0..e94e1738ef 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -45,9 +45,8 @@ impl Environment { Interface(TypeName), TypeAlias(TypeName), Constant(TypeName), - ModuleAlias(TypeName), + Alias(TypeName, fn(SingleEntry) -> AliasEntry), Global(SymbolId), - Other, } let node = DeclNode { @@ -71,12 +70,8 @@ impl Environment { Declaration::TypeAlias(d) => Kind::TypeAlias(d.name), Declaration::Constant(d) => Kind::Constant(d.name), Declaration::Global(d) => Kind::Global(d.name), - Declaration::ModuleAlias(d) => Kind::ModuleAlias(d.new_name), - // TODO: class aliases are skipped. A class, module, constant or - // module alias name clashing with one of them is not detected, - // and `constant_entry` and `class_entry` ignore class alias - // entries too. - Declaration::ClassAlias(_) => Kind::Other, + Declaration::ModuleAlias(d) => Kind::Alias(d.new_name, AliasEntry::Module), + Declaration::ClassAlias(d) => Kind::Alias(d.new_name, AliasEntry::Class), }; match kind { @@ -117,9 +112,9 @@ impl Environment { Kind::Constant(name) => self.insert_constant_like(namespace, name, node, |env, e| { env.constant_decls.insert(e.name, e); }), - Kind::ModuleAlias(name) => { + Kind::Alias(name, wrap) => { self.insert_constant_like(namespace, name, node, |env, e| { - env.class_alias_decls.insert(e.name, AliasEntry::Module(e)); + env.class_alias_decls.insert(e.name, wrap(e)); }) } Kind::Global(name) => self.insert_single( @@ -128,7 +123,6 @@ impl Environment { node, |i, n| i.strings.resolve(n).to_owned(), ), - Kind::Other => Ok(()), } } @@ -155,7 +149,7 @@ impl Environment { namespace: TypeName, name: TypeName, node: DeclNode, - store: fn(&mut Self, SingleEntry), + store: impl FnOnce(&mut Self, SingleEntry), ) -> Result<(), DuplicatedDeclarationError> { let name = self.interners.type_names.concat(namespace, name); if let Some((first, rest)) = self.constant_clash(name, None) { @@ -269,8 +263,8 @@ mod tests { use crate::ast::{AstConverter, Declaration}; use crate::environment::{ - ClassDeclEntry, ConstantLookup, Context, DuplicatedDeclarationError, Environment, - ModuleLookup, Source, SourceKind, + ClassDeclEntry, ClassLookup, ConstantLookup, Context, DuplicatedDeclarationError, + Environment, ModuleClassLookup, ModuleLookup, Source, SourceKind, }; use crate::ids::{SymbolId, TypeName}; use crate::node; @@ -1096,4 +1090,142 @@ mod tests { assert_eq!(env.class_alias_decls.len(), aliases); assert_eq!(env.constant_decls.len(), consts); } + + #[test] + fn insert_class_module_alias() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "module RBS\n module Kernel = ::Kernel\n\n class RbObject = Object\nend\n", + ); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [kernel, object] = ["::RBS::Kernel", "::RBS::RbObject"].map(|n| type_name(&mut env, n)); + assert!(env.is_module_alias(kernel)); + assert!(!env.is_class_alias(kernel)); + assert!(env.is_class_alias(object)); + assert!(!env.is_module_alias(object)); + assert!(env.is_module_name(object)); + assert!(env.is_type_name(object)); + assert!(env.is_constant_name(object)); + assert!(!env.is_class_decl(object)); + let Some(ConstantLookup::ClassAlias(entry)) = env.constant_entry(object) else { + panic!("expected a class alias entry"); + }; + assert_eq!(env.decl(entry.decl), nested_decl(&decls[0], 1)); + } + + #[test] + fn clash_with_a_class_alias_is_rejected_in_both_directions() { + for (first, second) in [ + ("class Foo = Object\n", "Foo: String\n"), + ("class Foo = Object\n", "class Foo\nend\n"), + ("class Foo = Object\n", "module Foo\nend\n"), + ("class Foo = Object\n", "module Foo = Kernel\n"), + ("class Foo = Object\n", "class Foo = String\n"), + ("Foo: String\n", "class Foo = Object\n"), + ("class Foo\nend\n", "class Foo = Object\n"), + ("module Foo\nend\n", "class Foo = Object\n"), + ("module Foo = Kernel\n", "class Foo = Object\n"), + ] { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, &format!("{first}{second}")); + 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(), "::Foo", "{first}{second}"); + assert_duplicate(&err, ("b.rbs", &decls[1]), &[("a.rbs", &decls[0])]); + } + } + + #[test] + fn class_entry_finds_class_and_class_alias_only() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class A\nend\nclass B = Object\nmodule C\nend\nmodule D = Kernel\nE: String\n", + ); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [a, b, c, d, e, f] = + ["::A", "::B", "::C", "::D", "::E", "::F"].map(|n| type_name(&mut env, n)); + let class = env.class_entry(a).unwrap(); + assert!(matches!(class, ClassLookup::Class(_))); + assert_eq!(class.name(), a); + assert_eq!(env.decl(class.primary_decl(&env)), &decls[0]); + let alias = env.class_entry(b).unwrap(); + assert!(matches!(alias, ClassLookup::ClassAlias(_))); + assert_eq!(alias.name(), b); + assert_eq!(env.decl(alias.primary_decl(&env)), &decls[1]); + assert_eq!(alias.each_decl().count(), 1); + for name in [c, d, e, f] { + assert!(env.class_entry(name).is_none()); + } + } + + #[test] + fn module_entry_does_not_find_a_class_alias() { + let mut env = Environment::new(); + let decls = parse_decls(&mut env, "class A = Object\n"); + add_rbs_source(&mut env, "a.rbs", decls).unwrap(); + + let a = type_name(&mut env, "::A"); + assert!(env.module_entry(a).is_none()); + } + + #[test] + fn module_class_entry_finds_everything_but_constants() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class A\nend\nmodule B\nend\nclass C = Object\nmodule D = Kernel\nE: String\n", + ); + add_rbs_source(&mut env, "a.rbs", decls.clone()).unwrap(); + + let [a, b, c, d, e, f] = + ["::A", "::B", "::C", "::D", "::E", "::F"].map(|n| type_name(&mut env, n)); + assert!(matches!( + env.module_class_entry(a), + Some(ModuleClassLookup::ClassDecl(ClassDeclEntry::Class(_))) + )); + assert!(matches!( + env.module_class_entry(b), + Some(ModuleClassLookup::ClassDecl(ClassDeclEntry::Module(_))) + )); + assert!(matches!( + env.module_class_entry(c), + Some(ModuleClassLookup::ClassAlias(_)) + )); + assert!(matches!( + env.module_class_entry(d), + Some(ModuleClassLookup::ModuleAlias(_)) + )); + for (idx, name) in [a, b, c, d].into_iter().enumerate() { + let found = env.module_class_entry(name).unwrap(); + assert_eq!(found.name(), name); + assert_eq!(found.each_decl().count(), 1); + assert_eq!(env.decl(found.primary_decl(&env)), &decls[idx]); + } + assert!(env.module_class_entry(e).is_none()); + assert!(env.module_class_entry(f).is_none()); + } + + #[test] + fn class_alias_clashing_with_a_reopened_class_reports_every_declaration() { + let mut env = Environment::new(); + let decls = parse_decls( + &mut env, + "class Foo\nend\nclass Foo\nend\nclass Foo = Object\n", + ); + + add_rbs_source(&mut env, "a.rbs", decls[..2].to_vec()).unwrap(); + let err = add_rbs_source(&mut env, "b.rbs", vec![decls[2].clone()]).unwrap_err(); + + assert_duplicate( + &err, + ("b.rbs", &decls[2]), + &[("a.rbs", &decls[0]), ("a.rbs", &decls[1])], + ); + } } diff --git a/rust/ruby-rbs/src/environment/mod.rs b/rust/ruby-rbs/src/environment/mod.rs index ef4ad0e331..ce4e1f258f 100644 --- a/rust/ruby-rbs/src/environment/mod.rs +++ b/rust/ruby-rbs/src/environment/mod.rs @@ -6,8 +6,8 @@ pub mod source; pub use decl::{Context, DeclId}; pub use entry::{ - AliasEntry, ClassDeclEntry, ClassDeclKind, ClassEntry, ConstantLookup, ModuleEntry, - ModuleLookup, SingleEntry, + AliasEntry, ClassDeclEntry, ClassDeclKind, ClassEntry, ClassLookup, ConstantLookup, + ModuleClassLookup, ModuleEntry, ModuleLookup, SingleEntry, }; pub use error::{DuplicatedDecl, DuplicatedDeclarationError}; pub use source::{Source, SourceKind}; From 5eeca09bfc3d8ac77573432a2dfcb769fa72d001 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:14:19 +0900 Subject: [PATCH 09/10] Remove unnecessary comments Comments restating what the code does drift from it; the code and test names already say that. Co-Authored-By: Claude Opus 5.5 --- rust/ruby-rbs/src/environment/decl.rs | 15 --------- rust/ruby-rbs/src/environment/entry.rs | 42 ++----------------------- rust/ruby-rbs/src/environment/error.rs | 5 ++- rust/ruby-rbs/src/environment/insert.rs | 12 ------- rust/ruby-rbs/src/loader/mod.rs | 3 -- rust/ruby-rbs/tests/loader.rs | 1 - 6 files changed, 5 insertions(+), 73 deletions(-) diff --git a/rust/ruby-rbs/src/environment/decl.rs b/rust/ruby-rbs/src/environment/decl.rs index c03f67c79e..c3a8ef912b 100644 --- a/rust/ruby-rbs/src/environment/decl.rs +++ b/rust/ruby-rbs/src/environment/decl.rs @@ -3,7 +3,6 @@ use crate::ids::TypeName; use super::Environment; -/// Only valid against the [`Environment`] that issued it. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct DeclId(u32); @@ -15,9 +14,6 @@ pub(super) struct DeclNode { pub(super) context: Context, } -/// Ruby's `Resolver::context` (a cons list of `[context, name]`), interned by -/// its chain of names. Equal across reopenings of the same module. Unlike -/// Ruby's, the chain holds absolute names rather than the written ones. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct Context(Option); @@ -35,27 +31,16 @@ pub(super) struct ContextNode { } impl Environment { - /// # Panics - /// - /// May panic if `id` was issued by a different `Environment`. #[must_use] pub fn decl(&self, id: DeclId) -> &Declaration { self.member_decl(self.node(id)) } - /// # Panics - /// - /// May panic if `id` was issued by a different `Environment`. #[must_use] pub fn context_of(&self, id: DeclId) -> Context { self.node(id).context } - /// Innermost first. - /// - /// # Panics - /// - /// May panic if `context` was issued by a different `Environment`. pub fn context_names(&self, context: Context) -> impl Iterator + '_ { let node = move |i: u32| &self.contexts[i as usize]; std::iter::successors(context.0, move |&i| node(i).parent.0).map(move |i| node(i).name) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index 1725eebfcc..ed585bee36 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -14,7 +14,6 @@ pub struct SingleEntry { pub decl: DeclId, } -/// Whether a `class_decls` name is a class or a module. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum ClassDeclKind { Class, @@ -67,13 +66,7 @@ impl Entry { } impl Entry { - /// The first declaration with a superclass, else the first one. Not - /// memoized as in Ruby: the result is the same either way. - /// - /// # Panics - /// - /// Panics if the entry is empty, which registration never leaves behind. - /// May also panic if `env` is not the one that issued the entry's ids. + /// Not memoized as in Ruby: the result is the same either way. #[must_use] pub fn primary_decl(&self, env: &Environment) -> DeclId { self.decls @@ -86,9 +79,6 @@ impl Entry { } impl Entry { - /// # Panics - /// - /// Panics if the entry is empty, which registration never leaves behind. #[must_use] // No `&Environment`: only a class needs to read the declarations to pick // the one with a superclass, and that lives on `ClassEntry`, as in Ruby. @@ -151,10 +141,6 @@ impl ClassDeclEntry { } } - /// # Panics - /// - /// Panics if the entry is empty, which registration never leaves behind. - /// May also panic if `env` is not the one that issued the entry's ids. #[must_use] pub fn primary_decl(&self, env: &Environment) -> DeclId { match self { @@ -164,7 +150,6 @@ impl ClassDeclEntry { } } -/// Ruby's `class_alias_decls` values: `ModuleAliasEntry | ClassAliasEntry`. #[derive(Clone, Debug, PartialEq, Eq)] pub enum AliasEntry { Module(SingleEntry), @@ -177,10 +162,8 @@ impl SingleEntry { } } -/// What `constant_entry` finds under a name: Ruby's `ClassEntry | -/// ModuleEntry | ClassAliasEntry | ModuleAliasEntry | ConstantEntry`. Class -/// and module stay split inside `ClassDeclEntry`. Named `*Lookup` to stay -/// apart from Ruby's `ConstantEntry`, which is the `Constant` arm here. +/// Named `*Lookup` to stay apart from Ruby's `ConstantEntry`, which is the +/// `Constant` arm here. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ConstantLookup<'a> { ClassDecl(&'a ClassDeclEntry), @@ -222,12 +205,6 @@ impl<'a> ClassLookup<'a> { decls.iter().copied() } - /// An alias has the one declaration it was registered with. - /// - /// # Panics - /// - /// Panics if a class entry is empty, which registration never leaves behind. - /// May also panic if `env` is not the one that issued the entry's ids. #[must_use] pub fn primary_decl(&self, env: &Environment) -> DeclId { match self { @@ -261,13 +238,6 @@ impl<'a> ModuleClassLookup<'a> { decls.iter().copied() } - /// An alias has the one declaration it was registered with. - /// - /// # Panics - /// - /// Panics if a class or module entry is empty, which registration never - /// leaves behind. - /// May also panic if `env` is not the one that issued the entry's ids. #[must_use] pub fn primary_decl(&self, env: &Environment) -> DeclId { match self { @@ -300,11 +270,6 @@ impl<'a> ModuleLookup<'a> { decls.iter().copied() } - /// An alias has the one declaration it was registered with. - /// - /// # Panics - /// - /// Panics if a module entry is empty, which registration never leaves behind. #[must_use] pub fn primary_decl(&self) -> DeclId { match self { @@ -424,7 +389,6 @@ impl Environment { 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 index 0d7bf96cf0..08f1dfc296 100644 --- a/rust/ruby-rbs/src/environment/error.rs +++ b/rust/ruby-rbs/src/environment/error.rs @@ -34,9 +34,8 @@ impl DuplicatedDeclarationError { &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 a constant clashing with a class reports every reopening. + /// A slice rather than a pair because Ruby's error takes `*decls`, so a + /// constant clashing with a class reports every reopening. #[must_use] pub fn decls(&self) -> &[DuplicatedDecl] { &self.decls diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index e94e1738ef..7eed8091d2 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -11,10 +11,6 @@ use super::{DeclId, Environment}; type SingleTable = IdIndexMap>>; impl Environment { - /// Like Ruby, nothing is rolled back on a duplication error: the source, - /// the entries inserted before the duplicate and the reopening of - /// enclosing modules all stay. The rejected declaration and everything - /// after it are 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 = @@ -216,10 +212,6 @@ impl Environment { ) } - /// The declarations a new constant-namespace entry would clash with. - /// `class_kind` is the kind of the class/module being inserted, `None` - /// for a constant declaration. Same-kind reopening is not a clash, and - /// a class/module kind mismatch reports only the primary decl. fn constant_clash( &self, name: TypeName, @@ -328,7 +320,6 @@ mod tests { } } - // environment_test.rb:169 #[test] fn interface_twice_duplication_error() { let mut env = Environment::new(); @@ -370,16 +361,13 @@ mod tests { assert_duplicate(&err, ("a.rbs", &decls[2]), &[("a.rbs", &decls[1])]); - // 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]); - // The rejected declaration is not registered. 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)); } diff --git a/rust/ruby-rbs/src/loader/mod.rs b/rust/ruby-rbs/src/loader/mod.rs index 4fde53e4d9..dd2b516e97 100644 --- a/rust/ruby-rbs/src/loader/mod.rs +++ b/rust/ruby-rbs/src/loader/mod.rs @@ -87,9 +87,6 @@ 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. 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(); diff --git a/rust/ruby-rbs/tests/loader.rs b/rust/ruby-rbs/tests/loader.rs index 33378a6749..6cbf19738a 100644 --- a/rust/ruby-rbs/tests/loader.rs +++ b/rust/ruby-rbs/tests/loader.rs @@ -143,7 +143,6 @@ fn duplicated_declarations_are_reported_as_load_errors() { 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()); } From c3d599fc17788e4cfdc569bebe63b9e3f854aab0 Mon Sep 17 00:00:00 2001 From: dak2 <32436625+dak2@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:28:03 +0900 Subject: [PATCH 10/10] Cut allocations and rehashing on the Environment insert path Type name and string IDs are already xxh3 hashes, so the interners now use IdHasher instead of re-hashing with SipHash. concat skips the segments Vec for single-segment names, the common case. Most classes and modules are declared once, so Entry keeps the first declaration inline and allocates only for reopenings. primary_decl is fixed at registration instead of scanning declarations on every call, which also lets it drop the &Environment argument. Co-Authored-By: Claude Sonnet 5.5 --- rust/ruby-rbs/src/environment/entry.rs | 112 ++++++++++++------------ rust/ruby-rbs/src/environment/insert.rs | 30 ++++--- rust/ruby-rbs/src/ids.rs | 3 + rust/ruby-rbs/src/interner.rs | 5 +- rust/ruby-rbs/src/type_name.rs | 12 ++- 5 files changed, 87 insertions(+), 75 deletions(-) diff --git a/rust/ruby-rbs/src/environment/entry.rs b/rust/ruby-rbs/src/environment/entry.rs index ed585bee36..0b0d473820 100644 --- a/rust/ruby-rbs/src/environment/entry.rs +++ b/rust/ruby-rbs/src/environment/entry.rs @@ -1,6 +1,7 @@ +use std::iter::{Chain, Copied, Once}; use std::marker::PhantomData; +use std::slice; -use crate::ast::Declaration; use crate::ids::{SymbolId, TypeName}; use super::{DeclId, Environment}; @@ -26,12 +27,17 @@ pub struct ClassTag; #[derive(Clone, Debug, PartialEq, Eq)] pub struct ModuleTag; +pub type DeclIter<'a> = Chain, Copied>>; + /// Ruby's `ClassEntry` and `ModuleEntry` differ only in `primary_decl`, so /// they share one layout and the tag selects which `primary_decl` exists. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Entry { name: TypeName, - decls: Vec, + first: DeclId, + rest: Vec, + primary: DeclId, + primary_has_super: bool, kind: PhantomData, } @@ -39,16 +45,23 @@ pub type ClassEntry = Entry; pub type ModuleEntry = Entry; impl Entry { - pub(super) fn new(name: TypeName) -> Self { + pub(super) fn new(name: TypeName, first: DeclId, has_super: bool) -> Self { Self { name, - decls: Vec::new(), + first, + rest: Vec::new(), + primary: first, + primary_has_super: has_super, kind: PhantomData, } } - pub(super) fn push(&mut self, decl: DeclId) { - self.decls.push(decl); + pub(super) fn push(&mut self, decl: DeclId, has_super: bool) { + self.rest.push(decl); + if has_super && !self.primary_has_super { + self.primary = decl; + self.primary_has_super = true; + } } #[must_use] @@ -56,37 +69,26 @@ impl Entry { self.name } - pub fn each_decl(&self) -> impl Iterator + '_ { - self.decls().iter().copied() + pub fn each_decl(&self) -> DeclIter<'_> { + self.decls() } - fn decls(&self) -> &[DeclId] { - &self.decls + fn decls(&self) -> DeclIter<'_> { + std::iter::once(self.first).chain(self.rest.iter().copied()) } } impl Entry { - /// Not memoized as in Ruby: the result is the same either way. #[must_use] - pub fn primary_decl(&self, env: &Environment) -> DeclId { - self.decls - .iter() - .copied() - .find(|&id| matches!(env.decl(id), Declaration::Class(c) if c.super_class.is_some())) - .or_else(|| self.decls.first().copied()) - .expect("an entry always has at least one declaration") + pub fn primary_decl(&self) -> DeclId { + self.primary } } impl Entry { #[must_use] - // No `&Environment`: only a class needs to read the declarations to pick - // the one with a superclass, and that lives on `ClassEntry`, as in Ruby. pub fn primary_decl(&self) -> DeclId { - *self - .decls - .first() - .expect("an entry always has at least one declaration") + self.first } } @@ -100,17 +102,17 @@ pub enum ClassDeclEntry { } impl ClassDeclEntry { - pub(super) fn new(name: TypeName, kind: ClassDeclKind) -> Self { + pub(super) fn new(name: TypeName, kind: ClassDeclKind, first: DeclId, has_super: bool) -> Self { match kind { - ClassDeclKind::Class => Self::Class(Entry::new(name)), - ClassDeclKind::Module => Self::Module(Entry::new(name)), + ClassDeclKind::Class => Self::Class(Entry::new(name, first, has_super)), + ClassDeclKind::Module => Self::Module(Entry::new(name, first, false)), } } - pub(super) fn push(&mut self, decl: DeclId) { + pub(super) fn push(&mut self, decl: DeclId, has_super: bool) { match self { - Self::Class(e) => e.push(decl), - Self::Module(e) => e.push(decl), + Self::Class(e) => e.push(decl, has_super), + Self::Module(e) => e.push(decl, false), } } @@ -130,11 +132,11 @@ impl ClassDeclEntry { } } - pub fn each_decl(&self) -> impl Iterator + '_ { - self.decls().iter().copied() + pub fn each_decl(&self) -> DeclIter<'_> { + self.decls() } - fn decls(&self) -> &[DeclId] { + fn decls(&self) -> DeclIter<'_> { match self { Self::Class(e) => e.decls(), Self::Module(e) => e.decls(), @@ -142,9 +144,9 @@ impl ClassDeclEntry { } #[must_use] - pub fn primary_decl(&self, env: &Environment) -> DeclId { + pub fn primary_decl(&self) -> DeclId { match self { - Self::Class(e) => e.primary_decl(env), + Self::Class(e) => e.primary_decl(), Self::Module(e) => e.primary_decl(), } } @@ -157,8 +159,8 @@ pub enum AliasEntry { } impl SingleEntry { - fn decls(&self) -> &[DeclId] { - std::slice::from_ref(&self.decl) + fn decls(&self) -> DeclIter<'_> { + std::iter::once(self.decl).chain([].iter().copied()) } } @@ -173,12 +175,11 @@ pub enum ConstantLookup<'a> { } impl<'a> ConstantLookup<'a> { - pub fn each_decl(&self) -> impl Iterator + 'a { - let decls = match *self { + pub fn each_decl(&self) -> DeclIter<'a> { + match *self { Self::ClassDecl(e) => e.decls(), Self::ClassAlias(e) | Self::ModuleAlias(e) | Self::Constant(e) => e.decls(), - }; - decls.iter().copied() + } } } @@ -197,18 +198,17 @@ impl<'a> ClassLookup<'a> { } } - pub fn each_decl(&self) -> impl Iterator + 'a { - let decls = match *self { + pub fn each_decl(&self) -> DeclIter<'a> { + match *self { Self::Class(e) => e.decls(), Self::ClassAlias(e) => e.decls(), - }; - decls.iter().copied() + } } #[must_use] - pub fn primary_decl(&self, env: &Environment) -> DeclId { + pub fn primary_decl(&self) -> DeclId { match self { - Self::Class(e) => e.primary_decl(env), + Self::Class(e) => e.primary_decl(), Self::ClassAlias(e) => e.decl, } } @@ -230,18 +230,17 @@ impl<'a> ModuleClassLookup<'a> { } } - pub fn each_decl(&self) -> impl Iterator + 'a { - let decls = match *self { + pub fn each_decl(&self) -> DeclIter<'a> { + match *self { Self::ClassDecl(e) => e.decls(), Self::ClassAlias(e) | Self::ModuleAlias(e) => e.decls(), - }; - decls.iter().copied() + } } #[must_use] - pub fn primary_decl(&self, env: &Environment) -> DeclId { + pub fn primary_decl(&self) -> DeclId { match self { - Self::ClassDecl(e) => e.primary_decl(env), + Self::ClassDecl(e) => e.primary_decl(), Self::ClassAlias(e) | Self::ModuleAlias(e) => e.decl, } } @@ -262,12 +261,11 @@ impl<'a> ModuleLookup<'a> { } } - pub fn each_decl(&self) -> impl Iterator + 'a { - let decls = match *self { + pub fn each_decl(&self) -> DeclIter<'a> { + match *self { Self::Module(e) => e.decls(), Self::ModuleAlias(e) => e.decls(), - }; - decls.iter().copied() + } } #[must_use] diff --git a/rust/ruby-rbs/src/environment/insert.rs b/rust/ruby-rbs/src/environment/insert.rs index 7eed8091d2..526798f41e 100644 --- a/rust/ruby-rbs/src/environment/insert.rs +++ b/rust/ruby-rbs/src/environment/insert.rs @@ -1,3 +1,5 @@ +use indexmap::map::Entry; + use crate::ast::{ClassMember, Declaration, ModuleMember}; use crate::ids::{Id, IdIndexMap, SymbolId, TypeName}; use crate::interners::Interners; @@ -36,6 +38,7 @@ impl Environment { ClassOrModule { kind: ClassDeclKind, name: TypeName, + has_super: bool, decl_members: Vec, }, Interface(TypeName), @@ -55,11 +58,13 @@ impl Environment { Declaration::Class(c) => Kind::ClassOrModule { kind: ClassDeclKind::Class, name: c.name, + has_super: c.super_class.is_some(), decl_members: decl_member_indices(&c.members, ClassMember::as_declaration), }, Declaration::Module(m) => Kind::ClassOrModule { kind: ClassDeclKind::Module, name: m.name, + has_super: false, decl_members: decl_member_indices(&m.members, ModuleMember::as_declaration), }, Declaration::Interface(d) => Kind::Interface(d.name), @@ -74,6 +79,7 @@ impl Environment { Kind::ClassOrModule { kind, name, + has_super, decl_members, } => { let name = self.interners.type_names.concat(namespace, name); @@ -81,10 +87,12 @@ impl Environment { return Err(self.duplicated_type_name(name, &node, first, rest)); } let id = self.push_node(node); - self.class_decls - .entry(name) - .or_insert_with(|| ClassDeclEntry::new(name, kind)) - .push(id); + match self.class_decls.entry(name) { + Entry::Occupied(mut e) => e.get_mut().push(id, has_super), + Entry::Vacant(e) => { + e.insert(ClassDeclEntry::new(name, kind, id, has_super)); + } + } self.insert_members(node.source, node.context, id, name, &decl_members) } Kind::Interface(name) => { @@ -219,7 +227,7 @@ impl Environment { ) -> Option<(DeclId, Vec)> { match (self.constant_entry(name)?, class_kind) { (ConstantLookup::ClassDecl(e), Some(kind)) => { - (e.kind() != kind).then(|| (e.primary_decl(self), Vec::new())) + (e.kind() != kind).then(|| (e.primary_decl(), Vec::new())) } (entry, _) => { let mut ids = entry.each_decl(); @@ -693,7 +701,7 @@ mod tests { let string = type_name(&mut env, "String"); let entry = env.class_entry(foo).unwrap(); assert_eq!(entry.each_decl().count(), 2); - let primary = env.decl(entry.primary_decl(&env)); + let primary = env.decl(entry.primary_decl()); assert_eq!(primary, &decls[1]); let Declaration::Class(primary) = primary else { panic!("expected a class declaration"); @@ -712,7 +720,7 @@ mod tests { let foo = type_name(&mut env, "::Foo"); let entry = env.class_entry(foo).unwrap(); - assert_eq!(env.decl(entry.primary_decl(&env)), &decls[0]); + assert_eq!(env.decl(entry.primary_decl()), &decls[0]); } #[test] @@ -797,7 +805,7 @@ mod tests { assert!(env.class_entry(bar).is_some()); assert!(env.module_entry(bar).is_none()); let names: Vec<_> = env - .context_names(env.context_of(env.class_entry(bar).unwrap().primary_decl(&env))) + .context_names(env.context_of(env.class_entry(bar).unwrap().primary_decl())) .collect(); assert_eq!(names, [foo]); } @@ -1141,11 +1149,11 @@ mod tests { let class = env.class_entry(a).unwrap(); assert!(matches!(class, ClassLookup::Class(_))); assert_eq!(class.name(), a); - assert_eq!(env.decl(class.primary_decl(&env)), &decls[0]); + assert_eq!(env.decl(class.primary_decl()), &decls[0]); let alias = env.class_entry(b).unwrap(); assert!(matches!(alias, ClassLookup::ClassAlias(_))); assert_eq!(alias.name(), b); - assert_eq!(env.decl(alias.primary_decl(&env)), &decls[1]); + assert_eq!(env.decl(alias.primary_decl()), &decls[1]); assert_eq!(alias.each_decl().count(), 1); for name in [c, d, e, f] { assert!(env.class_entry(name).is_none()); @@ -1193,7 +1201,7 @@ mod tests { let found = env.module_class_entry(name).unwrap(); assert_eq!(found.name(), name); assert_eq!(found.each_decl().count(), 1); - assert_eq!(env.decl(found.primary_decl(&env)), &decls[idx]); + assert_eq!(env.decl(found.primary_decl()), &decls[idx]); } assert!(env.module_class_entry(e).is_none()); assert!(env.module_class_entry(f).is_none()); diff --git a/rust/ruby-rbs/src/ids.rs b/rust/ruby-rbs/src/ids.rs index 50a6aaf688..efcd444a50 100644 --- a/rust/ruby-rbs/src/ids.rs +++ b/rust/ruby-rbs/src/ids.rs @@ -151,6 +151,9 @@ impl Hasher for IdHasher { } } +/// `HashMap` keyed by `Id` that skips re-hashing; see [`IdHasher`]. +pub(crate) type IdHashMap = std::collections::HashMap, V, BuildHasherDefault>; + /// Insertion-ordered like Ruby's `Hash`; `Environment` consumers depend on /// that order for deterministic results. /// diff --git a/rust/ruby-rbs/src/interner.rs b/rust/ruby-rbs/src/interner.rs index 16f0a97850..644e909eab 100644 --- a/rust/ruby-rbs/src/interner.rs +++ b/rust/ruby-rbs/src/interner.rs @@ -27,8 +27,7 @@ //! assert_eq!(global.resolve(b_array), "Array"); //! ``` -use crate::ids::SymbolId; -use std::collections::HashMap; +use crate::ids::{IdHashMap, SymbolId, SymbolTag}; use xxhash_rust::xxh3::xxh3_64; /// Interns strings and assigns each the content-addressed [`SymbolId`] @@ -40,7 +39,7 @@ use xxhash_rust::xxh3::xxh3_64; /// [`merge`]: Self::merge #[derive(Default)] pub struct StringInterner { - map: HashMap>, + map: IdHashMap>, } impl StringInterner { diff --git a/rust/ruby-rbs/src/type_name.rs b/rust/ruby-rbs/src/type_name.rs index eadb497d9d..9b3ac17564 100644 --- a/rust/ruby-rbs/src/type_name.rs +++ b/rust/ruby-rbs/src/type_name.rs @@ -31,9 +31,8 @@ //! assert_eq!(names.display(foo, &strings), "::RBS::Foo"); //! ``` -use crate::ids::{SymbolId, TypeName}; +use crate::ids::{IdHashMap, SymbolId, TypeName, TypeNameTag}; use crate::interner::StringInterner; -use std::collections::HashMap; use xxhash_rust::xxh3::xxh3_64; /// Fixed sentinel hash for the absolute (`::`) namespace root. Chosen so @@ -123,7 +122,7 @@ impl Entry { /// assert_eq!(names.display(n2, &strings), "::RBS::Foo"); /// ``` pub struct TypeNameInterner { - entries: HashMap, + entries: IdHashMap, relative_root: TypeName, absolute_root: TypeName, } @@ -132,7 +131,7 @@ impl Default for TypeNameInterner { fn default() -> Self { let relative_root = TypeName::from_hash(RELATIVE_ROOT_HASH); let absolute_root = TypeName::from_hash(ABSOLUTE_ROOT_HASH); - let mut entries = HashMap::new(); + let mut entries = IdHashMap::::default(); entries.insert( relative_root, Entry::Relative(RelativeTypeNameEntry { @@ -298,6 +297,11 @@ impl TypeNameInterner { if self.is_absolute(tail) { return tail; } + if let (Some(parent), Some(segment)) = (self.parent(tail), self.last_segment(tail)) + && self.is_root(parent) + { + return self.append(head, segment); + } let tail_segs = self.segments(tail); self.extend(head, tail_segs) }