(Closes #1734 #2975 #1258 #2949) Add stricter checking for symbols by arporter · Pull Request #3358 · stfc/PSyclone

arporter · 2026-03-02T15:10:49Z

I've broken this out of the work on #1823 - that work revealed issues with our Symbol handling so I thought I might as well do the job properly.

We now no longer add a wildcard import of constants_mod in LFRic generated code. This revealed a few bugs/misssing symbols that I've now fixed.

…n outer scope

arporter · 2026-03-04T14:55:54Z

Despite what it says above, if I go to CodeCov then it says there are 0 indirect coverage changes.
Therefore, I declare this ready for review. Probably one for @sergisiso.

sergisiso

This is a great step to make symbols internally consistent, thanks @arporter. There are several related issues that I would like to understand where this change leave us.

sergisiso · 2026-03-09T15:28:28Z

src/psyclone/domain/lfric/kern_call_arg_list.py

+            if module in self._symtab.symbols:
+                mod_sym_tab = self._symtab


Seems odd that the find_symbol_table(self._symtab.node) below doesn't already search the given scope as the parameter is :param node: the PSyIR node from which to search. but the implementation starts with a node.scope, wich means the ancestor scope.

Could you check if including the given scope in the search would break other things. I think this would match the expected behaviour of this method name. And even if it breaks other things should those other calls then be ...find_symbol_table(parent) instead?

That is odd. I've just checked node.scope though and it does:

node = self.ancestor(ScopingNode, include_self=True)

so it will return its own table first. I think the problem is that I hacked this code in towards the end because I hit situations where self._symtab.node was None. I'll refactor it.

sergisiso · 2026-03-09T15:52:22Z

src/psyclone/domain/lfric/lfric_symbol_table.py

-    # A mapping of 'i_def' etc. to the corresponding DataSymbol
-    _precision_map = {}
-
-    def __init__(self, node=None, default_visibility=Symbol.Visibility.PUBLIC):


Always happy to see methods deleted from this class, as we have a good strategy to do DSL nodes (by uplifting-lowering), but subclassing other parts of PSyclone for API-specific logic is problematic (discussed here: #1954).

This PR is big enough, but do you think we could fully remove this class? This is now just helpers to create symbols with the LFRic precisions, but the generic find_or_create already accepts a datatype argument to specify these.

Yes we could. I started doing it and it was going well but it got quite big so I decided to do the Right Thing and break it into #3366.

sergisiso · 2026-03-09T15:55:16Z

src/psyclone/domain/lfric/lfric_symbol_table.py

-    _precision_map = {}
-
-    def __init__(self, node=None, default_visibility=Symbol.Visibility.PUBLIC):
-        super().__init__(node, default_visibility)


Also, asking about related issues, does this PR bring any update to #2659 #1258 or #2975?

Arguably, #1258 could already be closed but I'll add it to the list for this PR. Unfortunately this PR doesn't do anything to address the root of #2659 - that is a harder problem. (Although it will improve the consistency of a tree as soon as it is copied.) #2975 will be closed by this (thanks for spotting).

sergisiso · 2026-03-09T16:03:27Z

src/psyclone/psyir/nodes/extract_node.py

+            # Take care in case we've found an existing ContainerSymbol and
+            # it's in an outer scope.
+            actual_table = container.find_symbol_table(symbol_table.node)


I am a bit confused about why do we need here, is it to "declare the new imported symbols in the same scope as container they come from"? If thats the case this may be a better comment.

Yes, updated

sergisiso · 2026-03-09T16:07:40Z

src/psyclone/psyir/nodes/scoping_node.py

        # Reorganise the symbol table construction to occur before we add
        # the children.
-        self._symbol_table = other.symbol_table.deep_copy(self)
+        self._symbol_table = other.symbol_table.deep_copy(new_node=self)


Not the fault of this PR but I always found that argument name strange. Could we replace "new_node" to "attached_to"? Since the symtab method that does the same is call "attach"

Oh yes, that's a good idea. I also realised that we didn't check the type of the supplied node so I've added that plus test.

sergisiso · 2026-03-09T16:25:21Z

src/psyclone/psyir/symbols/symbol_table.py

+                    new_sym.interface.container_symbol = new_st.find_or_create(
+                        name, symbol_type=ContainerSymbol)


It is a bit unexpected that deep_copy creates new symbols or changes the name of symbols (find_or_create behaviour). But I understand this is necessary if the symbol has a dependency to an outer scope AND we change the associated node, which could make a lookup go wrong. Is this the reason?

I will be happier if we extend the second paragraph of the docstring to explain this behaviour as a consequence of providing a different associated scope node.

Agreed. Note that it won't change the name of a Symbol but it will add a new ContainerSymbol if none is found. I tried removing this and ~70 tests broke, primarily because of the way we create a temporary copy of a SymbolTable within LFRicKern.reference_accesses(). That's already the subject of #2874 so I've put in a few more TODOs. I've extended the paragraph as you suggested.

However, I've also realised that perhaps I should be stricter about imported Symbols. As things are now, I check that a suitable ContainerSymbol is in scope when a Symbol with ImportInterface is added to a table. We could tighten this further to require that the ContainerSymbol is in the same table as the imported symbol. What do you think?

sergisiso · 2026-03-09T17:09:16Z

src/psyclone/psyir/symbols/symbol_table.py

+                            other_sym.name, other_table=other_table))
+            self.update_import_interface(isym)
+
+    def update_symbol_dependencies(self):


"thinking out loud" comment, no need to agree, but I am not sure if "update_" is too generic to find out what these two methods do without reading the docstrings but I am not sure of good alternatives. What I could think of is:

localise_symbol_dependencies

bind_symbol_dependencies_to_scope

same for the next one where I also wonder if adding "_of" would help make clear is the argument that we update, .e.g. "localise_import_interface_of(symbol)"

Agreed. I've gone for localise_all_symbol_dependencies and localise_import_interface_of.

sergisiso · 2026-03-09T17:18:12Z

src/psyclone/psyir/symbols/symbol_table.py

+        refers to a Container in this or an ancestor scope.
+
+        If no ContainerSymbol is found with the name of the one in the
+        ImportInterface of the Symbol, then the ContainerSymboll in the


"ContainerSymbol"

sergisiso · 2026-03-09T17:23:21Z

src/psyclone/tests/psyir/symbols/symbol_table_test.py

+    with pytest.raises(symbols.SymbolError) as err:
+        table.update_symbol_dependencies()
+    assert ("which is not of the same type as the original dependency"
+            in str(err.value))


If I am not wrong, this test also needs a case with an outer symbol that has been shadowed in the inner scope, but the "update" should not change the interface of an import in the inner scope (something the implementation already accounts for if I understand the comments correctly)

Good point. Test added - let me know if it isn't quite what you had in mind.

sergisiso · 2026-03-09T17:25:06Z

src/psyclone/lfric.py

-                try:
-                    constants_container = symtab.lookup(const_mod)
-                except KeyError:
-                    # TODO Once #696 is done, we should *always* have a


Where are we with #696?

Another good spot - #696 is done so I've closed it. I've just grepped and found no remaining TODOs for it.

arporter · 2026-03-12T18:01:30Z

Ready for another look now @sergisiso. I've fired off the ITs again...

arporter and others added 30 commits January 21, 2026 13:21

For #1823. Add Kernel symbols when Kernel is created.

6cbe966

#1823 first sketch of mixin [skip ci]

ba8f599

#1823 WIP fixing module-inlining for multiple kernel calls [skip ci]

5b4b68b

#1823 WIP removing .module_inline setter and fixing tests [skip ci]

95b5710

#1823 fix module-inline tests

1bb59eb

#1823 WIP fixing tests [skip ci]

3811893

