(Closes #2568) Adds a routine_symbol property to Routine and tests.

[JulienRemy]

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.85%. Comparing base (39dd617) to head (7b37ea9).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
- Coverage   99.86%   99.85%   -0.01%     
==========================================
  Files         357      357              
  Lines       48332    48327       -5     
==========================================
- Hits        48266    48257       -9     
- Misses         66       70       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[JulienRemy]

Hi @arporter and @sergisiso. I'm requesting review from both of you on this syntactic sugar PR.

Is the non required -0.01% codecov/project drop an issue? It's been reporting this on my 3 PRs.

[sergisiso]

Is the non required -0.01% codecov/project drop an issue? It's been reporting this on my 3 PRs.

No, in general we require a 100% of the patch (new code). Codecov has been acting strangely lately, so unless it lists something under the Codecov "indirect changes" (which this PR does not), don't worry about it.

[LonelyCat124]

Hi @JulienRemy - Can I request a quick test to check I'm not seeing some different behaviour with this - I think this is a quirk of how we initialise things in fparser/PSyIR/PSyclone.

The expected result from your call should be (one of ) the RoutineSymbol corresponding to the Routine in question, but I think if you try:

def test_my_test(fortran_reader):
   code = '''
    module test
    contains
   real function x(a)
     real :: a
     x =a 
    end function
end module'''

container = fortran_reader.psyir_from_source(code)
assert isinstance(container.children[0].children[0].symbol_table.lookup_with_tag("own_routine_symbol"), RoutineSymbol)

this will fail, as the "own_routine_symbol" is changed and has become a DataSymbol of the type of the function, and the only place the RoutineSymbol will still exist is in the parent Container (and at the moment, only if the parent Container is not a FileContainer, though I have a fix for that in progress).

Edit: Mostly I think here there is a decision of if "own_routine_symbol" returns the DataSymbol of a function - and thus the docs need updating, or whether you always want a RoutineSymbol, in which case we need to think a bit more.

[JulienRemy]

Hi @LonelyCat124,

Thanks for spotting this, there is indeed something confusing here, which I was unaware of.

In my mind (as things stand right now) the symbol tagged "own_routine_symbol" should probably be a RoutineSymbol indeed? A few arguments about this: 1/ creating a Call node to a function requires a RoutineSymbol or a Reference to one. But for (some) functions this symbol can only be obtained from the parent [File]Container symbol table and not from the Routine node itself, which is weird in my opinion. 2/ considering the following example, I personally find the current behavior a bit confusing, due to function y(a) result(b) having both a RoutineSymbol and a DataSymbol but not function x(a). I understand having both in the second case would cause a name conflict in the symbol table.

from psyclone.psyir.nodes import Routine
fortran_reader = FortranReader()

code = '''
module test
contains
    real function x(a)
        real :: a
        x = a 
    end function

    function y(a) result(b)
        real :: a, b
        b = a
    end function
end module'''

module_container = fortran_reader.psyir_from_source(code).children[0]
print(module_container.symbol_table.view())

for routine in module_container.walk(Routine):
    print(f"Routine {routine.name} has:\n"
          f"\t-routine_symbol {routine.routine_symbol}\n"
          f"\t-return_symbol {routine.return_symbol}\n")

Output:

---------------------------------
x: RoutineSymbol<Scalar<REAL, UNDEFINED>, pure=False, elemental=False>
y: RoutineSymbol<UnresolvedType, pure=False, elemental=False>

Routine x has:
        -routine_symbol x: DataSymbol<Scalar<REAL, UNDEFINED>, Automatic>
        -return_symbol x: DataSymbol<Scalar<REAL, UNDEFINED>, Automatic>

Routine y has:
        -routine_symbol y: RoutineSymbol<Scalar<REAL, UNDEFINED>, pure=unknown, elemental=unknown>
        -return_symbol b: DataSymbol<Scalar<REAL, UNDEFINED>, Automatic>

3/ by the way, why a DataSymbol when it bears the routine name, considering that RoutineSymbol can be typed and gives more information about what the symbol is? I get it for the result(b) but not the real function x.

As to #2566 and #2582, from what I understand, you would in time be working toward having it (only?) in the parent [File]Container symbol table, along the lines of #1200, as well as maybe getting rid of the current "own_routine_symbol" tag and then probably having a Routine._symbol attribute as per @sergisiso's suggestion https://github.com/stfc/PSyclone/pull/2566#discussion_r1595424992 ? For what it's worth, as a user I'd argue in favor of keeping said symbol (ideally a RoutineSymbol as per my Call remark above, not a DataSymbol) as an attribute of the Routine node (but ultimately not in its symbol table) as well as in the parent [File]Container symbol table, given that the Routine node could for example be detached or created on its own and that having to move the symbol between tables and to look in one or the other depending on this could get error prone. If the symbol is to be kept in both symbol tables, maybe it should be the same symbol instance in both, with different tags, which is not the case for now? At least SymbolTable().add does not prohibit this.

In any case, this syntactic sugar PR of mine might become useless or require some edits fairly fast if changes are made to the current approach, so I'm not sure there's much point in reviewing and merging it?

[LonelyCat124]

In my mind (as things stand right now) the symbol tagged "own_routine_symbol" should probably be a RoutineSymbol indeed?

I agree here, and think the routine_symbol property is useful and should return this. RIght now FileContainer's don't know the Routine's they contain - I have another PR to fix this behaviour once 2566 is completed.

My PRs don't plan to change the Routine's own RoutineSymbol - but 2566 simply ensures certain properties are correctly given the the symbol in the Routine that were previously getting lost, and 2582 should just ensure renaming maintains correct symbol names as opposed to changing any functionality.

I assume the functions symbols become a DataSymbol because when we do my_func = vlaue we create a Reference to it we usually expect a DataSymbol, but I don't see a requirement as to why it couldn't be a RoutineSymbol, but there may be sections of the code that break. This decision is done in fparser2.py in the frontend which I'm a bit unclear on some parts of.

@sergisiso @arporter Do either of you know why function's own symbols is a DataSymbol rather than a typed RoutineSymbol?

[arporter]

Am I right in thinking that #2596 will make this PR redundant?

[sergisiso]

Yes, that PR has a routine.symbol similar to the helper methos proposed here. Closing this PR