search

chapel-lang / chapel

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

internal error when using named argument after a `none`-defaulted type #20373

Open jhh67 opened 2 years ago

jhh67 commented 2 years ago

Summary of Problem

I got the following error

examples/baz.chpl:12: In method 'encode':
examples/baz.chpl:13: internal error: RES-ADD-LLS-0805 chpl version 1.28.0 pre-release (0085fba89a)

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.

  examples/baz.chpl:18: called as Foo.encode(key: nothing, value: int(64))
note: generic instantiations are underlined in the above callstack

Steps to Reproduce

Source Code:

class Encoder {
  // Encodes something, with optional key
  proc encode(ch:Foo, key:?t = none, x) : void throws {writeln("base");}
}

class DefaultEncoder: Encoder {
  override proc encode(ch:Foo, key:?t = none, x) : void throws { writeln("default");}
}

record Foo {
    var encoder: owned Encoder = new DefaultEncoder();
    proc encode(key:?t = none, value) {
        this.encoder.encode(this, key, value);
    }
}

var foo = new Foo();
foo.encode(value=42);

Compile command: chpl baz.chpl

Execution command: ./baz

Associated Future Test(s):

Configuration Information

bradcray commented 2 years ago

Strangely, the developer view of that error looks like it could almost be a user error, except that the counts reflect internal arguments that a user wouldn't be aware of:

testit.chpl:12: In method 'encode':
testit.chpl:13: internal error: number of actuals (5) does not match number of formals (4) in encode() [resolution/addAutoDestroyCalls.cpp:805]
  testit.chpl:18: called as Foo.encode(key: nothing, value: int(64))
note: generic instantiations are underlined in the above callstack

[edit: I was thinking that the mismatch actually was a user error, but in reading too quickly had missed that the defaulted argument + named passing at the callsite means that it is actually legal]

mppf commented 2 years ago

I looked at this a bit in the debugger & I think that the problem is that the none formal got removed from the formals but not from the call at this point in resolution.

bradcray commented 2 years ago

Belated realization: In:

    proc encode(key:?t = none, value) {

the default value of none suggests that the only legal type for key: ?t is nothing which suggests that that first argument isn't actually very useful, whether using the default or passing none in explicitly. Of course, we still shouldn't generate an internal error for it, but just noting that this isn't giving a completely generic option type as a programmer coming with a language that supported them might think—right?

Tagging @daviditen on this who I believe implemented these features (and may know something about the removal of the arguments, if it's at all related to our removal of void types and values (?).

mppf commented 2 years ago

the default value of none suggests that the only legal type for key: ?t is nothing which suggests that that first argument isn't actually very useful, whether using the default or passing none in explicitly.

The point of it is that you can pass in something other than none and it will accept it. That is what we get out of writing key:?t. We would not get that behavior if the ?t is dropped and it is just key=none.

bradcray commented 2 years ago

Oh gosh, you're right! I was under the mistaken impression that formal: ?t = defaultExpr required t to be a type that could legally be assigned a defaultExpr. But maybe my brain is just stuck in prehistoric times here and failed to digest a more recent change? [that's your cue to embarrass me further by telling me how long ago that change occurred]

Did you happen to ascertain why the none formal got removed? I'm specifically wondering whether it is related to void/none/nothing being eliminated from the compiler as an optimization or not. Replacing none with 42 seems to cause the error to go away, suggesting it probably is (?). In which case, @daviditen might be a very logical owner for fixing this.

bradcray commented 2 years ago

(I edited the issue title to try and make it a bit more specific to the use of none... assuming I didn't misdiagnose anything)

mppf commented 2 years ago

that's your cue to embarrass me further by telling me how long ago that change occurred

Definitely not a goal here. Of course any of us could try some old releases to see. I thought it was always this way though...

Did you happen to ascertain why the none formal got removed?

no

I'm specifically wondering whether it is related to void/none/nothing being eliminated from the compiler as an optimization or not.

I think it is related to that logic, but I haven't dug in further to know for sure.

bradcray commented 2 years ago

Definitely not a goal here.

I wasn't implying that was your goal, just that it was the way I'd feel if we had discussed and changed it at some point and I'd forgotten. :) I haven't bothered to go back and try to find out.

@daviditen: Still hoping from a word for you as to whether you agree that the void/none removal pass is likely to be the issue here, and if so whether it might have a simple fix.

daviditen commented 2 years ago

Yes, it looks like the nothing formal is being removed but the none value being passed isn't. I'm not positive what about this case is different from the cases that are working correctly, but the named argument and the generic on the none formal are likely culprits.