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

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

Open
jabraham17 opened this issue Oct 14, 2024 · 3 comments
Open

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

jabraham17 opened this issue Oct 14, 2024 · 3 comments

Comments

@jabraham17
Copy link
Member

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
Copy link
Member

mppf commented Oct 14, 2024

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:

  • test/functions/ferguson/spec-insn-method-no-this.chpl (for something like the bar case here)
  • test/classes/ferguson/forwarding/call-forwarded-omit-this.chpl (for a similar case not described here)

@jabraham17
Copy link
Member Author

Ok, with that in mind I have opened #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
Copy link
Member

mppf commented Oct 14, 2024

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).

jabraham17 added a commit that referenced this issue Oct 14, 2024
Adds new futures for #26083

Tested that futures pass locally

[Reviewed by @mppf]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants