Warn if extension receiver already has member#17543
Conversation
2041485 to
c521dae
Compare
c521dae to
e9ab309
Compare
|
@jchyb ping for triage. IIUC you're issue supervisor this week? |
e9ab309 to
35f27ec
Compare
35f27ec to
6303e64
Compare
smarter
left a comment
There was a problem hiding this comment.
Oops, looks like I completely missed this. I think this is fine as-is since it shouldn't have any false positives but I left a comment with a possible improvement to avoid false negatives.
| val other = paramTpt.tpe.nonPrivateMember(name) | ||
| if other.exists && sym.denot.info.resultType.matches(other.info) then |
There was a problem hiding this comment.
There's a few situations where this check wouldn't be enough:
-
The method defined in the class is overloaded (
other.existswill return false). -
The parameters in the extension definition are subtypes of the one in the primary definition:
class C { def foo(x: Any) = 0 }
extension (c: C) def foo(x: Int) = 1 // uncallable- Only the first (if any) parameter list matches:
class C { def foo(x: Int)(y: String) = 0 }
extension (c: C) def foo(x: Int)(y: Int) = 1 // uncallableclass C { def foo: Int = 0 }
extension (c: C) def foo: String = "" // uncallable- The primary definition might have a type parameter or implicit/using clause:
class C { def foo(using Int) = 0 }
extension (c: C) def foo = 1 // uncallable- The extension definition might have a type parameter or implicit/using clause in its prefix:
class C { def foo = 0 }
extension (using Int)(c: C) def foo = 1 // uncallable- Borderline (which we may or may not want to warn about): the extension definition might have a type parameter or implicit/using clause in its suffix:
class C { def foo = 0 }
extension (c: C) def foo(using Int) = 1 // only callable by explicitly passing `(using ...)`I think this is solvable by doing something like (using Applications.stripImplicit):
| val other = paramTpt.tpe.nonPrivateMember(name) | |
| if other.exists && sym.denot.info.resultType.matches(other.info) then | |
| def firstExplicitParamTypes(tp: Type) = stripImplicit(tp.stripPoly, wildcardOnly = true).firstParamTypes | |
| val extFirst = firstExplicitParamTypes(sym.denot.info) | |
| val matching = | |
| paramTpt.tpe.nonPrivateMember(name) | |
| .filterWithPredicate: clsMeth => | |
| val clsFirst = firstExplicitParamTypes(clsMeth.info) | |
| extFirst.lazyZip(clsFirst).forall(_ frozen_<:< _) | |
| .nonEmpty |
But I haven't actually tried it.
There was a problem hiding this comment.
Thanks! I was bored and in need of stimulation. If I'm able to work through the cases, I'm sorry in advance that I will request your re-review.
There was a problem hiding this comment.
case 7 is case 6 but where c.foo(using x) typechecks with native member returning { def apply(using X) }. I had to refresh my knowledge, and the reference is not current or precise (as it mentions only when m is missing, not when m is present but expression doesn't typecheck). I'll experiment now with your code suggestion.
6303e64 to
46d6a58
Compare
|
Ha, it errored on an old line of mine. The test almost covered it: member Result type does not drive extension search, so For non-nullary extension and nullary member, consider apply in result. The existing test case |
|
Moved the check to refchecks, where it is more at home. Added some test code and conditions, learned something about extension methods and something about dotty internals! Edit: previously, assumed incorrectly that all scaladoc test failures were spurious. where there is an extension method of the same name on a different type in A. I slapped a nowarn on it. I was hoping to get out for some noontime exercise, but local dotty says you can't annotate an extension! I'm learning so much about extensions today! I guess the annotation goes on the |
97bf8a3 to
badd133
Compare
|
Already missed the narrow window to claim that message ID. |
d947fbf to
db578b5
Compare
|
Followed up on previous review. |
smarter
left a comment
There was a problem hiding this comment.
Otherwise LGTM, thanks for your work!
Co-authored-by: Guillaume Martres <smarter@ubuntu.com>
80d7a03 to
fdb33a8
Compare
|
Last chance for this PR to grab an error ID before other PRs overtake it. #20143 |
|
🥇 |
Backports #17543 to the LTS branch. PR submitted by the release tooling. [skip ci]
|
This PR creates false positives under 3.5.0-RC7. Need to minimize. |
|
I'm sorry, I phrased it incorrectly. These are not false positives, but they create warnings for definitions I use as workaround for lack of imports in an extension method block. Here is an example: class Foo(val someField: Int)
extension (foo: Foo)
def someField = foo.someField
def bar1 = 1 + someField
def bar2 = 2 + someField
def bar3 = 3 + someFieldSince I cannot apply Or alternatively, should we just enable the original extended value fields within each of the extension method's scope? |
Best effort to warn if defining extension method for a type which has a non-private member of the same name and a subsuming signature.
Excludes opaque types (for defining forwarders to a public API) and other alias types (where the type member may override an abstract type member).
Fixes #16743