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

Type.to_text shall invoke instance method #6300

Closed
JaroslavTulach opened this issue Apr 16, 2023 · 6 comments · Fixed by #6441
Closed

Type.to_text shall invoke instance method #6300

JaroslavTulach opened this issue Apr 16, 2023 · 6 comments · Fixed by #6441
Assignees
Milestone

Comments

@JaroslavTulach
Copy link
Member

To turn the #6216 discussion into some practical resolution I propose following improvement:

If a method x is defined on Any, then the only "static call" syntax is Any.x. Every other form is an instance invocation. E.g. My_Type.x invokes the instance method with self=My_Type (that's actually the current behavior). The same way With_X.x should be the instance invocation (that has to change for types that override x).

The only identified drawback is that we don't have a way to invoke With_X.x statically. Let's ignore that, fix the above problem (which is really painful) and see whether we need to fix the identified drawback as well (so far we aren't aware of any such need).

@somebody1234
Copy link
Contributor

should the title say "instance method", not "static method", or am i just reading it wrong?

@wdanilo
Copy link
Member

wdanilo commented Apr 18, 2023

The discussion is not finished and we are chatting about this design with Radek actively. The change proposed by Radek is incompatible with how types are intended to work and this topic needs a lot more work/thinking before making such radical changes. We should revisit this topic in approx 1 week.

@JaroslavTulach
Copy link
Member Author

The change proposed by Radek is incompatible ... radical ... needs more thinking ...

@wdanilo please note that this issue isn't implementing Radek's proposal. No incompatible changes, no syntax changes, no radical changes.

This issue proposes minimal change and only affects how methods defined on Any behave. There is just a few methods on Any - e.g. the impact of this change is very limited.

We should revisit this topic .... (later)

We already have at least three duplicates and counting. The current behavior of Any-methods on Type object is contra-intuitive and we need a fix. The proposed fix isn't affecting non-Any methods - e.g. the main topic of the #6216 discussion isn't affected by the change proposed in this issue. Please let us move on, let us prepare a PR for review. Continue thinking, discussing #6216 in parallel.

@JaroslavTulach
Copy link
Member Author

I've just chatted with @wdanilo and Wojciech is fine with us implementing this issue, as long as it only affects dispatch of methods defined on Any.

@hubertp hubertp moved this from 📤 Backlog to 🔧 Implementation in Issues Board Apr 25, 2023
hubertp added a commit that referenced this issue Apr 26, 2023
This change modifies method dispatch for methods that override Any's
definitions. When an overrided method is invoked statically we call Any's
method to stay consistent.
This change primarily addresses the plethora of problems related to
`to_text` invocations. It does not attempt to completely modify method
dispatch logic.

Closes #6300.
hubertp added a commit that referenced this issue Apr 26, 2023
This change modifies method dispatch for methods that override Any's
definitions. When an overrided method is invoked statically we call Any's
method to stay consistent.
This change primarily addresses the plethora of problems related to
`to_text` invocations. It does not attempt to completely modify method
dispatch logic.

Closes #6300.
@hubertp hubertp moved this from 🔧 Implementation to 👁️ Code review in Issues Board Apr 27, 2023
@enso-bot
Copy link

enso-bot bot commented Apr 27, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-04-26):

Progress: Investigating special logic for methods that override Any's definitions. Draft PR is ready, most of corner cases seem to be covered. It should be finished by 2023-04-27.

Next Day: Next day I will be working on the #6300 task. Investigating a, potentially simpler solution, that involves first resolved the symbol for Any and then for self's type.

@enso-bot
Copy link

enso-bot bot commented Apr 27, 2023

Hubert Plociniczak reports a new STANDUP for today (2023-04-27):

Progress: More testing, addressing PR review. Went back to #6257 to address PR review. Investigating various LS bugs like #6447 or #6395 It should be finished by 2023-04-27.

Next Day: Next day I will be working on the #6395 task. Debugging LS bugs.

@mergify mergify bot closed this as completed in #6441 Apr 28, 2023
mergify bot pushed a commit that referenced this issue Apr 28, 2023
This change modifies method dispatch for methods that override Any's definitions. When an overrided method is invoked statically we call Any's method to stay consistent.
This change primarily addresses the plethora of problems related to `to_text` invocations. It does not attempt to completely modify method dispatch logic.

Closes #6300.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants