Skip to content

Resolve conflicting implementations for two similarly named types #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
swthorn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Resolve conflicting implementations for two similarly named types #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions rasn-compiler/src/generator/rasn/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ macro_rules! call_template {
($this:ident, $fn:ident, $tld:ident, $($args:expr),*) => {
Ok($fn(
$this.format_comments(&$tld.comments)?,
$this.to_rust_const_case(&$tld.name),
$this.to_rust_const_case_unique(&$tld.name),
$($args),*
))
};
Expand Down Expand Up @@ -132,15 +132,15 @@ impl Rasn {
if integer_type.is_unbounded() {
Ok(lazy_static_value_template(
self.format_comments(&tld.comments)?,
self.to_rust_const_case(&tld.name),
self.to_rust_const_case_unique(&tld.name),
ty,
val,
self.config.no_std_compliant_bindings,
))
} else {
Ok(integer_value_template(
self.format_comments(&tld.comments)?,
self.to_rust_const_case(&tld.name),
self.to_rust_const_case_unique(&tld.name),
ty,
val,
))
Expand Down Expand Up @@ -502,9 +502,11 @@ impl Rasn {
&self,
tld: ToplevelTypeDefinition,
) -> Result<TokenStream, GeneratorError> {
let name = self.to_rust_title_case(&tld.name);
let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
let mut annotations = vec![quote!(delegate), self.format_tag(tld.tag.as_ref(), false)];
if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {
annotations.push(self.format_identifier_annotation(&tld.name, &tld.comments, &tld.ty));
}
Ok(any_template(
Expand Down Expand Up @@ -595,10 +597,11 @@ impl Rasn {
#[non_exhaustive]}
})
.unwrap_or_default();
let name = self.to_rust_title_case(&tld.name);
let name = self.to_rust_title_case_unique(&tld.name);
let mut annotations =
vec![quote!(enumerated), self.format_tag(tld.tag.as_ref(), false)];
if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name || name_str != self.to_rust_title_case(&tld.name).to_string() {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition name_str != self.to_rust_title_case(&tld.name).to_string() is redundant because name_str is derived from to_rust_title_case_unique(&tld.name), which internally calls to_rust_title_case(&tld.name). When there's no collision, both will be identical. The actual check needed is whether the name was modified due to a collision (i.e., whether a number was appended).

Consider storing the base name (before uniquification) and comparing against it instead:

let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {

This pattern is correctly implemented in format_name_and_common_annotations at line 1355-1360 in utils.rs.

Copilot uses AI. Check for mistakes.
annotations.push(self.format_identifier_annotation(
&tld.name,
&tld.comments,
Expand All @@ -622,7 +625,7 @@ impl Rasn {
tld: ToplevelTypeDefinition,
) -> Result<TokenStream, GeneratorError> {
if let ASN1Type::Choice(ref choice) = tld.ty {
let name = self.to_rust_title_case(&tld.name);
let name = self.to_rust_title_case_unique(&tld.name);
let extensible = choice
.extensible
.or(
Expand All @@ -642,7 +645,8 @@ impl Rasn {
&& !choice.options.iter().any(|o| o.tag.is_some()),
),
];
if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name || name_str != self.to_rust_title_case(&tld.name).to_string() {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as line 604: the condition name_str != self.to_rust_title_case(&tld.name).to_string() is redundant. The check should compare name_str against the base name (before uniquification) to determine if a number was appended due to a name collision.

The correct pattern is:

let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {
Suggested change
if name_str != tld.name || name_str != self.to_rust_title_case(&tld.name).to_string() {
let base_name = self.to_rust_title_case(&tld.name).to_string();
if name_str != tld.name || name_str != base_name {

Copilot uses AI. Check for mistakes.
annotations.push(self.format_identifier_annotation(
&tld.name,
&tld.comments,
Expand Down Expand Up @@ -701,7 +705,7 @@ impl Rasn {
) -> Result<TokenStream, GeneratorError> {
match tld.ty {
ASN1Type::Sequence(ref seq) | ASN1Type::Set(ref seq) => {
let name = self.to_rust_title_case(&tld.name);
let name = self.to_rust_title_case_unique(&tld.name);
let extensible = seq
.extensible
.or(
Expand Down Expand Up @@ -775,7 +779,10 @@ impl Rasn {
&& !seq.members.iter().any(|m| m.tag.is_some()),
),
];
if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name
|| name_str != self.to_rust_title_case(&tld.name).to_string()
{
Comment on lines +782 to +785
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as lines 604 and 649: the condition at line 784 is redundant. The check should compare name_str against the base name (before uniquification) to determine if a number was appended due to a name collision.

The correct pattern is:

let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
// ... later in the code
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {
Suggested change
let name_str = name.to_string();
if name_str != tld.name
|| name_str != self.to_rust_title_case(&tld.name).to_string()
{
let base_name = self.to_rust_title_case(&tld.name).to_string();
let name_str = name.to_string();
if name_str != base_name {

Copilot uses AI. Check for mistakes.
annotations.push(self.format_identifier_annotation(
&tld.name,
&tld.comments,
Expand Down Expand Up @@ -814,7 +821,7 @@ impl Rasn {
))
}
};
let name = self.to_rust_title_case(&tld.name);
let name = self.to_rust_title_case_unique(&tld.name);
let anonymous_item = match seq_or_set_of.element_type.as_ref() {
ASN1Type::ElsewhereDeclaredType(_) => None,
n => Some(
Expand Down Expand Up @@ -843,7 +850,8 @@ impl Rasn {
self.format_range_annotations(true, &seq_or_set_of.constraints)?,
self.format_tag(tld.tag.as_ref(), false),
];
if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name || name_str != self.to_rust_title_case(&tld.name).to_string() {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as previous cases: the condition name_str != self.to_rust_title_case(&tld.name).to_string() is redundant. The check should compare name_str against the base name (before uniquification) to determine if a number was appended due to a name collision.

The correct pattern is:

let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
// ... later in the code
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {

Copilot uses AI. Check for mistakes.
annotations.push(self.format_identifier_annotation(&tld.name, &tld.comments, &tld.ty));
}
Ok(sequence_or_set_of_template(
Expand Down Expand Up @@ -924,7 +932,7 @@ impl Rasn {
}
}

let name = self.to_rust_title_case(&tld.name);
let name = self.to_rust_title_case_unique(&tld.name);
let class_unique_id_type = class
.fields
.iter()
Expand Down
6 changes: 6 additions & 0 deletions rasn-compiler/src/generator/rasn/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use std::{
cell::RefCell,
collections::HashSet,
env,
error::Error,
io::{self, Write},
Expand Down Expand Up @@ -30,6 +32,7 @@ pub struct Rasn {
config: Config,
tagging_environment: TaggingEnvironment,
extensibility_environment: ExtensibilityEnvironment,
used_names: RefCell<HashSet<String>>,
}

#[cfg_attr(target_family = "wasm", wasm_bindgen(getter_with_clone))]
Expand Down Expand Up @@ -124,6 +127,7 @@ impl Backend for Rasn {
config,
extensibility_environment,
tagging_environment,
used_names: RefCell::new(HashSet::new()),
}
}

Expand All @@ -142,6 +146,8 @@ impl Backend for Rasn {
&mut self,
tlds: Vec<ToplevelDefinition>,
) -> Result<GeneratedModule, GeneratorError> {
self.used_names.borrow_mut().clear();

if let Some(module_ref) = tlds.first().and_then(|tld| tld.get_module_header()) {
let module = module_ref.borrow();
self.tagging_environment = module.tagging_environment;
Expand Down
47 changes: 41 additions & 6 deletions rasn-compiler/src/generator/rasn/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,11 +514,14 @@ impl Rasn {
// ITU-T X.691 clause 30.1, 30.6: Known-multiplier character strings types
match c_string.ty {
// 30.1: Known-multiplier character string types
CharacterStringType::NumericString | CharacterStringType::PrintableString |
CharacterStringType::VisibleString | CharacterStringType::IA5String |
CharacterStringType::BMPString | CharacterStringType::UniversalString => {
CharacterStringType::NumericString
| CharacterStringType::PrintableString
| CharacterStringType::VisibleString
| CharacterStringType::IA5String
| CharacterStringType::BMPString
| CharacterStringType::UniversalString => {
self.format_range_annotations(false, &all_constraints)?
},
}
// 30.6: Non-known-multiplier character string types
_ => TokenStream::new(),
}
Expand Down Expand Up @@ -1271,6 +1274,12 @@ impl Rasn {
)
}

pub(crate) fn to_rust_const_case_unique(&self, input: &str) -> Ident {
let base_name = self.to_rust_const_case(input).to_string();
let unique_name = self.get_unique_name(&base_name);
Ident::new(&unique_name, Span::call_site())
}

pub(crate) fn to_rust_enum_identifier(&self, input: &str) -> Ident {
let mut formatted = format_ident!("{}", input.replace('-', "_"));
if Self::RUST_KEYWORDS.contains(&input) {
Expand Down Expand Up @@ -1300,6 +1309,30 @@ impl Rasn {
TokenStream::from_str(&name).unwrap()
}

pub(crate) fn to_rust_title_case_unique(&self, input: &str) -> TokenStream {
let base_name = self.to_rust_title_case(input).to_string();
let unique_name = self.get_unique_name(&base_name);
TokenStream::from_str(&unique_name).unwrap()
}

fn get_unique_name(&self, base_name: &str) -> String {
let mut used_names = self.used_names.borrow_mut();
if !used_names.contains(base_name) {
used_names.insert(base_name.to_string());
return base_name.to_string();
}

let mut counter = 2;
loop {
let candidate = format!("{base_name}{counter}");
if !used_names.contains(&candidate) {
used_names.insert(candidate.clone());
return candidate;
}
counter += 1;
}
}
Comment on lines +1318 to +1334
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new name conflict resolution mechanism (using get_unique_name and tracking used names in a HashSet) lacks test coverage. Given that the repository has comprehensive test coverage for other generator features (see rasn-compiler-tests/tests/ and tests in utils.rs), this new functionality should have tests to verify:

  1. Basic conflict resolution (e.g., two types with conflicting names get numbered correctly)
  2. Multiple conflicts (e.g., Name, Name2, Name3 generation)
  3. The identifier annotation is correctly added when names are modified
  4. Names are reset between module generations (via used_names.borrow_mut().clear())

Consider adding a test similar to the example in the PR description, which tests types like E-UTRAN-Trace-ID and EUTRANTraceID that resolve to the same Rust identifier.

Copilot uses AI. Check for mistakes.

/// Generate a `TokenStream` containing a type identifier, optionally qualified by module.
///
/// Module name is converted to snake case, and type name converted to title case.
Expand All @@ -1319,10 +1352,12 @@ impl Rasn {
&self,
tld: &ToplevelTypeDefinition,
) -> Result<(TokenStream, Vec<TokenStream>), GeneratorError> {
let name = self.to_rust_title_case(&tld.name);
let base_name = self.to_rust_title_case(&tld.name).to_string();
let name = self.to_rust_title_case_unique(&tld.name);
let mut annotations = vec![quote!(delegate), self.format_tag(tld.tag.as_ref(), false)];

if name.to_string() != tld.name {
let name_str = name.to_string();
if name_str != tld.name || name_str != base_name {
annotations.push(self.format_identifier_annotation(&tld.name, &tld.comments, &tld.ty));
}

Expand Down