Skip to content

Conversation

@AkiSakurai
Copy link
Contributor

This commit changes the evaluation of nested template arguments from lazy (pass-by-name) to eager (pass-by-value) to enforce correct lexical scoping.

Motivation:

Previously, in a nested template like T<K<x>>, the argument x was resolved lazily within the scope of T instead of the scope where it was declared. This pass-by-name behavior for nested types violated lexical scoping and caused unpredictable variable resolution.

Implementation:

The core of the fix was to separate a type's declaration from its application:

  1. A new ASTNodeTypeApplication node was introduced to represent a specific instantiation of a type. It captures the fully resolved template arguments at the declaration site, effectively creating a closure over the surrounding scope.

  2. The parser was updated to eagerly evaluate all arguments—including nested types—into this new ASTNodeTypeApplication node.

  3. ASTNodeTypeDecl was simplified to only handle the static declaration of a type, leaving all instantiation-specific logic to ASTNodeTypeApplication. As a key benefit of this cleaner separation, forward-declared types can now be used in templates, enabling mutually recursive template definitions.

  4. A new test suite (test_pattern_template_parameters_scope.hpp) was added to verify the corrected behavior with various nested and recursive template scenarios that previously failed.

fix #144

@WerWolv
Copy link
Owner

WerWolv commented Nov 30, 2025

Hey! Wow thank you so much for that work! That has been a very long standing issue.

If you haven't already, could you run your changes on the patterns in the WerWolv/ImHex-Patterns repo? The unit tests there should provide good coverage to make sure nothing else broke and will show if there's any changes that will need to be done due to the changed lexical scope now.

@AkiSakurai
Copy link
Contributor Author

If you haven't already, could you run your changes on the patterns in the WerWolv/ImHex-Patterns repo?

I've run it past all except this one. Need to remove the parent.

diff --git a/patterns/xilinx_bootgen.hexpat b/patterns/xilinx_bootgen.hexpat
index 5fb9b40..523630e 100644
--- a/patterns/xilinx_bootgen.hexpat
+++ b/patterns/xilinx_bootgen.hexpat
@@ -213,7 +213,7 @@ namespace zynqmp {
 
         u64 destination_execution_address;
         u64 destination_load_address;
-        NullableWordPtr<std::Array<u8,parent.encrypted_partition_data_word_length*4>, u32> actual_partition_word_offset;
+        NullableWordPtr<std::Array<u8,encrypted_partition_data_word_length*4>, u32> actual_partition_word_offset;
 
         PartitionAttributeBits attributes;
         u32 section_count;

@AkiSakurai
Copy link
Contributor Author

It seems that the CI for ImHex Patterns Tests does not pull the correct branch.

@WerWolv
Copy link
Owner

WerWolv commented Dec 1, 2025

It seems that the CI for ImHex Patterns Tests does not pull the correct branch.

That should be fixed now. The CI now passes along the current git hash and the git repo to the ImHex-Pattern repo unit tests CI and uses that repo instead of being hardcoded to master of this repo.
If you can rebase your PR, it should then properly run on the right repo

@AkiSakurai AkiSakurai force-pushed the backup2 branch 3 times, most recently from ad3842f to 70b4881 Compare December 5, 2025 13:42
This commit changes the evaluation of nested template arguments from lazy (pass-by-name) to eager (pass-by-value) to enforce correct lexical scoping.

Motivation:

Previously, in a nested template like `T<K<x>>`, the argument `x` was resolved lazily within the scope of `T` instead of the scope where it was declared. This pass-by-name behavior for nested types violated lexical scoping and caused unpredictable variable resolution.

Implementation:

The core of the fix was to separate a type's declaration from its application:

1. A new `ASTNodeTypeApplication` node was introduced to represent a specific instantiation of a type. It captures the fully resolved template arguments at the declaration site, effectively creating a closure over the surrounding scope.

2. The parser was updated to eagerly evaluate all arguments—including nested types—into this new `ASTNodeTypeApplication` node.

3. `ASTNodeTypeDecl` was simplified to only handle the static declaration of a type, leaving all instantiation-specific logic to `ASTNodeTypeApplication`. As a key benefit of this cleaner separation, forward-declared types can now be used in templates, enabling mutually recursive template definitions.

4. A new test suite (`test_pattern_template_parameters_scope.hpp`) was added to verify the corrected behavior with various nested and recursive template scenarios that previously failed.
Break the cycle manually when the parser and parser manager get reset.
@WerWolv
Copy link
Owner

WerWolv commented Dec 6, 2025

Seems to fully pass now besides the Xilinx Bootgen Pattern you mentioned earlier. I will update that one manually once your PR is merged

@WerWolv
Copy link
Owner

WerWolv commented Dec 6, 2025

I have also tested out what I could and it seems to be working really well overall. The implementation of it also looks decent. If you could try to make your code style match the one of the surrounding code, that would be great (e.g spaces between if and ( for example). Other than that, I'd be happy to merge this

@paxcut
Copy link
Collaborator

paxcut commented Dec 6, 2025

I wonder what impact this change is going to have on peoples local patterns . As far as I know templates have always needed parent to access them and there is even an example in the documentation that shows how to create an array of arrays that uses the parent in the inner most template argument. I guess there is not much we can do about it other than maybe a small explanation in the error message when patterns that have always works now suddenly start to fail.

@WerWolv
Copy link
Owner

WerWolv commented Dec 6, 2025

Yeah you're right, this is going to break patterns but I'm more than willing to do that because having to use parent there in the first place never made sense.
I will definitely update the docs and add a note about it to the next release notes. I think that should be alright. If we get support requests about it, it will be easy enough to explain

I'd expect not too many people to have used it before since it only worked for really basic cases. As you see from the unit tests now, there's only a single pattern in the ImHex-Patterns repo that was affected by this

@WerWolv WerWolv merged commit 6042a51 into WerWolv:master Dec 6, 2025
4 of 5 checks passed
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.

Incorrect evaluation occurs when a local variable name collides with the template argument of an instantiated template parameter.

3 participants