(closes #3460) Fix issues with recursive datatype copies

[sergisiso]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a74dfdf) to head (cd36fa9).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3521   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          397       397           
  Lines        55577     55579    +2     
=========================================
+ Hits         55577     55579    +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sergisiso]

@LonelyCat124 In addition to the recursive loops I found that copying partial_datatypes was chaning the original tree in some cases, so I added additional copy calls until this was not the case.

I am not sure I fully understand the whole recursive copy path which makes me a bit nervous, but I made the new test quite complete in testing both the consistency of the new copy and that the original remains unchanged (so at least I know that 2 problems currently on master are fixed by this)

[sergisiso]

Ah, it failed the integrations, I will look at this next week

[sergisiso]

After looking at the failed test I think the input fortran is wrong but it was unexpectedly fixed by copy not working :)

The input for test test_apply_symbol_dependencies is:

        code = (
            "module test_mod\n"
            "contains\n"
            "subroutine main()\n"
            "  real, dimension(10, 10) :: var = 0.0\n"
            "  call sub(var, 10)\n"
            "end subroutine main\n"
            "subroutine sub(x, ilen)\n"
            "  integer, intent(in) :: ilen\n"
            "  real, dimension(ilen, ilen), intent(inout) :: x\n"
            "  real, dimension(ilen, ilen) :: work\n"
            "  type nasty\n"
            "    integer, dimension(ilen+1) :: flag\n"
            "  end type nasty\n"

and once the sub is inlined it becomes:

  subroutine sub(x, ilen)
    integer, intent(in) :: ilen
    real, dimension(ilen,ilen), intent(inout) :: x
    type :: nasty
      integer, dimension(10 + 1) :: flag
    end type nasty

But inlining shouldn't change the original. After fixing the copy behaviour I get:

  subroutine sub(x, ilen)
    integer, intent(in) :: ilen
    real, dimension(ilen,ilen), intent(inout) :: x
    type :: nasty
      integer, dimension(ilen + 1) :: flag
    end type nasty

And it fails to compile with Expression at (1) in this context must be constant

@arporter Can I remove this "nasty" type, or should I replace it with something else?

[arporter]

And it fails to compile with Expression at (1) in this context must be constant

@arporter Can I remove this "nasty" type, or should I replace it with something else?

Ah! I hadn't realised you weren't allowed a dummy argument here. The only other thing I can come up with is a character string with len given by a dummy argument - pls could you try adding that instead of the "nasty" type?

[sergisiso]

The len symbols are not processed by inlining, and it results in a dangling symbol, I will attempt to fix it here.

[sergisiso]

That's unraveling some more issues:

• The call matcher has problems with character lengths, I won't fix it here, but I opened Call/callee subroutines with different len expressions cause CallMatchingArgumentsNotFound #3534
• A modification of a length copy also modifies the original.

[sergisiso]

@arporter @LonelyCat124 This is finally ready for review, I haven't fixed the inline issues as these already exist before this PR and the fix seems complicated and out of scope here, but I opened issues explaining them.

[LonelyCat124]

I'll take a look after lunch

[LonelyCat124]

@sergisiso One small query, but otherwise this looks fine to me. I'll set ITs running again as well to be safe.

[sergisiso]

@LonelyCat124 This is ready for another look

[LonelyCat124]

@sergisiso Small comment/query about the xfailing test. ITs were otherwise all green so once resolved we can merge.

[sergisiso]

@LonelyCat124 Ready for another look (CI-permitting)

[LonelyCat124]

All green now, fixes the original issue - will proceed to merge.