conditional with `throw` branch throws off use-before-def analysis

[bradcray]

Summary of Problem

I believe the following program should be legal:

proc testit2(d:domain) {
  return d;
}

proc testit(r: range(stridable=?)) throws {
  var dom: domain(1, stridable=true);
  if !isBoundedRange(r) {
    throw new owned IllegalArgumentError('input range must be bounded');
  } else {
    dom = {r};
  }
  return testit2(dom);
}

writeln(testit(1..));

Yet today, it is reporting:

testit.chpl:5: In function 'testit':
testit.chpl:12: error: 'dom' is used before it is initialized
testit.chpl:6: note: 'dom' declared here

due to a problem in use-def analysis. Adding an initialization of dom in the then clause fixes the problem, as does not passing dom on to testit2(), simply returning it directly instead.

Michael notes that callDestructors has some use-before-def checking (which is where this error is being printed), and that it might make sense to move all of it there because it's after the split-init processing occurs.

Steps to Reproduce

Associated Future Test(s):

Stay tuned...

Are there any tests in Chapel's test system that demonstrate this issue? e.g. test/errhandling/codePathCoverage/ifOrThrow2.chpl #15699

Configuration Information

• Output of chpl --version: chpl version 1.23.0 pre-release (dc8c98a9ae)

[vasslitvinov]

@bradcray or others - why does this code throw instead of issuing a compilerError ?

Here theif is controlled by a compile-time condition. If we change it to a run-time condition, the def-use / split-init analyses work fine.

The same question applies to proc channel.seek() and to proc choice() in Random https://github.com/chapel-lang/chapel/issues/14683#issuecomment-1224255458

[bradcray]

@vasslitvinov : I think you're right that in the original code motivating this issue, a compilerError() would be more appropriate than throwing. Looks like https://github.com/chapel-lang/chapel/pull/15154 introduced this code and that if we took your approach we could remove the workaround I took in #15699 of putting in a dummy return value.

That said, it seems like we should not prevent a user from writing this code and having it behave reasonably—would you agree?

[Noting for those who haven't tried, that it looks as though the behavior of the code in the OP has changed from a use-before-def complaint into a complaint about using testit() when it doesn't return a value:

testit.chpl:1: In module 'testit':
testit.chpl:15: error: illegal use of function that does not return a value: 'testit'

]

[vasslitvinov]

the behavior of the code in the OP has changed from a use-before-def complaint into a complaint about using testit() when it doesn't return a value

This is due to #20497. Given that return testit2(dom) is unreachable, the compiler now ignores it. testit() now looks to a compiler like a function without return statements and without a declared return type. So the compiler considers it a function that does not return a value.

we should not prevent a user from writing this code and having it behave reasonably

I agree. A testit()-like function could be part of a generic library. The question is: what is "reasonable behavior" in this case?

Since we ignore unreachable code, it is reasonable to infer that this function does not return a value in this case. If the user wants to pass this function's result to a writeln(), they need to declare its return type explicitly.

[bradcray]

Ah, I see, thanks for the explanation. Yeah, I agree that the current behavior is reasonable (definitely way more reasonable than the behavior in the OP). I think we could retire this future, locking in the current behavior, and close this issue. Ideally, it'd also be nice to remove the workaround(s) introduced in https://github.com/chapel-lang/chapel/pull/15699 (where probably the way to do so would be to replace a throws with a compilerError() as you suggest here. It was the introduction of that workaround that led me to open this, and it hadn't occurred to me to use a compilerError() since the throws was pre-existing... I apparently didn't think about it hard enough to see that that was an option).