Sema: Diagnose unbound method references on 'super.' #30097

slavapestov · 2020-02-27T17:02:22Z

This is something I noticed by inspection while working on
https://bugs.swift.org/browse/SR-75.

Inside a static method, 'self' is a metatype value, so
'self.instanceMethod' produces an unbound reference of type
(Self) -> (Args...) -> Results.

You might guess that 'super.instanceMethod' can similarly
be used to produce an unbound method reference that calls
the superclass method given any 'self' value, but unfortunately
it doesn't work.

Instead, 'super.instanceMethod' would produce the same
result as 'self.instanceMethod'. Maybe we can implement this
later, but for now, let's just diagnose the problem.

Note that partially-applied method references with 'super.'
-- namely, 'self.staticMethod' inside a static context, or
'self.instanceMethod' inside an instance context, continue
to work as before.

They have the type (Args...) -> Result; since the self value
has already been applied we don't hit the representational
issue.

slavapestov · 2020-02-27T17:02:42Z

@swift-ci Please smoke test

slavapestov · 2020-02-27T17:04:41Z

test/NameBinding/name_lookup.swift

    super.baseFunc1(42) // expected-error {{instance member 'baseFunc1' cannot be used on type 'ThisBase1'}}
-    super.baseFunc1(ThisBase1())(42)
+    super.baseFunc1(ThisBase1())(42) // expected-error {{'super' instance member 'baseFunc1' cannot be used on type 'ThisBase1'}}


@xedin @hborla do you know why the diagnostic output is inconsistent here? It's printing baseFunc0() vs baseFunc1

Hm, I don't know why it mentions baseFunc0 here when this expression has nothing to do with it...

Sorry, I meant compared to the line above, super.baseFunc0(ThisBase1())().

Ah, AFAIK that's related to how printer handles names - prints () if there are no arguments but avoids printing (_:) unless you pass it a special option set...

Alright, if this is expected behavior that's fine. I just wanted to make sure I hadn't done something wrong because names got a bit more complicated since the last time I looked at diagnostics :)

Yeah, I think that's expected :)

xedin · 2020-02-27T17:10:17Z

lib/Sema/CSApply.cpp

+      if (isSuper && isUnboundInstanceMember) {
+        context.Diags.diagnose(base->getLoc(),
+                               diag::super_instance_method_on_static_self,
+                               baseTy, DeclNameRef(member->getFullName()))


Can you please make it a fix so we don't even try to apply an invalid (for now) solution? I think it could be added to resolveOverload method.

xedin · 2020-02-27T17:15:36Z

test/NameBinding/name_lookup.swift

    super.baseFunc1(42) // expected-error {{instance member 'baseFunc1' cannot be used on type 'ThisBase1'}}
-    super.baseFunc1(ThisBase1())(42)
+    super.baseFunc1(ThisBase1())(42) // expected-error {{'super' instance member 'baseFunc1' cannot be used on type 'ThisBase1'}}


Hm, I don't know why it mentions baseFunc0 here when this expression has nothing to do with it...

This is something I noticed by inspection while working on <https://bugs.swift.org/browse/SR-75>. Inside a static method, 'self' is a metatype value, so 'self.instanceMethod' produces an unbound reference of type (Self) -> (Args...) -> Results. You might guess that 'super.instanceMethod' can similarly be used to produce an unbound method reference that calls the superclass method given any 'self' value, but unfortunately it doesn't work. Instead, 'super.instanceMethod' would produce the same result as 'self.instanceMethod'. Maybe we can implement this later, but for now, let's just diagnose the problem. Note that partially-applied method references with 'super.' -- namely, 'self.staticMethod' inside a static context, or 'self.instanceMethod' inside an instance context, continue to work as before. They have the type (Args...) -> Result; since the self value has already been applied we don't hit the representational issue.

slavapestov · 2020-02-27T22:33:27Z

@xedin Here's a new approach where we detect the error in the solver instead of CSApply. I had to refactor the existing code somewhat.

This also detects and bans a case we didn't before: unbound references to mutating methods, even with all argument lists applied. Eg, Foo.bar(&self)(). These actually do work today, but they won't with the new curry thunk implementation. Hopefully this won't cause any fallout, if it does, I'll hack something up.

slavapestov · 2020-02-27T22:33:51Z

@swift-ci Please smoke test

slavapestov · 2020-02-27T23:31:02Z

@swift-ci Please test source compatibility

xedin

Thank you!

davezarzycki · 2020-02-28T16:21:57Z

This also detects and bans a case we didn't before: unbound references to mutating methods, even with all argument lists applied. Eg, Foo.bar(&self)(). These actually do work today, but they won't with the new curry thunk implementation. Hopefully this won't cause any fallout, if it does, I'll hack something up.

I wasn't relying on this but I was testing it. (I also don't I mind the ban). That being said, where can I learn more about the new curry thunk implementation? :-)

davezarzycki · 2020-02-28T17:52:58Z

lib/Sema/ConstraintSystem.cpp


  // If base is a metatype it would be ignored (unless this is an initializer
  // call), but if it is some other type it means that we have a single
  // application level already.
-  unsigned level = baseTy->is<MetatypeType>() ? 0 : 1;
-  if (auto *call = dyn_cast_or_null<CallExpr>(cs.getParentExpr(anchor))) {
-    level += dyn_cast_or_null<CallExpr>(cs.getParentExpr(call)) ? 2 : 1;


Hi @slavapestov – I'm trying to understand why this line was removed. Why do we no longer need to check for the grandparent CallExpr?

xedin · 2020-02-28T18:43:51Z

@davezarzycki I think #28698 is PR you are looking for :)

After swiftlang#30097, some valid full applications are diagnosed as partial application failures. This restores the old behavior while preserving the `super` related fixes in swiftlang#30097.

slavapestov commented Feb 27, 2020

View reviewed changes

xedin reviewed Feb 27, 2020

View reviewed changes

slavapestov force-pushed the sr-75-super-method branch from 2bf9bc6 to 019452f Compare February 27, 2020 22:28

xedin approved these changes Feb 27, 2020

View reviewed changes

slavapestov merged commit ea9806d into swiftlang:master Feb 28, 2020

davezarzycki reviewed Feb 28, 2020

View reviewed changes

davezarzycki mentioned this pull request Feb 28, 2020

[Sema] Fix erroneous partial application errors #30127

Closed

Sema: Diagnose unbound method references on 'super.' #30097

Sema: Diagnose unbound method references on 'super.' #30097

Uh oh!

Conversation

slavapestov commented Feb 27, 2020

Uh oh!

slavapestov commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Feb 27, 2020

Uh oh!

slavapestov commented Feb 27, 2020

Uh oh!

slavapestov commented Feb 27, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

davezarzycki commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davezarzycki Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Feb 28, 2020

Uh oh!

Uh oh!

davezarzycki commented Feb 28, 2020 •

edited

Loading

davezarzycki Feb 28, 2020 •

edited

Loading