chapel-lang / chapel

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

[Bug]: implicit this does not work in some secondary methods #26083

Open jabraham17 opened 1 month ago

jabraham17 commented 1 month ago

The language spec states that identifiers can be used in a method without an explicit this. and still resolve. This is offered without qualification in the spec, and an example is shown with secondary methods. However I have found that secondary methods with more complicated receivers fail to resolve.

The following code shows an example of this. In foo, implicit this works properly. However both bar and baz fail to compile since x/t is not considered in scope. Note that in all cases, adding an explicit this. causes all functions to resolve properly.

record R {
  type t;
  var x: t;
}

proc R.foo() {
  writeln("foo ", x); // this is fine
}
proc (R(int)).bar() {
  writeln("bar ", x); // this is not fine
}

proc getR type do return R(int);
proc type getR.baz() {
  writeln("baz ", t:string); // this is not fine
}

var r = getR;
r.foo();
r.bar();
r.type.baz();

I think that in all three functions implicit this should work, and x/t should resolve to this.x/this.t.

mppf commented 1 month ago

The bar case is a known issue (well, a known-to-me issue, not sure if that makes it "known"...)

When PR #5057 added support for methods on instantiated types with parentheses (as you use above), it didn't handle the implicit this. part. The reason for this has to do with the production compiler's architecture, where implicit this. is added before type resolution has been completed.

The proc type getR.baz() case is similar, in that there is a non-trivial receiver expression. It doesn't use parens, but it's still more complicated than the production compiler's architecture can handle.

AFAIK the Dyno resolver will address both of these when it comes online. So it'd be good to have .futures.

Along those lines, I know of these futures:

jabraham17 commented 1 month ago

Ok, with that in mind I have opened https://github.com/chapel-lang/chapel/pull/26084 to add futures for this case so that these cases don't get forgotten as Dyno comes online.

I expanded test/functions/ferguson/spec-insn-method-no-this.chpl to handle the baz case as well and added the similar test/functions/ferguson/spec-insn-type-method-no-this.chplto handle thetype` method cases.

mppf commented 1 month ago

See also issue #5979 which describes some of the cases here (and this issue is arguably a duplicate of that one, but IMO we should keep both because they emphasize different things).