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

SIP-54 - Multi-Source Extension Overloads. #60

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Mar 10, 2023

@odersky I added your name as author of the proposal, since the proposed spec changes are actually yours.

@julienrf julienrf changed the title SIP-54: Multi-Source Extension Overloads. SIP-54 - Multi-Source Extension Overloads. Mar 13, 2023
@bjornregnell
Copy link

For clarity and completeness perhaps add also what happens with "normal" call syntax, i.e. add also this line to the example:

meth(foo) // works with this SIP; "ambiguous import" without it

Also, I wonder what the situation is if extension methods are used from Java. Is the situation improved there as well?

@odersky
Copy link
Contributor

odersky commented Mar 13, 2023

For clarity and completeness perhaps add also what happens with "normal" call syntax, i.e. add also this line to the example:

I agree. In particular since it does not work as you might expect:

meth(foo)      // always gives "ambiguous import"

@bjornregnell
Copy link

Hmm. That is surprising. Is it to complex to fix that as well?

@soronpo
Copy link
Contributor

soronpo commented Mar 13, 2023

Hmm. That is surprising. Is it to complex to fix that as well?

I would actually expect this to be ambiguous, just like regular definitions are in this kind of application.

@sjrd sjrd force-pushed the multi-source-extension-overloads branch from 512ef48 to 983f56a Compare March 13, 2023 12:59
@odersky
Copy link
Contributor

odersky commented Mar 13, 2023

Hmm. That is surprising. Is it to complex to fix that as well?

Yes, that would require a completely different approach which would likely interact badly with lots of other things. The trick is that we enter the special import node only if we have already parsed e.m and now try a m(e).

@bjornregnell
Copy link

Ok I see. Well the current docs says more or less that e.m is just another way of m(e) so people might think that they are equivalent. Hence I think this "irregularity" should be explained in docs as well - should proposed docs text be part of this SIP too?
https://docs.scala-lang.org/scala3/reference/contextual/extension-methods.html#translation-of-extension-methods

@bjornregnell
Copy link

"So, the definition of circumference above translates to the following method, and can also be invoked as such"

@soronpo
Copy link
Contributor

soronpo commented Mar 14, 2023

@odersky Can you please elaborate on the motivation why extension methods are available to the namespace like regular methods? I think scala/scala3#17659 raises a real issue of namespace pollution, and I wonder if we should just remove the ability to call extension methods like regular methods altogether (it's perfectly fine that underneath the hood they are implemented as regular methods).

@odersky
Copy link
Contributor

odersky commented Mar 14, 2023

Extension methods have to be regular methods with some name, since we need to abstract over them. I.e.

trait SemiGroup[T]:
  extension (x: T) def combine (y: T): T

trait Monoid[T] extends SemiGroup[T]:
  def unit: T

given Monoid[String] with
  extension (x: String) def combine (y: String): String = x.concat(y)
  def unit: String = ""

I considered several alternatives of mangling the name somehow but these are all unappealing. Mangling would not help in the situation here anyway since we are talking about namespace conflicts with two different extension methods.

@bishabosha
Copy link
Member

bishabosha commented Mar 14, 2023

I considered several alternatives of mangling the name somehow but these are all unappealing. Mangling would not help in the situation here anyway since we are talking about namespace conflicts with two different extension methods.

doesn't the extension flag count as mangling (for purpose of name resolution, not erasure conflicts) completions/name resolution for unqualified ident foo could filter out by the extension flag

@sjrd
Copy link
Member Author

sjrd commented Mar 14, 2023

"So, the definition of circumference above translates to the following method, and can also be invoked as such"

Good point. I think we'll need to qualify that a bit. Probably something like: translates to the following method or a more explicitly qualified version thereof, and can also be invoked as such.

@bjornregnell
Copy link

bjornregnell commented Mar 14, 2023

doesn't the extension flag count as mangling

Perhaps it is not too strange if the normal method was named something like extension_circumfence(c: Circle): Double if that can help (?) to make multi-imported overloaded normal calls extension_meth(obj, p) as non-ambiguous as extension calls obj.meth(p) by this SIP. And that might also help the "name-space-polution" issue. And it would help discoverability in the REPL using TAB.

@prolativ
Copy link
Contributor

@bjornregnell do you currently have any problems with discoverability of extension methods in REPL? Not sure what you would like to improve here.

@sjrd
Copy link
Member Author

sjrd commented Mar 14, 2023

Perhaps it is not too strange if the normal method was named something like extension_circumfence(c: Circle): Double if that can help (?)

That's not something we can do in a TASTy nor binary compatible way anymore.

@bjornregnell
Copy link

bjornregnell commented Mar 14, 2023

problems with discoverability

image

@prolativ It is not easy to know, when pressing tab, which methods are extension methods. But that's maybe not so often you want to know that...

@sjrd
Copy link
Member Author

sjrd commented Apr 18, 2023

I added a commit to address the recent comments from #60 (comment) and #60 (review)

@soronpo
Copy link
Contributor

soronpo commented Apr 26, 2023

