librasn / compiler

An ASN1 compiler producing Rust bindings for the rasn framework
Other
10 stars 7 forks source link

Issue when trying to compile asn1 files for s1ap protocol #14

Open yochayKen opened 4 months ago

yochayKen commented 4 months ago

Hello everyone!

I'm having an issue when I'm trying to compile asn1 files of s1ap protocol through the CLI tool. I'm using the files from the Wireshark repository.

The error message that I'm getting is: Encountered error while parsing MatchingError(Tag) - Error matching ASN syntax while parsing:SONInformationReply-ExtIEs S1AP-PROTOCOL-EXTENSION

Which appears in the S1AP-IEs.asn file.

Another issue that I get: Encountered error while parsing MatchingError(Tag) - Error matching ASN syntax while parsing:S1AP-ELEMENTARY-PROCEDURES-CLASS-1 S1AP-ELEMENTARY-PROCEDURE

from S1AP-PDU-Descriptions.asn file.

Thanks for anyone who can help!

6d7a commented 4 months ago

Thank you for your issue. I could not reproduce your exact issue with the asn sources from the wireshark repo. However, while our online compiler does produce bindings, I noticed that they still contain some errors that we will need to look into. Unfortunately, the mobile telephony standards are notoriously complicated. I will add the S1AP to our test set, so that we can generate working bindings ASAP.

v0-e commented 2 months ago

@yochayKen seems to be trying to compile a more recent version of S1AP, where S1AP-ELEMENTARY-PROCEDURES-CLASS-1 is extended. Link

v0-e commented 2 months ago

Apparently there are also two different types with basically the same name, ECGIList and ECGI-List which get compiled into the same Rust type ECGIList, conflicting with each other. Maybe it is best to preserve the underscores in to_rust_title_case() @6d7a?

6d7a commented 1 month ago

In my opinion, the compiler should catch naming conflicts in the linking and validation phase. If we only preserve underscores, we still get naming conflicts for edge cases like Type-name and Type_name. Also we would sacrifice rust's title case convention for a few edge cases. We do preserve the original names throughout the compilation, so if the compiler can detect and handle a conflict once it has collected all top-level type definitions.

v0-e commented 1 month ago

Underscores are illegal in ASN.1 identifiers I think. So Type_name as a Rust type could safely represent the Type-name ASN.1 type. There are no conflicts in S1AP, however the compiler creates one by changing the hyphen to an underscore and then removing it from ECGI-List.

6d7a commented 1 month ago

You're right, I remembered the identifier rules wrong. I'm still not sure whether we should throw rust's camel case convention overboard. On the one hand, I like the hyphen-to-underscore conversion because, glancing over a spec, it is visually very close to the original names (and JER also uses it). On the other hand, we are neither following rust's naming convention, nor are we conserving the original names. I've seen that other compilers like asn1c have configuration options for leaving the decision regarding identifier transformations up to the user. Perhaps that's the way to go. What do you think?

v0-e commented 1 month ago

Since ABCD, AB-CD, ABcd, AB-cd, Ab-cd, Ab-Cd, ..., are all valid and distinct ASN.1 types, then we cannot force the cohersion to title-case/PascalCase everytime. Yeah, I think it would be cool to have a few options regarding this:

  1. Just convert hyphens to underscores;
  2. Convert hyphens to underscores, iif, the compiler detects any conflicts, otherwise use PascalCase. (default option?)
  3. Force PascalCase, but warn the user if and which conflicts exist so these may be resolved manually in the generated bindings.