[Bug] Undesired Behavior on Walker With Entry and With Exit

[ChrisIsKing]

Describe the bug

In Jac 1 walkers had the ability to execute functions/abilities based on the node they are currently traversing. e.g

walker test_walker {
    test_node {
        // Execute this logic when the node is of type test node
    }
}

the same can be achieved in jaclang with a more descriptive and readable syntax. eg.

walker test_walker {
    can execute_logic with test_node entry {
        // Execute this logic when the node is of type test node
    }
}

This execute logic when entering a node of type test_node.

However, missing from jaclang is the ability to execute upon the the entry and exit of the entire walk. e.g in Jac 1 you could do

walker test_walker {
    with entry {
        // Execute this logic upon walker traversal start
    }

    with exit {
        // Execute this logic upon walker traversal completion
    }
}

Syntactically, it appear you could the same in jaclang by doing the following

walker test_walker {
    can execute_entry with entry {
        // Execute this logic upon walker traversal start
    }

    can execute_exit with exit {
        // Execute this logic upon walker traversal completion
    }
}

However, this results in the logic being executed upon the entry and exit of any node type, which is not the desired behavior. This should behave like jac1 since the same can already be achieved by doing can execute_entry with any entry.

To Reproduce

Here is a sample program to reproduce this:

node test_node {
    has value: int;
}

walker test_walker {
    has visited_nodes: list = [];
    has entry_count: int = 0;
    has exit_count: int = 0;

    can traverse with `root entry {
        visit [-->](`?test_node);
    }

    can log_entry with entry {
        self.entry_count += 1;
    }

    can log_exit with exit {
        self.exit_count += 1;
    }

    can log_visit with test_node exit {
        self.visited_nodes.append(here);
    }
}

with entry {
    for i in range(10) {
        root ++> (next:=test_node(value=i));
    }
    wlk_obj = root spawn test_walker();
    print(wlk_obj);
}

[marsninja]

Hey, I may not have time to knock out a pr for this but its very easy:

In this piece of code below just move the walker entry and exit blocks outside the loop:

These are the blocks that should be before and after loop:

[marsninja]

@ChrisIsKing Does kugesan's pr on this resolve your issue?

[ChrisIsKing]

@marsninja @kugesan1105 I think I found a bug, not sure if this is related to the PR but I modified the test case to check for the condition where the developer wants to have an ability that is triggered on any node entry

