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

Applying parens to .re/.im results in an internal error #26263

Closed
bradcray opened this issue Nov 19, 2024 · 5 comments · Fixed by #26275
Closed

Applying parens to .re/.im results in an internal error #26263

bradcray opened this issue Nov 19, 2024 · 5 comments · Fixed by #26275

Comments

@bradcray
Copy link
Member

Summary of Problem

Description:

When calling .re or .im on an expression with type complex, an internal error is generated. A user-facing error should presumably be generated instead.

var c: complex = 1.2 + 3.4i;

writeln(c.re());  // generates an internal error

In developer mode, the internal error is of the form:

internal error: Cannot call insertAfter on Expr not in a list [AST/expr.cpp:404]

Interestingly, this doesn't happen for user-defined paren-less methods, suggesting it's something special about complexes or these methods, as built-ins:

record R {
  proc foo {   // similar result whether this is returning a literal, as here, or a ref or const ref to a field
    writeln("In R.foo");
    return 4.2;
  }
}

var myR = new R();
writeln(myR.foo());  // generates a user-facing error as expected

Steps to Reproduce

Associated Future Test(s):
test/types/complex/parenfulReIm.chpl #26262

Configuration Information

  • Output of chpl --version: chpl version 2.3.0 pre-release (10cc4042fd)
@jabraham17
Copy link
Member

I can replicate this with a user defined parenless method. Having two overloaded parenless methods with differing return intents seems to be the root cause.

record R {}
var r = new R();
var myConstant = 1.0;

proc R.foo {
  return myConstant;
}
proc R.foo ref {
  return myConstant;
}

r.foo();

@bradcray
Copy link
Member Author

Aha, thanks for that insight, Jade! I thought I was being very clever for thinking of return intents at all, but gave up too soon…

@DanilaFe DanilaFe self-assigned this Nov 20, 2024
@DanilaFe
Copy link
Contributor

Taking look as a little treat for myself :)

@DanilaFe
Copy link
Contributor

I've got a fix with the associated tests (#26275); if that merges before #26262, please consider not including the future :)

@bradcray
Copy link
Member Author

Thanks Daniel! I can just close my PR is you prefer.

DanilaFe added a commit that referenced this issue Nov 20, 2024
Closes #26263.

The issue in question is that we get an internal error while invoking a
return-intent-overloaded parenless procedure.

```Chapel
record R {}
var r = new R();
var myConstant = 1.0;

proc R.foo {
  return myConstant;
}
proc R.foo ref {
  return myConstant;
}

r.foo();
```

At the surface level, this is caused by the code that attempts to do
return intent overloading; it assumes that the overloaded call is a part
of a list. When a field is being invoked, like `r.foo()`, the overloaded
call (`r.foo`) is actually the base expression, which is not in any
list.

I think it's conceivable for this combination of features to be used in
user code; if `myConstant` wasn't an integer but actually a record/class
with `proc this` defined on it, `r.foo()` would invoke that `proc this`,
and that seems reasonable enough. So, my approach was to make this case
"work", up to the type-correctness of invoking the `proc this`.

To that end, this PR adds some extra checking to handle the case where a
return-intent-overloaded call is the base expression of another call.
This required two primary changes; the `insertAfter` diff is the first,
and the `partialParent->baseExpr = contextCall;` is the other.

This gets us further, but code for invoking `proc this` doesn't expect
the base expression to be a `ContextCall` / overloaded call either. This
PR adjusts that as well.

Reviewed by @lydia-duncan -- thanks!

## Testing
- [x] both tests from 26263
- [x] paratest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants