From c22b0738c225e604939f1481862d09f58f4866d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Sun, 21 Jun 2026 15:14:00 +0200 Subject: [PATCH] fix: stop versioned go imports colliding on version segment --- bindgen/internal/emit/emitter.go | 15 +++- .../major_version_self_named/v2/v2.go | 9 ++ .../major_version_self_named/v2.d.lis | 10 +++ crates/emit/src/analyze/facts.rs | 4 + crates/emit/src/lib.rs | 7 +- crates/emit/src/names/resolution.rs | 8 +- crates/emit/src/output/imports.rs | 36 ++++++-- crates/semantics/src/cache/go_stdlib.rs | 1 - crates/semantics/src/checker/mod.rs | 15 +--- .../semantics/src/checker/registration/mod.rs | 4 +- crates/semantics/src/store.rs | 3 +- crates/syntax/src/program/file.rs | 87 ++++++++++++++++-- crates/syntax/src/program/mod.rs | 2 +- tests/spec/build/mod.rs | 88 ++++++++++++++++++- tests/spec/emit/imports.rs | 37 ++++++++ tests/spec/infer/types.rs | 54 ++++++++++++ 16 files changed, 335 insertions(+), 45 deletions(-) create mode 100644 bindgen/tests/testdata/fixtures/major_version_self_named/v2/v2.go create mode 100644 bindgen/tests/testdata/snapshots/major_version_self_named/v2.d.lis diff --git a/bindgen/internal/emit/emitter.go b/bindgen/internal/emit/emitter.go index c2d2b0cee..7cd1730dd 100644 --- a/bindgen/internal/emit/emitter.go +++ b/bindgen/internal/emit/emitter.go @@ -3,6 +3,7 @@ package emit import ( "fmt" "slices" + "strconv" "strings" "github.com/ivov/lisette/bindgen/internal/config" @@ -60,7 +61,9 @@ func NewEmitter(cfg *config.Config, pkgPath, pkgName string, bitFlagSetTypes, cl func (e *Emitter) EmitHeader(pkg, pkgName, lisetteVersion, goVersion string) { e.buf.WriteString("// Generated by Lisette bindgen\n") fmt.Fprintf(&e.buf, "// Source: %s (%s)\n", pkg, packageSourceType(pkg)) - if lastSegment := pkg[strings.LastIndex(pkg, "/")+1:]; pkgName != lastSegment { + // Lisette derives a Go import's local name from the segment before a `/vN` + // suffix, so a package actually named `vN` must declare `// Package:` itself. + if lastSegment := pkg[strings.LastIndex(pkg, "/")+1:]; pkgName != lastSegment || hasMajorVersionSuffix(pkg) { fmt.Fprintf(&e.buf, "// Package: %s\n", pkgName) } fmt.Fprintf(&e.buf, "// Go: %s\n", strings.TrimPrefix(goVersion, "go")) @@ -68,6 +71,16 @@ func (e *Emitter) EmitHeader(pkg, pkgName, lisetteVersion, goVersion string) { e.buf.WriteString("\n") } +// hasMajorVersionSuffix reports whether the path ends in a `/v2`+ major-version suffix. +func hasMajorVersionSuffix(pkgPath string) bool { + last := pkgPath[strings.LastIndex(pkgPath, "/")+1:] + if len(last) < 2 || last[0] != 'v' { + return false + } + major, err := strconv.Atoi(last[1:]) + return err == nil && major >= 2 +} + func packageSourceType(pkg string) string { if strings.HasPrefix(pkg, "/") || strings.HasPrefix(pkg, "./") || strings.HasPrefix(pkg, "../") { return "local" diff --git a/bindgen/tests/testdata/fixtures/major_version_self_named/v2/v2.go b/bindgen/tests/testdata/fixtures/major_version_self_named/v2/v2.go new file mode 100644 index 000000000..dadfd48d6 --- /dev/null +++ b/bindgen/tests/testdata/fixtures/major_version_self_named/v2/v2.go @@ -0,0 +1,9 @@ +// Package v2 sits at a `/v2` path but is genuinely named `v2` (like the +// `k8s.io/api/.../v2` packages), so bindgen must emit an explicit +// `// Package: v2` directive even though it matches the last path segment. +package v2 + +// Config is a marker type to give the package an exported surface. +type Config struct { + Name string +} diff --git a/bindgen/tests/testdata/snapshots/major_version_self_named/v2.d.lis b/bindgen/tests/testdata/snapshots/major_version_self_named/v2.d.lis new file mode 100644 index 000000000..dafa3bcbf --- /dev/null +++ b/bindgen/tests/testdata/snapshots/major_version_self_named/v2.d.lis @@ -0,0 +1,10 @@ +// Generated by Lisette bindgen +// Source: ./testdata/fixtures/major_version_self_named/v2 (local) +// Package: v2 +// Go: 0.0.0 +// Lisette: 0.0.0 + +/// Config is a marker type to give the package an exported surface. +pub struct Config { + pub Name: string, +} diff --git a/crates/emit/src/analyze/facts.rs b/crates/emit/src/analyze/facts.rs index 39310c068..75f04852c 100644 --- a/crates/emit/src/analyze/facts.rs +++ b/crates/emit/src/analyze/facts.rs @@ -216,6 +216,10 @@ impl<'a> EmitFacts<'a> { self.go_package_names } + pub(crate) fn go_module_ids(&self) -> &'a HashSet { + self.go_module_ids + } + pub(crate) fn has_global_exported_method_name(&self, method: &str) -> bool { self.globals.exported_method_names.contains(method) } diff --git a/crates/emit/src/lib.rs b/crates/emit/src/lib.rs index 418fda775..5d5a88c3a 100644 --- a/crates/emit/src/lib.rs +++ b/crates/emit/src/lib.rs @@ -506,8 +506,11 @@ impl<'a> Planner<'a> { } } - let mut import_builder = - ImportBuilder::from_plan(&file_plan.imports, self.facts.go_package_names()); + let mut import_builder = ImportBuilder::from_plan( + &file_plan.imports, + self.facts.go_package_names(), + self.facts.go_module_ids(), + ); self.drain_file_emission_into(&mut source); fx.drain_into(&mut import_builder); diff --git a/crates/emit/src/names/resolution.rs b/crates/emit/src/names/resolution.rs index 411efdb36..afbb9b1c9 100644 --- a/crates/emit/src/names/resolution.rs +++ b/crates/emit/src/names/resolution.rs @@ -119,10 +119,10 @@ impl Planner<'_> { if let Some(pkg_name) = self.facts.go_package_name(module) { return pkg_name.to_string(); } - let path = module - .strip_prefix(go_name::GO_IMPORT_PREFIX) - .unwrap_or(module); - go_name::go_package_name(path).to_string() + match module.strip_prefix(go_name::GO_IMPORT_PREFIX) { + Some(go_path) => syntax::program::go_import_default_name(go_path).to_string(), + None => go_name::go_package_name(module).to_string(), + } } pub(crate) fn qualify_method_call( diff --git a/crates/emit/src/output/imports.rs b/crates/emit/src/output/imports.rs index 9fe8e48f0..1d3d2692a 100644 --- a/crates/emit/src/output/imports.rs +++ b/crates/emit/src/output/imports.rs @@ -51,6 +51,7 @@ impl ImportPlan { pub struct ImportBuilder<'a> { go_package_names: &'a HashMap, + go_module_ids: &'a HashSet, imports: HashMap, /// Generated imports of paths the source already imports under another /// alias; emitted alongside (Go permits duplicate import paths). @@ -60,9 +61,13 @@ pub struct ImportBuilder<'a> { } impl<'a> ImportBuilder<'a> { - pub fn new(go_package_names: &'a HashMap) -> Self { + pub fn new( + go_package_names: &'a HashMap, + go_module_ids: &'a HashSet, + ) -> Self { Self { go_package_names, + go_module_ids, imports: HashMap::default(), generated_duplicates: Vec::new(), dropped_aliases: HashMap::default(), @@ -73,9 +78,11 @@ impl<'a> ImportBuilder<'a> { pub(crate) fn from_plan( plan: &ImportPlan, go_package_names: &'a HashMap, + go_module_ids: &'a HashSet, ) -> Self { Self { go_package_names, + go_module_ids, imports: plan.imports.clone(), generated_duplicates: Vec::new(), dropped_aliases: plan.dropped_aliases.clone(), @@ -108,7 +115,8 @@ impl<'a> ImportBuilder<'a> { let canonical = package.qualifier(); self.used_modules.insert(path.to_string()); match self.imports.get(path) { - Some(alias) if effective_package_name(path, alias) == canonical => {} + Some(alias) if effective_package_name(path, alias, self.go_module_ids) == canonical => { + } Some(_) => { if !self.generated_duplicates.iter().any(|(p, _)| p == path) { self.generated_duplicates @@ -131,12 +139,15 @@ impl<'a> ImportBuilder<'a> { entries.extend(self.generated_duplicates); entries.sort(); entries.dedup(); - let diagnostics = detect_collisions(&entries); + let diagnostics = detect_collisions(&entries, self.go_module_ids); (entries, diagnostics) } } -fn detect_collisions(entries: &[(String, String)]) -> Vec { +fn detect_collisions( + entries: &[(String, String)], + go_module_ids: &HashSet, +) -> Vec { if entries.len() < 2 { return Vec::new(); } @@ -145,7 +156,7 @@ fn detect_collisions(entries: &[(String, String)]) -> Vec { if alias == "_" { continue; } - let effective = effective_package_name(path, alias); + let effective = effective_package_name(path, alias, go_module_ids); let sanitized = go_name::sanitize_package_name(effective).into_owned(); groups.entry(sanitized).or_default().push(path.as_str()); } @@ -160,11 +171,18 @@ fn detect_collisions(entries: &[(String, String)]) -> Vec { .collect() } -fn effective_package_name<'a>(path: &'a str, alias: &'a str) -> &'a str { - if alias.is_empty() { - path.rsplit('/').next().unwrap_or(path) +fn effective_package_name<'a>( + path: &'a str, + alias: &'a str, + go_module_ids: &HashSet, +) -> &'a str { + if !alias.is_empty() { + return alias; + } + if go_module_ids.contains(&format!("{}{path}", go_name::GO_IMPORT_PREFIX)) { + syntax::program::go_import_default_name(path) } else { - alias + path.rsplit('/').next().unwrap_or(path) } } diff --git a/crates/semantics/src/cache/go_stdlib.rs b/crates/semantics/src/cache/go_stdlib.rs index c119cf300..366ee38b5 100644 --- a/crates/semantics/src/cache/go_stdlib.rs +++ b/crates/semantics/src/cache/go_stdlib.rs @@ -160,7 +160,6 @@ fn register_cached_go_module( if let Some(source) = source && let Some(pkg_name) = extract_package_directive(source) - && module_id.rsplit('/').next() != Some(pkg_name.as_str()) { store .go_package_names diff --git a/crates/semantics/src/checker/mod.rs b/crates/semantics/src/checker/mod.rs index bf0ad0b29..7e558301b 100644 --- a/crates/semantics/src/checker/mod.rs +++ b/crates/semantics/src/checker/mod.rs @@ -18,7 +18,9 @@ use ecow::EcoString; use scopes::Scopes; use syntax::ast::Visibility as AstVisibility; use syntax::ast::{Annotation, Expression, Generic, ImportAlias, Span, StructFieldDefinition}; -use syntax::program::{Definition, DefinitionBody, File, FileImport, MethodSignatures, Module}; +use syntax::program::{ + Definition, DefinitionBody, File, FileImport, MethodSignatures, Module, go_import_default_name, +}; use syntax::types::{SubstitutionMap, Symbol, Type, substitute}; pub use infer::expressions::comparison::check_not_comparable; @@ -708,7 +710,7 @@ impl<'s> TaskState<'s> { .go_package_names .get(module_id) .cloned() - .unwrap_or_else(|| go_module_last_segment(module_id).to_string()); + .unwrap_or_else(|| go_import_default_name(module_id).to_string()); self.imports .prefix_to_module .insert(self_alias, module_id.into()); @@ -909,15 +911,6 @@ impl<'s> TaskState<'s> { } } -fn go_module_last_segment(module_id: &str) -> &str { - module_id - .strip_prefix("go:") - .unwrap_or(module_id) - .rsplit('/') - .next() - .unwrap_or(module_id) -} - /// Returns `true` if the given name is reserved and cannot be used as an import alias. /// /// Reserved names include Go keywords, Go predeclared identifiers, Go builtins, diff --git a/crates/semantics/src/checker/registration/mod.rs b/crates/semantics/src/checker/registration/mod.rs index 7950e2577..544305011 100644 --- a/crates/semantics/src/checker/registration/mod.rs +++ b/crates/semantics/src/checker/registration/mod.rs @@ -276,9 +276,7 @@ impl TaskState<'_> { store.mark_visited(module_id); store.add_module(module_id); - if let Some(pkg_name) = extract_package_directive(source) - && module_id.rsplit('/').next() != Some(pkg_name.as_str()) - { + if let Some(pkg_name) = extract_package_directive(source) { store .go_package_names .insert(module_id.to_string(), pkg_name); diff --git a/crates/semantics/src/store.rs b/crates/semantics/src/store.rs index bd953893e..f969f5df0 100644 --- a/crates/semantics/src/store.rs +++ b/crates/semantics/src/store.rs @@ -104,8 +104,7 @@ pub struct Store { pub module_ids: Vec, /// file ID -> module ID pub files: HashMap, - /// Go module ID -> Go package name, from the typedef `// Package:` directive. - /// Present only when the package name differs from the final path segment. + /// Go module ID -> package name from the typedef `// Package:` directive. pub go_package_names: HashMap, /// File ID -> on-disk path of the `.d.lis` typedef. Lets the LSP map go: typedef /// file IDs to the actual cache path so go-to-definition can navigate there. diff --git a/crates/syntax/src/program/file.rs b/crates/syntax/src/program/file.rs index da6c4bbf6..9c01b2ab0 100644 --- a/crates/syntax/src/program/file.rs +++ b/crates/syntax/src/program/file.rs @@ -39,20 +39,35 @@ impl FileImport { if let Some(pkg_name) = go_package_names.get(self.name.as_str()) { return Some(pkg_name.clone()); } - Some( - self.name - .strip_prefix("go:") - .unwrap_or(&self.name) - .split('/') - .next_back() - .unwrap_or(&self.name) - .to_string(), - ) + let default = match self.name.strip_prefix("go:") { + Some(go_path) => go_import_default_name(go_path), + None => self.name.rsplit('/').next().unwrap_or(&self.name), + }; + Some(default.to_string()) } } } } +pub fn go_import_default_name(import_path: &str) -> &str { + let path = import_path.strip_prefix("go:").unwrap_or(import_path); + let mut segments = path.rsplit('/'); + let last = segments.next().unwrap_or(path); + if is_major_version_segment(last) + && let Some(preceding) = segments.next() + { + return preceding; + } + last +} + +fn is_major_version_segment(segment: &str) -> bool { + segment + .strip_prefix('v') + .and_then(|digits| digits.parse::().ok()) + .is_some_and(|major| major >= 2) +} + impl File { pub fn new( module_id: &str, @@ -132,3 +147,57 @@ impl File { .to_string() } } + +#[cfg(test)] +mod tests { + use super::go_import_default_name; + + #[test] + fn major_version_suffix_resolves_to_preceding_segment() { + assert_eq!(go_import_default_name("github.com/pion/sdp/v3"), "sdp"); + assert_eq!( + go_import_default_name("go:github.com/pion/webrtc/v4"), + "webrtc" + ); + assert_eq!( + go_import_default_name("go:github.com/pion/transport/v4"), + "transport" + ); + } + + #[test] + fn non_version_last_segment_is_kept() { + assert_eq!(go_import_default_name("go:strings"), "strings"); + assert_eq!( + go_import_default_name("go:github.com/pion/datachannel"), + "datachannel" + ); + assert_eq!( + go_import_default_name("go:github.com/pion/transport/v4/packetio"), + "packetio" + ); + } + + #[test] + fn v0_and_v1_are_ordinary_segments() { + assert_eq!(go_import_default_name("go:k8s.io/api/core/v1"), "v1"); + assert_eq!(go_import_default_name("go:example.com/pkg/v0"), "v0"); + } + + #[test] + fn dotted_version_suffix_is_not_a_major_version_segment() { + assert_eq!(go_import_default_name("go:gopkg.in/yaml.v3"), "yaml.v3"); + } + + #[test] + fn version_like_segment_without_preceding_segment_is_kept() { + assert_eq!(go_import_default_name("v2"), "v2"); + assert_eq!(go_import_default_name("go:v2"), "v2"); + } + + #[test] + fn bare_v_or_non_numeric_is_not_a_version() { + assert_eq!(go_import_default_name("go:example.com/foo/v"), "v"); + assert_eq!(go_import_default_name("go:example.com/foo/vx"), "vx"); + } +} diff --git a/crates/syntax/src/program/mod.rs b/crates/syntax/src/program/mod.rs index e168b008e..86f3385ae 100644 --- a/crates/syntax/src/program/mod.rs +++ b/crates/syntax/src/program/mod.rs @@ -11,6 +11,6 @@ pub use emit_input::{ EmitInput, EqualityIndex, EqualityInfo, EqualityUnusableReason, MutationInfo, TestFunction, TestIndex, UnusedInfo, }; -pub use file::{File, FileImport}; +pub use file::{File, FileImport, go_import_default_name}; pub use module::{Module, ModuleId, ModuleInfo}; pub use resolution::{CallKind, DotAccessKind, NativeTypeKind, ReceiverCoercion}; diff --git a/tests/spec/build/mod.rs b/tests/spec/build/mod.rs index 2bbbe748c..4caf0a344 100644 --- a/tests/spec/build/mod.rs +++ b/tests/spec/build/mod.rs @@ -5790,7 +5790,12 @@ fn go_import_collision_flags_shared_last_segment() { use rustc_hash::FxHashMap as HashMap; let go_package_names: HashMap = HashMap::default(); - let mut builder = emit::imports::ImportBuilder::new(&go_package_names); + let go_module_ids: rustc_hash::FxHashSet = + ["go:database/sql", "go:entgo.io/ent/dialect/sql"] + .iter() + .map(|s| s.to_string()) + .collect(); + let mut builder = emit::imports::ImportBuilder::new(&go_package_names, &go_module_ids); builder.extend_with_modules( &["database/sql", "entgo.io/ent/dialect/sql"] .iter() @@ -5818,8 +5823,13 @@ fn go_import_collision_silent_when_aliases_differ() { "go:entgo.io/ent/dialect/sql".to_string(), "entsql".to_string(), ); + let go_module_ids: rustc_hash::FxHashSet = + ["go:database/sql", "go:entgo.io/ent/dialect/sql"] + .iter() + .map(|s| s.to_string()) + .collect(); - let mut builder = emit::imports::ImportBuilder::new(&go_package_names); + let mut builder = emit::imports::ImportBuilder::new(&go_package_names, &go_module_ids); builder.extend_with_modules( &["database/sql", "entgo.io/ent/dialect/sql"] .iter() @@ -5835,6 +5845,80 @@ fn go_import_collision_silent_when_aliases_differ() { ); } +#[test] +fn go_import_collision_silent_for_distinct_versioned_modules() { + use rustc_hash::FxHashMap as HashMap; + + let go_package_names: HashMap = HashMap::default(); + let go_module_ids: rustc_hash::FxHashSet = + ["go:github.com/pion/sdp/v3", "go:github.com/pion/dtls/v3"] + .iter() + .map(|s| s.to_string()) + .collect(); + let mut builder = emit::imports::ImportBuilder::new(&go_package_names, &go_module_ids); + builder.extend_with_modules( + &["github.com/pion/sdp/v3", "github.com/pion/dtls/v3"] + .iter() + .map(|s| s.to_string()) + .collect(), + ); + + let (_imports, diagnostics) = builder.build(); + assert!( + diagnostics.is_empty(), + "distinct `/v3` modules must not collide, got: {:?}", + diagnostics.iter().map(|d| d.code_str()).collect::>() + ); +} + +#[test] +fn go_import_collision_flags_local_modules_sharing_last_segment() { + use rustc_hash::FxHashMap as HashMap; + + let go_package_names: HashMap = HashMap::default(); + let go_module_ids: rustc_hash::FxHashSet = rustc_hash::FxHashSet::default(); + let mut builder = emit::imports::ImportBuilder::new(&go_package_names, &go_module_ids); + builder.extend_with_modules( + &["myproject/api/v2", "myproject/admin/v2"] + .iter() + .map(|s| s.to_string()) + .collect(), + ); + + let (_imports, diagnostics) = builder.build(); + assert_eq!( + diagnostics.len(), + 1, + "local modules both packaging as `v2` must collide, got: {:?}", + diagnostics.iter().map(|d| d.code_str()).collect::>() + ); +} + +#[test] +fn go_import_under_project_module_resolves_by_package_not_version() { + use rustc_hash::FxHashMap as HashMap; + + let go_package_names: HashMap = HashMap::default(); + let go_module_ids: rustc_hash::FxHashSet = ["go:myproject/plugins/v2"] + .iter() + .map(|s| s.to_string()) + .collect(); + let mut builder = emit::imports::ImportBuilder::new(&go_package_names, &go_module_ids); + builder.extend_with_modules( + &["myproject/plugins/v2", "myproject/api/v2"] + .iter() + .map(|s| s.to_string()) + .collect(), + ); + + let (_imports, diagnostics) = builder.build(); + assert!( + diagnostics.is_empty(), + "a go: import under the project module must resolve by package name, got: {:?}", + diagnostics.iter().map(|d| d.code_str()).collect::>() + ); +} + #[test] fn write_only_mut_param_keeps_name_in_codegen_via_reassignment() { let mut fs = MockFileSystem::new(); diff --git a/tests/spec/emit/imports.rs b/tests/spec/emit/imports.rs index 584098c19..658397e2c 100644 --- a/tests/spec/emit/imports.rs +++ b/tests/spec/emit/imports.rs @@ -296,6 +296,43 @@ pub struct Style {} ); } +#[test] +fn distinct_versioned_modules_do_not_falsely_collide() { + let sdp = r#" +pub struct SessionDescription {} +"#; + let dtls = r#" +pub struct Config {} +"#; + let input = r#" +import "go:example.com/sdp/v3" +import "go:example.com/dtls/v3" + +fn make() { + let _ = sdp.SessionDescription {} + let _ = dtls.Config {} +} +"#; + let result = crate::_harness::emit::emit_with_go_typedefs( + input, + &[ + ("go:example.com/sdp/v3", sdp), + ("go:example.com/dtls/v3", dtls), + ], + ); + let collisions: Vec<_> = result + .files + .iter() + .flat_map(|file| &file.diagnostics) + .filter(|diagnostic| diagnostic.code_str() == Some("emit.go_import_collision")) + .collect(); + assert!( + collisions.is_empty(), + "distinct `/v3` modules must not collide, got: {:?}", + collisions + ); +} + #[test] fn go_type_uses_declared_package_name_not_path_segment() { let input = r#" diff --git a/tests/spec/infer/types.rs b/tests/spec/infer/types.rs index a94b440de..1fbf37e10 100644 --- a/tests/spec/infer/types.rs +++ b/tests/spec/infer/types.rs @@ -7807,3 +7807,57 @@ fn main() { ) .assert_no_errors(); } + +#[test] +fn versioned_go_modules_do_not_falsely_conflict_when_typedefs_missing() { + let webrtc = r#"// Package: webrtc + +import "go:example.com/sdp/v3" +import "go:example.com/dtls/v3" + +pub struct PeerConnection {} +"#; + let input = r#" +import "go:example.com/webrtc/v4" + +fn main() { + let _ = webrtc.PeerConnection {} +} +"#; + let result = infer_with_go_typedefs(input, &[("go:example.com/webrtc/v4", webrtc)]); + assert!( + !result + .errors + .iter() + .any(|error| error.code_str() == Some("resolve.import_conflict")), + "expected no import conflict for distinct `/vN` modules, got: {:?}", + result.errors + ); +} + +#[test] +fn versioned_go_modules_resolve_to_preceding_segment_without_directive() { + let sdp = r#" +pub struct SessionDescription {} +"#; + let dtls = r#" +pub struct Config {} +"#; + let input = r#" +import "go:example.com/sdp/v3" +import "go:example.com/dtls/v3" + +fn main() { + let _ = sdp.SessionDescription {} + let _ = dtls.Config {} +} +"#; + infer_with_go_typedefs( + input, + &[ + ("go:example.com/sdp/v3", sdp), + ("go:example.com/dtls/v3", dtls), + ], + ) + .assert_no_errors(); +}