A function nested in an initializer can be called in phase1 and do non-phase1 things.

[cassella]

Summary of Problem

A function nested in an initializer can be called in phase 1 and reference uninitialized member fields and modify already-initialized ones.

[edited by blc to add new future based on discussion in #13171]

Steps to Reproduce

Source Code:

record R {
  var x: int;
  proc init() {

    proc helper(c: int) {
      writeln(c, ": ", this.x);
      this.x = 7;
    }

    helper(1);
    x = 3;
    helper(2);
    super.init();
  }
}

var r = new R();
writeln(r);

Compile command: chpl inithelper.chpl

Execution command:

./inithelper -nl 1
1: 0
2: 3
(x = 7)

Associated Future Test(s):

test/classes/initializers/phase1/nested-function-incorrect.chpl #13182 test/classes/initializers/phase1/nested-function-mods-field1.chpl test/classes/initializers/phase1/nested-function-mods-field2.chpl test/classes/initializers/phase1/nested-function-phase-2-mods-field.chpl

test/classes/initializers/generics/phase1/nested-function-mods-field1.chpl test/classes/initializers/generics/phase1/nested-function-mods-field2.chpl test/classes/initializers/generics/phase1/nested-function-phase-2-mods-field.chpl

Configuration Information

fortytwo@magrathea:~/src/chapel (master)$ chpl --version
chpl Version 1.16.0 pre-release (a22daf8)
Copyright (c) 2004-2017, Cray Inc.  (See LICENSE file for more details)
fortytwo@magrathea:~/src/chapel (master)$ printchplenv --anonymize
CHPL_TARGET_PLATFORM: linux64
CHPL_TARGET_COMPILER: gnu
CHPL_TARGET_ARCH: native *
CHPL_LOCALE_MODEL: flat
CHPL_COMM: gasnet *
  CHPL_COMM_SUBSTRATE: udp
  CHPL_GASNET_SEGMENT: everything
CHPL_TASKS: qthreads
CHPL_LAUNCHER: amudprun
CHPL_TIMERS: generic
CHPL_UNWIND: none
CHPL_MEM: jemalloc
CHPL_MAKE: make
CHPL_ATOMICS: intrinsics
  CHPL_NETWORK_ATOMICS: none
CHPL_GMP: gmp
CHPL_HWLOC: hwloc
CHPL_REGEXP: re2
CHPL_WIDE_POINTERS: struct
CHPL_AUX_FILESYS: none
fortytwo@magrathea:~/src/chapel (master)$ gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

[lydia-duncan]

This is a known issue :( see: https://github.com/chapel-lang/chapel/blob/master/test/classes/initializers/phase1/nested-function-mods-field2.chpl https://github.com/chapel-lang/chapel/blob/master/test/classes/initializers/phase1/nested-function-phase-2-mods-fields.chpl https://github.com/chapel-lang/chapel/blob/master/test/classes/initializers/phase1/nested-function-mods-field1.chpl And their duplicates in the generics directory

[cassella]

Oops, sorry about that. :(

Should I close this issue? Alternately, I don't see an open issue that mentions those futures -- should I leave this one open?

[lydia-duncan]

Go ahead and leave the issue open, I've not been as disciplined about opening issues on initializer bugs as I have been on writing futures (and I still have a long list of tests that I have thought about and haven't written yet...)

[cassella]

I updated the issue description with those futures. Thanks for pointing them out -- I should have looked before opening the issue.

Since the helper gets an implicit this argument (I'm not sure if it's technically a method), I was surprised that it could be called in phase1, due to phase1 disallowing calls to methods, or passing this as an argument generally.

[lydia-duncan]

There's 300+ tests in the initializers directory, 56 of them being futures. Looking through them is not necessarily reasonable, lol.

Our detection of methods is not necessarily as robust as it could be, since there are some cases where the method name is just an "unresolved symbol" until we reach function resolution, and the rules are checked earlier. I haven't thought deeply about how to solve that problem yet, but there are definitely other cases where methods aren't noticed.

Still, I don't really consider nested functions to be methods. Their visibility means they are only useful within the bounds of the function in which they are defined. I think as long as they don't modify fields or access fields, or invoke the super.init or this.init calls when they are called in Phase 1, they should be fine, but that's open to debate.

[cassella]

Hmm. A function that doesn't modify or access class fields sounds like a type method. But the compiler concurs in your judgment that nested functions aren't methods,

  proc init() {
    proc type helper(c: int) {

inithelper.chpl:5: error: 'this' intents can only be applied to methods

(I was thinking that such a type nested function could be called in phase 1 or 2, but a regular nested function only in phase 2. But even if the former existed, the "unresolved symbol" issue would hinder enforcing the distinction.

OTOH, with unrestricted nested functions allowed in phase 1, a careful user could write a helper that accesses only fields that have been initialized prior to all the callsites of the helper.)

[lydia-duncan]

I don't think nested type methods should be allowed. If you're in a scope where you can see the nested function (i.e. inside the initializer), then you aren't operating on the type - you are operating on an instance of the type. So it doesn't make sense to be able to define a type method in that context.

[cassella]

Since the helper gets an implicit this argument (I'm not sure if it's technically a method),

Ahh! #10466 gave me a clue: it's not getting an implicit this argument like a method -- it just has access to the this in init()'s scope as it would to any local variable in init(). I think?

While I'm here, in reviewing these futures in light of #9178, nested-function-mods-field1 and nested-function-mods-field2 (taken literally) both take the compiler to task for not detecting field reinitialization in a helper function, when that reinitialization would be legal if written in the initializer itself.

An example of an illegal reinitialization, as noted in #9178, would be if the field being reinitialized was one whose initialization was done implicitly by init()'s phase1 due to the explicit initialization of a later field. And indeed, the compiler doesn't complain about it in a nested function as it does when it's in init() directly:

class Foo {
  var field1: int;
  var field2: int;
  var field3: int;

  proc init(val) {
    field2 = 1;
    nested(); // This call shouldn't be allowed in Phase 1
    this.complete();
    proc nested() {
      field1 = 2; // because this modifies a field already implicitly initialized in phase1
      field3 = 3; // field3 gets initialized to 0 after nested() returns
    }
  }
}

proc main() {
  var f = new Foo(13);
  writeln(f);
  delete f;
}

As a bonus unfortunate behavior, nested()'s initialization of field3 to 3 is undone/overwritten by init() after nested() returns. (Maybe something more exciting would happen if the fields were of a type where initialization and assignment have observably different behaviors, or where assignment will error if initialization hasn't happened? I don't know what kind of type that would be.)

{field1 = 2, field2 = 1, field3 = 0}

[lydia-duncan]

Ahh! #10466 gave me a clue: it's not getting an implicit this argument like a method -- it just has access to the this in init()'s scope as it would to any local variable in init(). I think?

That sounds right.

You're right, we haven't updated these after the change in specified behavior. @benharsh - there may be other futures in the test/classes/initializers directory that are no longer applicable. Not sure where that would fall priority-wise, but I'm happy to help if need be

[benharsh]

(Assigning myself for tracking purposes)

I think it would be good to audit the various initializer futures at some point, I'm sure there are some that shouldn't exist anymore.