Chapel internal error RES-PRE-OLD-2591 in function pointer code

[npadmana]

Summary of Problem

Compiling the code below in Chapel 1.24.1 results in an internal error.

Steps to Reproduce

Source Code:

use M1 only Integrator;

var cc = new owned Integrator(R);
cc();

record R {}

module M1 {

  use CPtr;

  class Integrator {
    type T;

    proc this() {
      proc _helper() : real {
        return 0;
      }
      var gf = c_ptrTo(_helper);
    }
  }
}

Compile command: chpl foo.chpl

fails with

foo.chpl:3: warning: This file-scope code is outside of any explicit module declarations (e.g., module M1), so an implicit module named 'foo' is being introduced to contain the file's contents.
foo.chpl:20: internal error: RES-PRE-OLD-2591 chpl version 1.24.1
Note: This source location is a guess.

Internal errors indicate a bug in the Chapel compiler ("It's us, not you"),
and we're sorry for the hassle.  We would appreciate your reporting this bug -- 
please see https://chapel-lang.org/bugs.html for instructions.  In the meantime,
the filename + line number above may be useful in working around the issue.

[bradcray]

@npadmana: Thanks for filing this. I'm not quite certain what's going wrong, though given that it butts up against first-class functions which don't enjoy first-class support yet, I'm not 100% surprised. Have you found a workaround for this for the code that ran you into it with? Is it blocking you?

[for reference, the nature of the error is essentially that the compiler is failing to find _helper. I'm not yet sure whether it's caused by its nested function-in-a-method nature, whether the generic nature of the class is having an impact, or what].

[npadmana]

@bradcray -- No, not a blocker at all.

And your description of the error reminded me -- this issue goes away if I remove the only clause in the use statement (i.e. use M1;). So somehow, maybe that the nested function isn't being propagated.

[bradcray]

Oh, that's interesting, thanks for sharing that!

[leekillough]

@npadmana: I cannot reproduce this with the latest master.

If you can reproduce it on master, please provide the output of ./util/printchplenv so that I can attempt to reproduce it.

It might be a corner-case which is hard to trigger, and which goes away if there are any changes in the compiler or source.

[bradcray]

Huh... I'm seeing the same thing as Lee, though I think I reproduced it when I commented a few weeks ago.

@leekillough : Would you git bisect to see whether you can find the PR where the change occurred so that we can feel more confident that something got fixed rather than something being unstable? (And check in the test to prevent backsliding).

[leekillough]

This commit by @lydia-duncan makes the assertion go away.

foo.chpl:20: internal error: assertion error [resolution/preFold.cpp:2591]

(@npadmana: You can set CHPL_DEVELOPER=true to get these more meaningful error messages.)

@lydia-duncan: Did you test for this assertion in your tests, or should we add this test case to our suite?

[lydia-duncan]

Are you sure? This commit adds new calls, it doesn't prevent previous calls from occurring. I would expect an assertion error like that to occur due to a call that was already present.

When I do bisects, I try to go by merge commits rather than individual commits, since merged branches can be working from different base branches - I've found in the past that bisecting on all commits rather than merge commits led to wrong answers as a result

[leekillough]

Well, I verified that the assertion occurs here, which is the last merge commit before your change.

And I verified that it does not occur with your change, even if it hasn't been merged yet (or even if it wasn't merged with master at the time).

I'll keep searching to find if there is a more well-defined crossover. I can limit it to merge commits, to prevent false crossovers.

[lydia-duncan]

Huh, weird. Well, if it does seem to be my change, I don't think any of the tests I added covered it, so it would be good to add the motivating case here as a test.

[leekillough]

Skipping all non-merge commits:

There are only 'skip'ped commits left to test.
The first new commit could be any of:

https://github.com/chapel-lang/chapel/commit/ff788649699880df2e76aa8cc4b8c272f75386f5 https://github.com/chapel-lang/chapel/commit/6428e5795a0c80984e1b37ee0f06b2099cc56b03 https://github.com/chapel-lang/chapel/commit/a1913547c1f88e46f260368ed32e5be18acb98d5 https://github.com/chapel-lang/chapel/commit/91dc45c6798e3422d9dda6f88ced0cf30363a131 https://github.com/chapel-lang/chapel/commit/a961b23386dc8c0c319500fec93d5bcbe9d77880 https://github.com/chapel-lang/chapel/commit/14de403ff2eade6f0823e6c6aa9f6edb6d5d3ca1 https://github.com/chapel-lang/chapel/commit/e9ffe27df00c1f83b392e57d8849078b47b19787 https://github.com/chapel-lang/chapel/commit/c104a39288f22fabc077d05ac55da52ebbfbbb20

We cannot bisect more!

That confirms that your commit "fixes" it, but there may be something else going on, such as a minor modification of the AST causing the bug to "go away".

This is the assertion which fails:

/*                                                                                                                                      
  Captures a function as a first-class value by creating an object that will                                                            
  represent the function.  The class is created at the same scope as the                                                                
  function being referenced.  Each class is unique and shared among all                                                                 
  uses of that function as a value.  Once built, the class will override                                                                
  the .this method of the parent and wrap the call to the function being                                                                
  captured as a value.  Then, an instance of the class is instantiated and                                                              
  returned.                                                                                                                             
*/
static Expr* createFunctionAsValue(CallExpr *call) {
  static int unique_fcf_id = 0;

  UnresolvedSymExpr* use    = toUnresolvedSymExpr(call->get(1));
  const char*        flname = use->unresolved;

  Vec<FnSymbol*>     visibleFns;

  getVisibleFunctions(flname, call, visibleFns);

  if (visibleFns.n > 1) {
    USR_FATAL(call, "%s: can not capture overloaded functions as values",
                    visibleFns.v[0]->name);
  }

  INT_ASSERT(visibleFns.n == 1);

I'll add this to the tests.

[leekillough]

It could be that the first-class function, which is represented as a class type, did not have an operator overload available, which @lydia-duncan's changes fixed.

[bradcray]

That just adds one more reason that Lydia's #17684 is my favorite PR of the week.

[leekillough]

Could someone please review https://github.com/chapel-lang/chapel/pull/17794 so that this issue can be closed?

[bradcray]

I can't tell whether there's a sense of exasperation in the comment just above, but just in case, I wanted to note that it's typical for PRs to go unreviewed until the developer asks someone to review them. It rarely happens organically (in part because opening a PR doesn't generate a notification for most developers, and in part because opening a PR isn't always a sign that it's ready to be reviewed). The main time that I look at PRs proactively is when I'm trying to figure out why there are so many open at a given time.

[bradcray]

@npadmana : Thanks for reporting this, and please let us know if you encounter similar issues again.