Skip to content

Conversation

@lukem
Copy link

@lukem lukem commented Oct 29, 2025

asn1c: fix IMPORTS of same name from multiple modules

ASN.1 supports importing the same name from different modules as well as allowing that name to be in the current module - refer to ITU-T X.680 and https://stackoverflow.com/a/67910959.

Rework the IMPORTS tracking in imp from dict name -> module dict name -> list(module, ...), and resolve types that way.

Fixes #56

lukem added 4 commits October 29, 2025 19:13
Enhance JSONDepGraphGenerator to sort the nodes and links.
Test JSONDepGraphGenerator.
Extend test of HardcoreSyntax to import from two new
embedded modules ImportModule1 and ImportModule2,
and check the IMPORTS support that is working.
Add test that imports the same name from multiple modules,
with an expected failure. This is permitted by ITU-T X.680,
but compile_text() doesn't support this.
ASN.1 supports importing the same name from different modules
as well as allowing that name to be in the current module -
refer to ITU-T X.680 and https://stackoverflow.com/a/67910959.

Rework the IMPORTS tracking in _imp_ from dict name -> module
dict name -> list(module, ...), and resolve types that way.

Fixes pycrate-org#56
@mitshell
Copy link
Member

Thanks for this PR: this looks like an ambitious one.

I tested the changes against the entire set of ASN.1 specs, but the JSON code generator now breaks on module IETF_PKI_RFC5958. You can test the compilation of all modules with the cmd below:

python -m pycrate_asn1c.asnproc

I will need to investigate your changes further to understand the root cause of the issue. If you want to solve this on your side, here are the steps I'd follow when touching the ASN.1 compiler:

  • re-compiling all the ASN.1 modules
  • running the unit tests

You can otherwise set the globals TEST_ASN1C_ALL_COMP and TEST_ASN1C_ALL_LOAD to True in the main unit test file test_pycrate.py. This re-compiles all the ASN.1 modules and test their loading, each time the unit tests are run.

lukem added 4 commits October 31, 2025 17:59
The JSON nodes and links are now sorted by JSONDepGraphGenerator
(see commit 1939a8e), so regenerate the .json files.
Test import of a type (ImportModule1.Imp1Str) that itself is imported
from another module (ImportModule2.Imp2Str)
@lukem
Copy link
Author

lukem commented Oct 31, 2025

Apologies for not running python -m pycrate_asn1c.asnproc - I missed that step in the README for running all the tests.

I've updated the branch and made the following improvements:

  1. Regenerated pycrate_asn1dir/**.json per the change I made in commit 1939a8e to ensure deterministic node and link ordering.
  2. Extended my IMPORTS .asn tests to attempt to reproduce the problem. (Didn't hit the bug)
  3. Fixed JSONDepGraphGenerator to prevent the problem that the full recompile hit. (This was actually a TODO I'd added).

I can now successfully run all the tests on the branch:

python -m unittest test.test_pycrate
python ./test.py
python -m pycrate_asn1c.asnproc

@lukem
Copy link
Author

lukem commented Nov 4, 2025

@mitshell : if it's easier, I can break up this PR into two new PRs to make it easier to review the parts separately:

  1. Change JSONDepGraphGenerator to order the nodes in the output, and the regeneration of the .JSON files. The reason for this commit is that prior to python 3.7 dict ordering isn't deterministic. From python 3.7, insert order is maintained, but it can be hard to "eyeball verify" output files when they're not sorted. Sorting the results from processing a dict ensures there's deterministic output across tests.

  2. The import fix, which changes how _imp_ dict value from a single module name to a collection of modules. This was the core fix for Importing same name from multiple modules fails #56, and resolves problems compiling and parsing some complex ASN.1 specs that have complex import dependencies.

Thoughts?

@mitshell
Copy link
Member

mitshell commented Nov 7, 2025

No worries, I'll take some time to review this PR as is: I don't have much time right now and I'll do my best. Thanks anyway for this PR, which loolks pretty nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing same name from multiple modules fails

2 participants