chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.78k stars 418 forks source link

internal error: RES-CLE-UPS-0792 chpl version 1.28.0 #21265

Closed nathanielknight closed 7 months ago

nathanielknight commented 1 year ago

Summary of Problem

While attempting to solve an Advent of Code challenge I triggered an internal compiler error. A minimal reproducing program and the full compiler output is included below.

Steps to Reproduce

Source Code:

// solve.chpl

proc main() {
    const bs = f();
    for b in bs {
        writeln(b);
    }
}

config const inf = "input.txt";

iter f() {
    use FileSystem, IO, FormattedIO;

    var f = open(inf, iomode.r);
    var r = f.reader();
}

Compile command:

$ chpl ./solve.chpl 
internal error: RES-CLE-UPS-0792 chpl version 1.28.0

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.

Configuration Information

$ chpl --version
chpl version 1.28.0
  built with LLVM version 14.0.6
Copyright 2020-2022 Hewlett Packard Enterprise Development LP
Copyright 2004-2019 Cray Inc.
(See LICENSE file for more details)

$ clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin22.2.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

I don't seem to have printchplenv with my installation (I'm on MacOS 13.1).

mppf commented 1 year ago

@nathanielknight - thanks for reporting this bug.

Actually here is an even smaller reproducer:

proc main() {
  const bs = f();
  for b in bs {
      writeln(b);
  }
}

iter f() {
}

I took a peek at how the compiler is behaving here but did not get all the way to a fix. It looks like the compiler is trying to infer the iterator's yielded type to nothing and then later running into problems. As far as a workaround, I would expect that the problem will go away as soon as you implement enough of the iterator to include yield statements.

nathanielknight commented 1 year ago

Interesting, thanks for the swift reply :)

I was able to get this working with more of the iterator implemented; I think I arrived at the above program while trying to debug a bad format string (I commented out the yield in my loop to see if it was actually running). I tried a few variations of this and didn't find a meaningfully different way to trigger this bug. :+1:

bradcray commented 1 year ago

My first reaction was "I could've sworn I've done this before", but I think the difference is invoking a yield-less iterator in a loop, like:

for i in f() do
  writeln(i);

(which works) and capturing it into an inferred-array variable (which apparently doesn't). And thinking about what the type of the array would end up being, I can understand why... like should it be a 0-element array of void or nothing or int or ...?

Of course, the answer is definitely not "It should give an internal error." I think it'd be pretty reasonable to generate an error in this case... though if f() declared its yielded type (i.e., iter f(): int) then it would be preferable to infer it to be a 0-element array of int...

Thanks for filing this, @nathanielknight!

jabraham17 commented 1 year ago

Adding on to this, I just ran into the same issue while ramping up. My reproducer was actually even smaller

iter foo() {}
var x = foo();

The error confused me for a bit until figuring out it was an internal error, a proper error message would definitely help.

bradcray commented 1 year ago

@jabraham17 : If you felt motivated to convert the internal error to a (for now) user-facing error (which I'd support but don't have time for myself), I think it should be as easy as replacing the assertion at cleanups.cpp:792 from an INT_ASSERT() to a conditional that generated a USR_FATAL() instead. I'm imagining an error like "iterators that do not yield cannot currently be used in this way" (or, if we were confident that the code paths that were hitting this line were all cases of initialization variables, we could say "...currently be used to initialize arrays").

If you (or someone) were to do and file your test as a reproducer, it'd be interesting to create a second variation that declares the iterator as iter foo(): int {}. I expect this would behave the same today, but someday, the latter could cause x to be an empty array of int (whereas the first may never be supported because it's not clear what type x should have).

[edit: Sheesh, tagged the wrong developer due to reading too quickly, sorry about that!]

jabraham17 commented 1 year ago

I will add this to my list for next sprint then.

jabraham17 commented 1 year ago

For the sake of this conversation, in testing this further I found that explicitly declaring the return type does not currently fail today. So no need to worry about it later because it is already handled.

iter foo():int {}
var x = foo();
bradcray commented 1 year ago

Oh, even better. I was too pessimistic for once!

dlongnecke-cray commented 7 months ago

I started work on this and could use some clarity on a related issue:

Does Chapel even support arrays of nothing? Should we fold the array type away? Do we keep them - and if so, what is stored in the slot? Does nothing even have a runtime value? Code that tries to declare an array or c_array of nothing all seems to crash.

dlongnecke-cray commented 7 months ago

After thinking about it a bit more:

bradcray commented 7 months ago

Hi David β€”

A few thoughts here:

To your other questions:

Does nothing even have a runtime value?

No, I believe that, currently, none/nothing types and values are folded out of the code at compile-time

Does Chapel even support arrays of nothing?

I'd be surprised if it works and don't see much motivation to support them.

Should we fold the array type away?

Either that, or just assert that they're not supported. I'd probably go with "not supported" because it seems like strictly less work and wouldn't be a breaking change if we decided we wanted/needed to support them later.

dlongnecke-cray commented 7 months ago

Thanks for the great example about how a loop with yields may still never yield. I talked to Jade and they agree that arrays of nothing are a bit confusing, but I don't want to put words in their mouth πŸ˜„.

Right now I'm working on a patch that:

And if there are any existing analysis which determine reachability that I can use, that'd be great πŸ˜„.

jabraham17 commented 7 months ago

We should have @jabraham17 remind us where they left this since they looked at it about a year ago

Essentially where I last left this making iterators with a yield type of nothing cause a nice error, I was running into problems getting that error specific enough and nice enough to not cause false-positives. But I also haven't looked at this in several months.

I talked to Jade and they agree that arrays of nothing are a bit confusing, but I don't want to put words in their mouth

Arrays of nothing are definitely confusing and should probably be an error, because I can't imagine it being what the user intended and probably indicates an error elsewhere (like, an iterator not yielding anything with no explicit type :smile:)

mppf commented 7 months ago

Emits an error if an iterator with zero reachable yields (in the compiler sense) does not annotate its return type

And if there are any existing analysis which determine reachability that I can use, that'd be great πŸ˜„.

I'd expect that you could get to the error you want by adjusting resolveReturnTypeAndYieldedType (at least, that code should handle the details of what yields should be traversed / considered already).

dlongnecke-cray commented 7 months ago

Thanks Michael, I've done exactly that!

bradcray commented 7 months ago

Thanks for the improvements here, @dlongnecke-cray, and thanks for reporting this issue, @nathanielknight!