On Apr 21st the SIP committee discussed the SIP and accepted the proposal.
It provides a good tradeoff between limited complexity and language consistency when enabling extension methods with the same name to be imported from different scopes while regular methods cannot. It is hoped that this is another good step towards deprecating implicit classes in the future in favor of extension methods.
Thank you very much for the efforts!

@bjornregnell
Copy link

In addition to the excellently succinct summary by @soronpo, I'd like to add that there are two specific points from the discussion that I think should be addressed (see details in the thread above):

  1. Update of the docs to explain the semantic differences between calling an extension method as an extension or a "normal" method, as introduced by this SIP.
  2. Improvement of error messages when ambiguity errors arise due to overloading to inform the user that this is because it is an extension method and explain the reason for the ambiguity (that the extension method is called as a "normal" method etc)

@odersky
Copy link
Contributor

odersky commented May 8, 2023

@bjornregnell I am not sure how to improve the error messages in this case. Do you have a specific usage scenario where you would like a change?

@bjornregnell
Copy link

@odersky Yes when you get an error because of overloading ambiguity when called as a normal method it would be good if the error message explains that it is actually an extension method. It would also be good if the error message can tell if it would be unambiguous to call it as an extension. The use case is as follows: you have discovered the method as a name e.g. in docs or in repl or by ide and you choose to call it as a normal method but then the overloading ambiguity kicks in and you are confused if you do not know that it is actually a name of an extension and you want help to understand what can be done about the ambiguity error...
Does this makes sense?

@bjornregnell
Copy link

See also above #60 (comment)

@odersky
Copy link
Contributor

odersky commented May 8, 2023

Thanks for clarifying! yes, I think we can do that.

@julienrf
Copy link
Contributor

julienrf commented May 9, 2023

Dear @smarter, @bjornregnell, and @soronpo. The authors of the proposals would like the Committee to vote on promoting the proposal to a stable feature. Do you agree to request a vote in our next SIP meeting? If yes, what would be your recommendation?

@julienrf
Copy link
Contributor

julienrf commented May 9, 2023

FTR, I’ve tried with IntelliJ (231.8109.175), and it supports the feature. The code example given in the content of the proposal is correctly highlighted (no red squiggle), the extension method is listed in the auto-completion popup, and “go to definition” works as expected.

I wanted to also try with Metals but it seems Metals does not support nightly versions of Scala 3. I expect the feature will just work out of the box in Metals when it becomes available in a non-nightly compiler version. Edit: I managed to use Metals, and I confirm that the feature works.

@prolativ
Copy link
Contributor

prolativ commented May 9, 2023

@julienrf as far as I know Metals should support 5 latest nightly versions of the compiler (probably with a delay of 1 day, for some reason, unless this problem has already got solved). But you would also have to use the nightly version of Metals for this to work.

@bjornregnell
Copy link

Do you agree to request a vote in our next SIP meeting? If yes, what would be your recommendation?

Yes I agree and my recommendation is to vote yes.

@soronpo
Copy link
Contributor

soronpo commented May 9, 2023

Dear @smarter, @bjornregnell, and @soronpo. The authors of the proposals would like the Committee to vote on promoting the proposal to a stable feature. Do you agree to request a vote in our next SIP meeting? If yes, what would be your recommendation?

Yes to both questions

@odersky
Copy link
Contributor

odersky commented May 9, 2023

See scala/scala3#17441

@bjornregnell
Copy link

bjornregnell commented May 9, 2023

A better usability of err messages is not a show-stopper, but it is a bit unsatisfactory that an implementation details prevents nicer UX. I would have assumed that the keyword extension would enable the compiler to mark that it is an extension method early on and that it should be a simple if-expression to emit a string like "note this is an extension method...". But if this is not the case and it is too much work or too much architectural turmoil for this SIP then the potential for improved UX can e.g. be deferred to an issue for future fix.

@odersky
Copy link
Contributor

odersky commented May 9, 2023

and that it should be a simple if-expression to emit a string like "note this is an extension method...

Yes, that would be possible. However, we don't know whether this was called as an extension method or not. If we add the note: "this ambiguity would go away if the method is called as an extension method" and the method was in fact called as an extension method (but the error would not be filtered away for some other reason), this would be embarrassing.

@smarter
Copy link
Member

smarter commented May 9, 2023

However, we don't know whether this was called as an extension method or not.

Could we compare the span of the extension method with the span of its argument to figure that out? In any case, I'm +1 to accepting this at the next meeting.

@bjornregnell
Copy link

bjornregnell commented May 9, 2023

this would be embarrassing

Good then that compilers don't have consciousness (yet) and can't get embarrassed :)

I'd assume that the call-site could lookup the declaration and see if it's an extension or not, but I don't know the implementation so this comment might be off.

Anyway, the comment could be a hint rather than very certain, e.g. "hint: this error may arise if xxx is an extension called as a normal method" which might be less "embarrassing" and just "noise" if its not the case. But I'm not sure if it is worse to introduce noise and a potential false positive. So better if the hint can be reliably given when it actually is an extension.

@odersky
Copy link
Contributor

odersky commented May 9, 2023

I think a hint like that is a good compromise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants