Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is lvalue checking too strict? #26685

Open
mppf opened this issue Feb 10, 2025 · 2 comments
Open

is lvalue checking too strict? #26685

mppf opened this issue Feb 10, 2025 · 2 comments

Comments

@mppf
Copy link
Member

mppf commented Feb 10, 2025

Summary of Problem

Description:
I have a subroutine in my program that can introduce parallel work, however it is is sometimes called within another parallel section, and in that case I don't want it to introduce parallelism. However it is nice if the same code can be used for both.

At the same time, the subroutine needs to use an aggregator (from CopyAggregation). When it is called in a parallel section, I need to pass the aggregator from the outer parallel section, to avoid aggregation creation/destruction overheads.

I was expecting that in this case the subroutine could accept a generic formal argument that is 'none' in the case it should create its own aggregator and otherwise an aggregator that can be used that was allocated in the calling loop. However, I am running into lvalue-checking errors & as a result I can't figure out a way to write this. I'm aware of a possible workaround which is to duplicate the subroutine into a parallel and non-parallel version. Another workaround is to run new R() even when the code should use the outer variable, so that it is placed into a variable.

So, I am trying to write something like the example below. Should it be possible to write something like this? My reading of the spec, as it stands today, is that it should be a compilation error.

Is this issue currently blocking your progress?
No

Steps to Reproduce

Source Code:

config param useOuter = false;

proc main() {
 
  var x: R;
  x.v = 1;

  {
    writeln("nested");
    ref xx = if useOuter
             then x
             else new R(); // error: Cannot set a non-const reference to a const variable.

    writeln(xx);
  }
}

record R {
  var v: int;
  proc init() {
    writeln("R.init()");
    v = 42;
  }
  proc init=(const ref rhs: R) {
    writeln("R.init=()");
  }
  operator =(ref lhs, const ref rhs) {
    writeln("R.=");
  }
}

A similar that puts the new R() into a helper routine fails in the same way:

config param useOuter = false;

proc main() {
 
  var x: R;
  x.v = 1;

  {
    writeln("nested");
    ref xx = if useOuter
             then x
             else createR(); // error: Cannot set a non-const reference to a const variable.

    writeln(xx);
  }
}

proc createR() {
  var x: R;
  return x;
}

record R {
  var v: int;
  proc init() {
    writeln("R.init()");
    v = 42;
  }
  proc init=(const ref rhs: R) {
    writeln("R.init=()");
  }
  operator =(ref lhs, const ref rhs) {
    writeln("R.=");
  }
}

Here is a workaround to the issue that stores the result of new R() into a local variable:

config param useOuter = false;

proc main() {

  var x: R;
  x.v = 1;

  {
    writeln("nested");
    var myR = if useOuter then none else new R();
    ref xx = if useOuter then x else myR;

    writeln(xx);
  }
}

record R {
  var v: int;
  proc init() {
    writeln("R.init()");
    v = 42;
  }
  proc init=(const ref rhs: R) {
    writeln("R.init=()");
  }
  operator =(ref lhs, const ref rhs) {
    writeln("R.=");
  }
}

Compile command:
chpl foo.chpl

Associated Future Test(s):

TODO

Configuration Information

chpl 2.4 pre-release

@mppf mppf added the type: Bug label Feb 10, 2025
@bradcray
Copy link
Member

For the first case:

    ref xx = if useOuter
             then x
             else new R(); // error: Cannot set a non-const reference to a const variable.

remind me what we ended up doing w.r.t. expression-level compiler-introduced temporaries? If it's guaranteed that they exist through the end of the lexical scope. then permitting this seems reasonable to me, where we'd effectively be saying that it's OK to take a reference to a compiler-introduced temporary. This seems analogous to the following, which we seem to support (but I think used to not, maybe for similar concerns about refs to compiler-introduced temps?)

record R { var x = 42; }
proc foo(ref x) { writeln("x is: ", x); }
foo(new R());

And if we permit the first case, I think we permit the second as well (both seem like they're cases of the compiler needing to introduce a temp to capture the result of an expression—one just happens to be a new expression and the other a call expression.

@mppf
Copy link
Member Author

mppf commented Feb 18, 2025

remind me what we ended up doing w.r.t. expression-level compiler-introduced temporaries?

From https://chapel-lang.org/docs/language/spec/variables.html#deinit-points --

If the containing statement is an initialization expression for a ref or const ref, such as const ref x = f(g());, then the temporary local variables for that statement are deinitialized at the end of the containing block. Otherwise, the temporary local variables are deinitialized at the end of the containing statement.

It's not a given that this is working correctly for if expressions, but if it's not, I think that's a bug and not a language design issue.

This seems analogous to the following, which we seem to support (but I think used to not, maybe for similar concerns about refs to compiler-introduced temps?)

Actually that might be a bug; or at least it's an inconsistency. We get the error if we call a function instead of new:

record R { var x = 42; }
proc foo(ref x) { writeln("x is: ", x); }
foo(new R()); // compiles without issue

foo(bar()); // error: non-lvalue actual is passed to 'ref' formal 'x' of foo()

proc bar() { return new R(); }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants