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

Function argument alias #129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

malthe
Copy link
Contributor

@malthe malthe commented Sep 24, 2018

Pony's shadowing prohibition means that using an object field or method name as a function argument is not allowed. This is a proposal to add an optional clause to function arguments that gives the argument a local alias.

Rendered

@jemc
Copy link
Member

jemc commented Sep 26, 2018

We discussed this on today's sync call. We discussed a lot of different options, but @sylvanc suggested (and @kulibali, @mfelsche, and I agreed) that a good alternative to solve this set of problems is to allow locals to shadow members (fields and methods), with the understanding that members will always be accessible by prefixing with this.

This would make the as workaround unnecessary, and would solve several other problems, while still preventing the worst of shadowing issues.

I mentioned that we'd have to make sure that error messages would try to detect and suggest disambiguation issues when you forget to use this.

@mfelsche mentioned that it would be important to update the tutorial to reflect that change.

@malthe
Copy link
Contributor Author

malthe commented Sep 27, 2018

That seems like a good option. The main reason I went with as was that the this.p() notation was discussed previously: ponylang/ponyc#305 (comment).

But I think this.p() ultimately is the better compromise. It's less verbose and allows writing clearer code, albeit with the risk of p having that ambiguity in what it actually refers to.

@mfelsche
Copy link
Contributor

mfelsche commented Sep 27, 2018

Could we restrict the rule mentioned above a bit more and say, that only parameters should be able to shadow members, not let or var variables?

I have to admit, having slept a night over it, i still have mixed feelings about it.

@SeanTAllen
Copy link
Member

SeanTAllen commented Sep 28, 2018

I'm really uneasy about allowing any sort of shadowing. The only real time I have ever felt the pain of this is when using "where = BLAH" in a function calling and not being able to use the name I wanted because it was the same as a public field of the object.

I think malthe's idea from my understanding addresses this one pain point. I don't like the idea discussed on the sync call of allowing locals to shadow fields and access via this.

@EpicEric
Copy link

I personally prefer an explicit solution (i.e. the proposed RFC) than an implicit one that might lead to refactoring bugs.

That said, I am in favour of this RFC, as it'd be helpful specially for named arguments on a constructor, avoiding using primes both on the API and in the actor/class fields.

@chalcolith
Copy link
Member

I am generally not in favour of this RFC, as I think the use of as is confusing. I would prefer only relaxing shadowing for method names, and perhaps even only in constructors.

The only annoyance I've found with Pony's shadowing rules is when I want to have a parameter in a constructor be the same as a getter method.

@jemc
Copy link
Member

jemc commented Sep 28, 2018

I'm in general not in favor of this RFC as currently written. I'm mentioned in the text of the RFC as recommending that primes be optional when invoking with named arguments, and I still believe this, as I agree with @SeanTAllen that this is the biggest problem to solve here. The RFC points out that this wouldn't solve the problem of primes showing up in documentation and such, but I don't really see that as being a big issue.

I find the as syntax in this RFC to be a really verbose way of solving a problem that is mostly about convenience for the caller. Inconveniencing the function author via verbosity for the convenience of the caller doesn't seem like a great solution, and because it's optional, it's likely that function authors won't bother to do it and the callers will still be left with using primes in their named arguments.

@jemc
Copy link
Member

jemc commented Nov 27, 2018

This was discussed in depth on today's sync call. We're not in favor of this approach to solving the problem, for the reasons already discussed. There are a few alternatives that we discussed that remain on the table for further discussion:

  • Solve the issue by removing named arguments and adding something like named tuples or other value types, which would probably be a great feature for the language, but there's a lot that needs to be thought through with this approach and it's unlikely to land soon.
  • Solve the issue by allowing a field name to be used as a parameter name, which will implicitly assign the value to that field at the start of the method body.
  • Solve the issue by allowing the prime to be optionally omitted from the call site when using named arguments. @SeanTAllen is not in favor of the inconsistency this would bring.
  • Do nothing to solve this issue.

Base automatically changed from master to main February 8, 2021 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants