SR-13976: Improve compiler error message: "partial application of ‘mutating’ method is not allowed" #35466

abdulajet · 2021-01-17T19:35:14Z

As mentioned on the Jira ticket, partial application is not a well-known term so I have reworded the error messages with a suggested phrase to help people diagnose the issue easier.

Based on the diagnostics definition file there were 4 different combinations that I saw that used the error, below is the wording changes i made:

partial application of 'mutating' method is not allowed -> calling a 'mutating' method without an argument list is not allowed
partial application of 'super.init' initializer chain is not allowed -> calling a 'super.init' initializer chain without an argument list is not allowed
partial application of 'self.init' initializer delegation is not allowed -> calling a 'self.init' initializer delegation without an argument list is not allowed
partial application of 'super' instance method with metatype base is not allowed -> calling a 'super' instance method with metatype base without an argument list is not allowed

The first commit has the change, second commit has the test updates.

Resolves SR-13976. cc @varungandhi-apple

test/Constraints/members.swift

varungandhi-apple · 2021-01-19T18:09:54Z

Thanks for the PR. 🙂

Minor comment: Generally we try to implement the test and change in the same commit (or alternately, add a failing test marked as XFAIL in the first commit, and implement the fix and remove the XFAIL in the second commit). That way, bisection becomes easier. For this particular case, it's not a big deal though, as we can squash the changes at the end.

abdulajet · 2021-01-19T18:15:10Z

I can squash the commits when I change from calling -> using

abdulajet · 2021-01-29T18:51:56Z

This is what I have come up with! What do you folks think before I go further? @varungandhi-apple @LucianoPAlmeida @xedin

 if (auto FD = dyn_cast<FuncDecl>(getDC())) {
    if (FD->getParameters()->size() == 0) {
      emitDiagnosticAt(getLoc(), diag::add_empty_parameter_list_fixit,
                       anchor->getName().getBaseIdentifier())
          .fixItInsertAfter(getLoc(), "()");
      }
  }

Here is some of the test output from Constraints/mutating_members.swift

/Users/aajetunmobi/Documents/swiftproject/swift/test/Constraints/mutating_members.swift:9:37: error: incorrect message found
let x = Foo.boom // expected-error{{partial application of 'mutating' method}}
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                   cannot reference 'mutating' method as a function value
/Users/aajetunmobi/Documents/swiftproject/swift/test/Constraints/mutating_members.swift:11:36: error: incorrect message found
let z0 = y.boom // expected-error{{partial application of 'mutating' method}}
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                  cannot reference 'mutating' method as a function value
/Users/aajetunmobi/Documents/swiftproject/swift/test/Constraints/mutating_members.swift:12:42: error: incorrect message found
let z1 = Foo.boom(&y) // expected-error{{partial application of 'mutating' method}}
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                        cannot reference 'mutating' method as a function value
/Users/aajetunmobi/Documents/swiftproject/swift/test/Constraints/mutating_members.swift:15:10: error: unexpected note produced: did you mean to call 'boom' with '()'?
 return Foo.boom // expected-error{{partial application of 'mutating' method}}

xedin · 2021-01-29T19:57:22Z

I like it! I'd also suggest to add a note - did you mean to call it with '()' and attach a fix-it to that instead of fix-it alone, it make it more clearer.

xedin · 2021-01-29T19:59:06Z

And one more thing - a better check would be to account for defaultable parameters as well, so if there are 0 parameters or all of the parameters are defaultable - fix-it.

abdulajet · 2021-02-01T17:59:05Z

I like it! I'd also suggest to add a note - did you mean to call it with '()' and attach a fix-it to that instead of fix-it alone, it make it more clearer.

I thought that was the purpose of the add_empty_parameter_list_fixit diagnostic? The last error has an additonal error did you mean to call 'boom' with '()'? @xedin

xedin · 2021-02-01T18:01:56Z

Ah, right, sorry I missed that one, then the only viable thing is related to default parameters and we should be good to go!

abdulajet · 2021-02-01T18:57:21Z

Ah, right, sorry I missed that one, then the only viable thing is related to default parameters and we should be good to go!

I cannot figure out how to check for that. Is there a part of the docs I should be looking at specifically?

xedin · 2021-02-01T19:09:45Z

If I remember correctly parameter list consists of ParamDecl * and there is a way to check whether it has a default or not, so you'd just need add a check to make sure that if there are parameters they are all defaulted.

LucianoPAlmeida · 2021-02-01T20:50:33Z

I cannot figure out how to check for that. Is there a part of the docs I should be looking at specifically?

An example you can look at :) https://github.com/apple/swift/blob/19a4df331c3de41dac1b2d069cbda863ed0f24e5/lib/Sema/CSSimplify.cpp#L3437-L3446

abdulajet · 2021-02-05T20:17:51Z

Sorry for the delay folks. The example posted earlier has some functions that aren't available in the file, as to not add more imports I went down the ParamDecl route.

  if (auto FD = dyn_cast<FuncDecl>(getDC())) {
    auto params = FD->getParameters();

    bool hasNonDefaultParam =
        std::any_of(params->begin(), params->end(),
                    [](ParamDecl *param) { return !param->isDefaultArgument(); });

    if (params->size() == 0 && !hasNonDefaultParam) {
      emitDiagnosticAt(getLoc(), diag::add_empty_parameter_list_fixit,
                       anchor->getName().getBaseIdentifier())
          .fixItInsertAfter(getLoc(), "()");
    }
  }

Btw is it ok to force push over this branch?

xedin · 2021-02-05T20:21:51Z

Yeah, it's fine to force push.

xedin · 2021-02-05T20:23:10Z

@abdulajet There is an llvm equivalent of std::any_of - it works with ArrayRefs so you don't have to use begin/end e.g. llvm::any_of(FD->getParameters(), [](const ParamDecl *param) { ... })

xedin · 2021-02-05T20:24:20Z

I would suggest using llvm::none_of here though to avoid negative checks.

varungandhi-apple · 2021-02-05T20:25:38Z

Also, please don't apologize about delays or similar. 😄 It's totally okay. You're doing this in your free time and we are thankful for your contributions, at whatever pace works for you.

abdulajet · 2021-02-05T23:08:11Z

Thanks for all the help!

 if (auto FD = dyn_cast<FuncDecl>(getDC())) {
    auto params = FD->getParameters();

    bool hasNoNonDefaultParams = llvm::none_of(
        *params, [](ParamDecl *param) { return param->isDefaultArgument(); });

    if (params->size() == 0 && hasNoNonDefaultParams) {
      emitDiagnosticAt(getLoc(), diag::add_empty_parameter_list_fixit,
                       anchor->getName().getBaseIdentifier())
          .fixItInsertAfter(getLoc(), "()");
    }
  }

Ill get the tests done then push this up.

xedin · 2021-02-05T23:17:13Z

Sounds great, thank you!

xedin · 2021-02-05T23:17:42Z

if (params->size() == 0 && hasNoNonDefaultParams) {

I think you mean ||

abdulajet · 2021-02-05T23:22:18Z

aha yes i 100% do. cheers

LucianoPAlmeida · 2021-02-05T23:38:35Z

Sorry if this is being nitpicking but I think that llvm::all_of would be more readable here
Because (params->size() == 0 || hasNoNonDefaultParams) reads has no params or has no non default params
But (params->size() == 0 || allParamsHaveDefaultArg) reads has no params or all parameters have a default argument which reads as exactly what is the intention here without double no. WDYT @xedin?
Both are fine but the first one took me more time to understand ...

xedin · 2021-02-06T00:07:33Z

Works either way, I think it's a matter of personal preference.

abdulajet · 2021-02-12T20:24:34Z

I have an issue where the conditions for the fixit are being applied where they shouldn't:

super.baseFunc0() in NameLookup/name_lookup.swift is passing the condition of no params. So i think we also need to check if the function isnt being called already:

if ((params->size() == 0 && allParamsHaveDefaultArgs) && notBeingCalled) {

}

Is there someone I can see an example of a check for if the function is being called? Not sure i can see anything on the public interface of FuncDecl or UnresolvedDotExpr

For @xedin you mentioned

Please add a test case or two for the fix-it as well

in the initial discussion. Should this be in its own test file?

Thanks all!

xedin · 2021-02-16T19:29:39Z

So i think we also need to check if the function isnt being called already:

Shouldn't the logic which detects that it's a partial application already handle that?

Should this be in its own test file?

I think you can add new tests to test/Constraints/members.swift.

abdulajet · 2021-02-16T19:54:45Z

Shouldn't the logic which detects that it's a partial application already handle that?

Oh yeah, of course! In that case, I am not sure why the diag is being called for this test

Here is an example of the failing test from NameLookup/name_lookup.swift

super.baseFunc0() // expected-error {{instance member 'baseFunc0' cannot be used on type 'ThisBase1'}}
// expected-error@-1 {{cannot reference 'super' instance method with metatype base as a function value}}

xedin · 2021-02-16T20:10:28Z

@abdulajet Since baseFunc0 is an instance method its signature is (Self) -> () -> Void, I think the diagnostic logic is wrong and it shouldn't even talk about partial application in this case since it's used from a class func context so super there means a different thing.

abdulajet · 2021-02-17T11:41:58Z

Cool, ill look into that. Thanks

abdulajet · 2021-02-19T19:11:10Z

@xedin I have had a look around, the path I was going down was trying to see if the function was marked with a class descriptor and returning false out of the diagnostic. Would this a good solution (if it is, can I have a hand in checking the function descriptors. getDescriptiveKind on Decl didn't seem to work) or would it be better to go further upstream and stop the PartialApplicationFailure::diagnoseAsError() function from being called for a class func entirely.

Thanks again

xedin · 2021-02-23T20:30:09Z

@abdulajet The way diagnostics are currently implemented in the solver is two-fold - there is a "ConstraintFix" and that has a "FailureDiagnostic" attached to that. ConstraintFix subtypes are responsible for failure identification and information storage, FailureDiagnostic subtypes are responsible for diagnostic emission based on collected information.

What you want to do here is to prevent a fix (AllowInvalidPartialApplication in this case) from being recorded in situations like what you have described. There is a method which is responsible for verifying references - isInvalidPartialApplication (it's called by ConstraintSystem::resolveOverload), you'd need to adjust isInvalidPartialApplication to consider declaration context associated with given constraint system (accessible via DC member of ConstraintSystem) before making decisions about invalid partial applications.

abdulajet · 2021-02-23T21:40:50Z

Amazing! Will take a look, thanks.

LucianoPAlmeida · 2021-09-14T00:08:09Z

Should we scope this down to message adjustments? I think changing it to say referencing ... as a function value should be landable by itself and all of the additional changes could be done in a separate PR.

Given that the SR is only about the terminology, I think this would be fine :)
Perhaps we could create another SR for the additional issues? It would also be easier to describe the additional issues specifically there to narrow that from this thread since it got a lot of things(e.g. implementation details, crash logs and so on...) and is a bit hard to narrow it down.
Maybe it would also be good to add a FIXME(diagnostics) comment around the problematic case as well(as far as I remenber the additional issue is about a specific edge case)? What you think @xedin?

xedin · 2021-09-14T00:22:15Z

I think using FIXME(diagnostics) would be fine as soon as we have all of the issues filed as SRs so it's easy to keep track of them and tackle one by one.

abdulajet · 2021-09-14T11:22:14Z

ok to revert back to just changing the text of the diagnostic then 2 new SRs. One for the diagnostic being called incorrectly and a dependent one that adds the fix it?

LucianoPAlmeida · 2021-09-14T14:35:24Z

ok to revert back to just changing the text of the diagnostic then 2 new SRs. One for the diagnostic being called incorrectly and a dependent one that adds the fix it?

Sounds good to me =]

abdulajet · 2021-09-27T17:49:21Z

SRs:
https://bugs.swift.org/browse/SR-15250
https://bugs.swift.org/browse/SR-15251

LucianoPAlmeida · 2021-09-28T10:32:48Z

SRs:
https://bugs.swift.org/browse/SR-15250
https://bugs.swift.org/browse/SR-15251

Thank you!
For this one then we only need to rebase with man, make sure all tests pass, add the proper TODO(diagnostics) referencing the SRs and this would be good to go I think

abdulajet · 2021-09-28T13:38:25Z

Sure thing! Will do it this weekend

abdulajet · 2021-09-28T14:21:33Z

Did we want to leave it as "calling" or change to "referencing"

LucianoPAlmeida · 2021-09-28T23:21:35Z

Did we want to leave it as "calling" or change to "referencing"

From what I see here #35466 (comment) we agreed on cannot reference 'mutating' method "foo" as a function value

xedin · 2021-10-01T05:34:13Z

Agree with @LucianoPAlmeida here, "referencing" seems more appropriate here.

abdulajet · 2021-10-03T09:55:53Z

@LucianoPAlmeida updated to use "reference" and added some comments to name_lookup.swift for the linked SRs. Let me know if they are better placed elsewhere

test/NameLookup/name_lookup.swift

LucianoPAlmeida · 2021-10-03T15:26:14Z

@swift-ci Please smoke test

LucianoPAlmeida

Looks good to go to me!

LucianoPAlmeida · 2021-10-04T12:39:50Z

@abdulajet there is one missing test to adjust /test/type/self.swift:358

xedin · 2021-10-04T16:41:45Z

include/swift/AST/DiagnosticsSema.def

        "'mutating' method|"
        "'super.init' initializer chain|"
        "'self.init' initializer delegation|"
        "'super' instance method with metatype base"
-      "}0 is not allowed",
+      "}0 without an argument list",


I thought that we agreed to use as function value instead of without an argument list? Also nit on the grammar - diagnostics shouldn't include articles "a" or "an" in the text because they make a diagnostic longer without adding any information (see https://github.com/apple/swift/blob/main/docs/Diagnostics.md).

Ah yeah, that's true, I just miss that 😅

my bad. referencing x as a function value coming up

LucianoPAlmeida · 2021-10-06T11:16:44Z

@swift-ci Please smoke test

abdulajet · 2021-10-06T13:36:33Z

include/swift/AST/DiagnosticsSema.def

        "'mutating' method|"
        "'super.init' initializer chain|"
        "'self.init' initializer delegation|"
        "'super' instance method with metatype base"
-      "}0 is not allowed; calling the function has undefined behavior and will "
+      "}0 as function value; calling the function has undefined behavior and will "


I cannot build swift locally at the moment, while I fix that so I can run the test suite locally. Does the second clause here make sense? "cannot reference x as function value calling the function has undefined..."

I think so since is error/warning will be in the context of a function I believe

Yes, I think it makes sense, second part is a clarification on why it can't be used as a function value - because calling it is UB.

…tating’ method is not allowed”

abdulajet · 2021-10-06T19:06:04Z

test/Constraints/mutating_members_compat.swift

@@ -6,31 +6,31 @@ struct Foo {
  mutating func boom() {}
 }

-let x = Foo.boom // expected-warning{{partial application of 'mutating' method}}
+let x = Foo.boom // expected-warning{{cannot reference 'mutating' method as function value; calling the function has undefined behavior and will be an error in future Swift versions}}


I am not sure why this test was working on main before? the warnings for swift 4 should have always had the second clause.

But alas, run the tests locally and fixed the CI issues

xedin · 2021-10-06T19:10:29Z

@swift-ci please test

xedin · 2021-10-07T19:40:31Z

Looks like it's time to land this! Thank you, @abdulajet and @LucianoPAlmeida!

abdulajet · 2021-10-07T20:53:02Z

thanks all for the help!

LucianoPAlmeida reviewed Jan 17, 2021

View reviewed changes

test/Constraints/members.swift Outdated Show resolved Hide resolved

abdulajet force-pushed the sr-13976 branch from edff3dc to d326399 Compare October 3, 2021 09:51

LucianoPAlmeida reviewed Oct 3, 2021

View reviewed changes

test/NameLookup/name_lookup.swift Outdated Show resolved Hide resolved

abdulajet force-pushed the sr-13976 branch from d326399 to 3fefd3f Compare October 3, 2021 15:17

LucianoPAlmeida approved these changes Oct 3, 2021

View reviewed changes

LucianoPAlmeida requested a review from xedin October 3, 2021 15:52

xedin reviewed Oct 4, 2021

View reviewed changes

abdulajet force-pushed the sr-13976 branch from 3fefd3f to b54106e Compare October 6, 2021 08:09

abdulajet commented Oct 6, 2021

View reviewed changes

SR-13976: Improve compiler error message: "partial application of ‘mu…

f8c737b

…tating’ method is not allowed”

abdulajet force-pushed the sr-13976 branch from b54106e to f8c737b Compare October 6, 2021 19:04

abdulajet commented Oct 6, 2021

View reviewed changes

xedin merged commit 6b2c016 into swiftlang:main Oct 7, 2021

typesanitizer mentioned this pull request Sep 13, 2021

[SR-13976] Improve compiler error message: "partial application of ‘mutating’ method is not allowed" #56371

Closed

SR-13976: Improve compiler error message: "partial application of ‘mutating’ method is not allowed" #35466

SR-13976: Improve compiler error message: "partial application of ‘mutating’ method is not allowed" #35466

Conversation

abdulajet commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

varungandhi-apple commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abdulajet commented Jan 19, 2021

Uh oh!

abdulajet commented Jan 29, 2021

Uh oh!

xedin commented Jan 29, 2021

Uh oh!

xedin commented Jan 29, 2021

Uh oh!

abdulajet commented Feb 1, 2021

Uh oh!

xedin commented Feb 1, 2021

Uh oh!

abdulajet commented Feb 1, 2021

Uh oh!

xedin commented Feb 1, 2021

Uh oh!

LucianoPAlmeida commented Feb 1, 2021

Uh oh!

abdulajet commented Feb 5, 2021

Uh oh!

xedin commented Feb 5, 2021

Uh oh!

xedin commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 5, 2021

Uh oh!

varungandhi-apple commented Feb 5, 2021

Uh oh!

abdulajet commented Feb 5, 2021

Uh oh!

xedin commented Feb 5, 2021

Uh oh!

xedin commented Feb 5, 2021

Uh oh!

abdulajet commented Feb 5, 2021

Uh oh!

LucianoPAlmeida commented Feb 5, 2021

Uh oh!

xedin commented Feb 6, 2021

Uh oh!

abdulajet commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 16, 2021

Uh oh!

abdulajet commented Feb 16, 2021

Uh oh!

xedin commented Feb 16, 2021

Uh oh!

abdulajet commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abdulajet commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 23, 2021

Uh oh!

abdulajet commented Feb 23, 2021

Uh oh!

LucianoPAlmeida commented Sep 14, 2021

Uh oh!

xedin commented Sep 14, 2021

Uh oh!

abdulajet commented Sep 14, 2021

Uh oh!

LucianoPAlmeida commented Sep 14, 2021

Uh oh!

abdulajet commented Sep 27, 2021

Uh oh!

abdulajet commented Jan 17, 2021 •

edited

Loading

varungandhi-apple commented Jan 19, 2021 •

edited

Loading

xedin commented Feb 5, 2021 •

edited

Loading

abdulajet commented Feb 12, 2021 •

edited

Loading

abdulajet commented Feb 17, 2021 •

edited

Loading

abdulajet commented Feb 19, 2021 •

edited

Loading

abdulajet Oct 6, 2021 •

edited

Loading