#1823 WIP fixing more tests [skip ci]

3082aa8

Merge branch 'master' into 1823_transform_inlined_kerns_only

2ae13c8

#1823 more test fixes [skip ci]

564e46d

#1823 fix lint [skip ci]

134cd05

#1823 WIP fixing tests [skip ci]

06341f7

#1823 more test fixing

0230c2f

Merge branch 'master' into 1823_transform_inlined_kerns_only

8fec073

#1823 WIP fixing examples [skip ci]

240f86d

#1823 add docstrings to new mixin

b269f7a

Merge branch 'master' into 1823_transform_inlined_kerns_only

5a88b29

#1823 begin adding new tests [skip ci]

9d8ac4a

Merge branch 'master' into 1823_transform_inlined_kerns_only

1c07cb1

#1823 get coverage of new mixin

2e0dbc1

#1823 remove unused/unreachable code and reinstate some tests

f73cd68

#1823 allow for ContainerSymbol from which kernel is imported to be i…

2eaf62c

…n outer scope

Merge branch 'master' into 1823_transform_inlined_kerns_only

7d8ada3

Merge branch 'master' into 1823_transform_inlined_kerns_only

c5e1211

#3294 update documentation

001e8e5

#3294 rm kernel-naming option from source

24072f4

#1823 updates for review

7c2e8e8

Merge branch 'master' into 1823_transform_inlined_kerns_only

41f84da

#1823 rm _modified property of CodedKern

b68c40d

#1823 update kernel-transformation documentation

192c0f7

#1823 fix tabs and update command-line help message

63bd9a9

arporter added 4 commits March 3, 2026 15:55

#1734 tidy comment

4f7630e

#1734 experiment with moving functionality into ScopingNode [skip ci]

23b47b2

#1734 mv functionality into SymbolTable and test

f73ea88

#1734 revert psyGen test changes to get cov

fb27241

arporter marked this pull request as ready for review March 4, 2026 14:54

arporter self-assigned this Mar 4, 2026

arporter added enhancement ready for review labels Mar 4, 2026

sergisiso added under review and removed ready for review labels Mar 9, 2026

sergisiso requested changes Mar 9, 2026

View reviewed changes

sergisiso added reviewed with actions and removed under review labels Mar 9, 2026

arporter added in progress and removed reviewed with actions labels Mar 10, 2026

Merge branch 'master' into 1734_stricter_symbols

4e12af4

arporter changed the title ~~(Closes #1734) Add stricter checking for symbols~~ (Closes #1734 #2975) Add stricter checking for symbols Mar 10, 2026

arporter changed the title ~~(Closes #1734 #2975) Add stricter checking for symbols~~ (Closes #1734 #2975 #1258) Add stricter checking for symbols Mar 10, 2026

arporter changed the title ~~(Closes #1734 #2975 #1258) Add stricter checking for symbols~~ (Closes #1734 #2975 #1258 #2949) Add stricter checking for symbols Mar 10, 2026

arporter and others added 5 commits March 10, 2026 22:16

#1734 improvements for review

7034130

Merge branch 'master' into 1734_stricter_symbols

5ca70e0

#1734 updates for review

7c6c703

#1734 add test with nested scopes

114204b

Merge branch 'master' into 1734_stricter_symbols

a93124b

arporter temporarily deployed to integration March 12, 2026 18:00 — with GitHub Actions Inactive

arporter added ready for review and removed in progress labels Mar 12, 2026

arporter mentioned this pull request Mar 13, 2026

(Closes #3366) rm LFRicSymbolTable #3374

Draft

		if module in self._symtab.symbols:
		mod_sym_tab = self._symtab

		new_sym.interface.container_symbol = new_st.find_or_create(
		name, symbol_type=ContainerSymbol)

Conversation

arporter commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arporter commented Mar 4, 2026

Uh oh!

sergisiso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arporter commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arporter commented Mar 2, 2026 •

edited

Loading