can log_visit with `Any exit {
        print("Visiting node : ", here);
        self.visited_nodes.append(here);
    }

It resulted in this error:

Entering at the beginning of walker:  Root()
ERROR - Error: typing.Any cannot be used with isinstance()
        raise TypeError("typing.Any cannot be used with isinstance()")
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    at __instancecheck__() /usr/local/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/typing.py:530
    at spawn_call() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/runtimelib/architype.py:558
    at spawn_call() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/default.py:371
    at _multicall() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py:103
    at _multicall() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py:139
    at _hookexec() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_manager.py:120
    at __call__() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_hooks.py:513
    at spawn_call() /Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/feature.py:168
    at <module> /Users/chrisisking/dev/jaseci/jac/jaclang/tests/fixtures/entry_exit.jac:34
Traceback (most recent call last):
  File "/Users/chrisisking/dev/jaseci/venv/bin/jac", line 8, in <module>
    sys.exit(start_cli())
             ^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/cli/cli.py", line 514, in start_cli
    ret = command.call(**args_dict)
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/cli/cmdreg.py", line 25, in call
    return self.func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/cli/cli.py", line 91, in run
    jac_import(
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/feature.py", line 119, in jac_import
    return pm.hook.jac_import(
           ^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py", line 139, in _multicall
    raise exception.with_traceback(exception.__traceback__)
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/default.py", line 268, in jac_import
    import_result = JacImporter(JacMachine.get()).run_import(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/runtimelib/importer.py", line 354, in run_import
    raise e
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/runtimelib/importer.py", line 351, in run_import
    exec(codeobj, module.__dict__)
  File "/Users/chrisisking/dev/jaseci/jac/jaclang/tests/fixtures/entry_exit.jac", line 34, in <module>
    wlk_obj = root spawn test_walker();
               ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/feature.py", line 168, in spawn_call
    return pm.hook.spawn_call(op1=op1, op2=op2)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_hooks.py", line 513, in __call__
    return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_manager.py", line 120, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py", line 139, in _multicall
    raise exception.with_traceback(exception.__traceback__)
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/vendor/pluggy/_callers.py", line 103, in _multicall
    res = hook_impl.function(*args)
          ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/plugin/default.py", line 371, in spawn_call
    return op2.__jac__.spawn_call(op1.__jac__)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/chrisisking/dev/jaseci/venv/lib/python3.12/site-packages/jaclang/runtimelib/architype.py", line 558, in spawn_call
    if not i.trigger or isinstance(current_node, i.trigger):
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/typing.py", line 530, in __instancecheck__
    raise TypeError("typing.Any cannot be used with isinstance()")
TypeError: typing.Any cannot be used with isinstance()

Since this addition replaces the ability to trigger on an entry of any type, we should still cater for this.

[ChrisIsKing]

Seems like you can achieve this using the object type.

can log_visit with object exit {
        print("Visiting node : ", here);
        self.visited_nodes.append(here);
}

All nodes are python object so it makes sense that this works but there is a readability argument here as core jac users may not be familiar with this keyword.

[ChrisIsKing]

@marsninja If you're fine with the above comments, we can close this issue.

[marsninja]

@ChrisIsKing and @kugesan1105, Actually this is actually and important issue for us to address in the language. Essentially a number of legit types from the typing library cannot be used with isinstance and thus will break similarly. We definitely need a PR that adds a test of this issue where we have better error reporting.

@kugesan1105 The best way to build this static error condition (that would trigger in the static compiler passes) is to look into the below code location in pythons standard library and see all teh casese that trigger this type of error and statically try to catch them during compile.

  File "/usr/local/Cellar/python@3.12/3.12.5/Frameworks/Python.framework/Versions/3.12/lib/python3.12/typing.py", line 530, in __instancecheck__
    raise TypeError("typing.Any cannot be used with isinstance()")

Spool up a pr with the test and your (or someone else on the team) should look into whether that fix makes sense. Certainly keep me posted.

Also, @ChrisIsKing Thank goodness you didn't close this and move on with the work around. These are exactly the types of things we need to do to button up Jac. object is the way to go here but better error reporting is super necessary and shouldnt fall throught eh cracks.

[ChrisIsKing]

@marsninja My understanding of the with entry and with exit blocks is that they're core data spatial abilities that trigger with graph traversal. These graph traversal triggers are only relevant to nodes and walkers. As such why not modify the language only to allow objects of type node or walker to trigger for a with some_object entry/exit. Leaving it up to any type supported by isinstance() allows devs to create weird abilities like for e.g

walker test_walker {
    can print_something with int entry {
        print("Something:", f'{here}');
    }
}

with entry {
    root spawn test_walker();
}

[marsninja]

You are 100% correct @ChrisIsKing , and now that you mention it there should indeed be a typecheck error in the with entry syntax. Right now all of the typechecks are warnings but in principle and when its beta tested enough we should flip a switch to make jac treat these (or at least some of these) as static errors. This would be in the realm of Jac as a typescript to javascript mode (which woudl be an option to make type checks warnigns).

@kugesan1105 do we have a test case specificly for hte kind of code chris shows in his example. This would simply validate that the type check pass produces an error with the with int entry. If not do assign that PR to me once created and i can take a quick look.

[ChrisIsKing]

@kugesan1105 Is this issue good to close now? I believe the inconsistency mentioned is a separate issue/feature request right?

[ChrisIsKing]

Closing as the original issue was fixed in PR #1333. Will track typing issue and recommendation in a new issue