SR-2484: Improve diagnostic for incorrectly called private init #5237

xedin · 2016-10-11T21:39:54Z

Fixes situation where lookup of the constructors is attempted on non MetatypeType base.

Minor refactoring is done around how discovered structural members are treated,
which reduced amount of condition logic.

Resolves SR-2484.

xedin · 2016-10-11T21:40:01Z

/cc @rudkx

rudkx · 2016-10-11T22:40:35Z

@jrose-apple /cc Jordan in case he wants to take a look.

jrose-apple · 2016-10-12T15:43:07Z

lib/Sema/CSDiag.cpp

+                      decl->getName(), decl->getFormalAccess());
+    }
+
+    return true;


This doesn't look right. The return is inside the loop, which means you only diagnose at most one candidate. What if there are two inaccessible candidates?

I just followed the common theme which can be found throughout diagnostics code - once a single problem is diagnosed is returns right away. But i can change this code in a way it where would try to diagnose as many problems with access as possible and return only then. WDYT?

I think the problem is "there are no available initializers" and the diagnosis would then include all of the possible candidates. That more closely matches what CalleeCandidateInfo::diagnoseSimpleErrors does, although I'll admit the current code is not set up as well to do that.

It looks like diagnoseSimpleErrors includes candidates which aren't the intended callees (e.g. if you call c.foo(a:) and there is a c.foo() and a c.foo(a:) and they are both private, it lists both of those). If only c.foo() is accessible and you call c.foo(a:) we say you're calling a function that takes no arguments.

I think we could do better for both of these, but this seems like a step in the right direction.

@jrose-apple Do you think this still needs additional work before being committed?

:-/ I really don't like it. It seems like the kind of thing where a user would say "but I wasn't trying to call that initializer anyway; I was trying to call [some other one that would have been a better match]."

Just to clarify, do you think it's a good idea to include all of the possible inaccessible candidates? Or you don't think this is correct?

Ah, just including all possible inaccessible candidates.

All right, I'll do that then :)

jrose-apple · 2016-10-12T15:44:18Z

lib/Sema/CSDiag.cpp

+      !baseObjTy->is<MetatypeType>()) {
+    if (auto ctorRef = dyn_cast<UnresolvedDotExpr>(expr)) {
+      if (isa<SuperRefExpr>(ctorRef->getBase())) {
+        diagnose(anchor->getLoc(), diag::super_initializer_not_in_initializer);


Please add a test for this diagnostic.

This is pre-existing diagnostic which just got moved, there is a test for it already in
test/Constraints/super_constructor.swift and test/Parse/super.swift

Ah, got it.

jrose-apple · 2016-10-12T15:46:32Z

lib/Sema/CSDiag.cpp

+
+      // Suggest inserting a call to 'type(of:)' to construct another object
+      // of the same dynamic type.
+      SourceRange fixItRng = ctorRef->getNameLoc().getSourceRange();


I'm not convinced this is a good idea. Most class initializers can't be called on a dynamic type because they're not required, and most protocols don't have initializer requirements at all. Additionally, you've changed the lookup to search on the type rather than the instance all the time, so if we get here there might really be no viable initializers. Or am I missing something?

Same as above, I've just move it, this was pre-existing.

rudkx

LGTM.

rudkx · 2016-10-12T20:25:29Z

lib/Sema/CSDiag.cpp

+                      decl->getName(), decl->getFormalAccess());
+    }
+
+    return true;


It looks like diagnoseSimpleErrors includes candidates which aren't the intended callees (e.g. if you call c.foo(a:) and there is a c.foo() and a c.foo(a:) and they are both private, it lists both of those). If only c.foo() is accessible and you call c.foo(a:) we say you're calling a function that takes no arguments.

I think we could do better for both of these, but this seems like a step in the right direction.

rudkx · 2016-10-12T20:27:05Z

@swift-ci Smoke test.

xedin · 2016-10-13T23:04:02Z

@jrose-apple I've made a change to diagnose all of the inaccessible candidates found. Please take a look and let me know!

jrose-apple · 2016-10-14T16:21:20Z

Ah, this isn't quite what I meant; this will produce N diagnostics at the same site with the same message (if there are multiple overloads with the same argument labels). I think something like what diagnoseSimpleErrors is doing is better: one error message, with notes for all the candidates (even if we don't display those notes very well in Xcode right now). That does have a downside of picking only one name for the primary message if there are multiple names, though.

Thoughts?

xedin · 2016-10-14T17:37:58Z

@jrose-apple ah, I've totally misunderstood what you meant, I'll do that momentarily!

xedin · 2016-10-14T19:16:27Z

@jrose-apple Done, it's going to emit 'declared at' messages now for all of the candidates found if there was one with incorrect access. After looking at diagnoseSimpleErrors this makes a lot more sense now! :)

jrose-apple · 2016-10-14T23:26:09Z

lib/Sema/CSDiag.cpp

+
+    for (auto &candidate : result.ViableCandidates) {
+      auto decl = candidate.getDecl();
+      CS->TC.diagnose(decl, diag::decl_declared_here, decl->getName());


It seems a little funny here that we were checking for non-decls before and now we're going to skip that check.

Also, nitpick: please use getFullName for these notes, which will help distinguish overloads. (Plain getName is good for the main error because that will drop the argument labels, which is important if they're different.)

Ah, it looks like it's a bug in the code in diagnoseSimpleErrors as well, I'll change here and in diagnoseSimpleErrors as well.

Aha, I was wondering if diagnoseSimpleErrors was in a context where it already knew everything was a decl, but that's probably silly.

yeah, it was only checking that for candidates[0], I'm running tests right now locally, will push in a few.

xedin · 2016-10-15T00:38:23Z

@jrose-apple Done, changed both new code and diagnoseSimpleErrors.

…ate init

jrose-apple · 2016-10-18T00:09:16Z

Thanks, @xedin!

@swift-ci Please test

swift-ci · 2016-10-18T00:09:36Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - f0b621a8cbd8d336cfb7b9e24dbc210d63a9ded5
Test requested by - @jrose-apple

jrose-apple · 2016-10-18T00:09:42Z

Once more for the force push.

@swift-ci Please test

xedin · 2016-10-18T00:25:27Z

@jrose-apple Thanks for looking at this!

xedin changed the title ~~[Diagnostics] SR-2484: Improve diagnostic for incorrectly called private init~~ SR-2484: Improve diagnostic for incorrectly called private init Oct 11, 2016

rudkx self-assigned this Oct 11, 2016

jrose-apple suggested changes Oct 12, 2016

View reviewed changes

rudkx approved these changes Oct 12, 2016

View reviewed changes

xedin force-pushed the SR-2484 branch from f4412fc to f0b621a Compare October 13, 2016 23:02

xedin force-pushed the SR-2484 branch from f0b621a to 4ae1944 Compare October 14, 2016 19:12

jrose-apple reviewed Oct 14, 2016

View reviewed changes

xedin force-pushed the SR-2484 branch from 4ae1944 to ba2fe1d Compare October 15, 2016 00:37

[Diagnostics] SR-2484: Improve diagnostic for incorrectly called priv…

352f4b7

…ate init

xedin force-pushed the SR-2484 branch from ba2fe1d to 352f4b7 Compare October 15, 2016 09:35

jrose-apple approved these changes Oct 18, 2016

View reviewed changes

rudkx merged commit f72576d into swiftlang:master Oct 18, 2016

jtbandes mentioned this pull request Nov 19, 2016

[QoI] Improve diagnostics when accessing .init on a non-metatype #5869

Merged

rudkx mentioned this pull request Oct 14, 2016

[SR-2484] Bad diagnostic for incorrectly calling private init #45089

Closed

SR-2484: Improve diagnostic for incorrectly called private init #5237

SR-2484: Improve diagnostic for incorrectly called private init #5237

Uh oh!

Conversation

xedin commented Oct 11, 2016

Uh oh!

xedin commented Oct 11, 2016

Uh oh!

rudkx commented Oct 11, 2016

Uh oh!

jrose-apple Oct 12, 2016 • 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 Oct 12, 2016 • 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!

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!

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!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudkx commented Oct 12, 2016

Uh oh!

xedin commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Oct 14, 2016

Uh oh!

xedin commented Oct 14, 2016

Uh oh!

xedin commented Oct 14, 2016

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!

xedin commented Oct 15, 2016

Uh oh!

jrose-apple commented Oct 18, 2016

Uh oh!

swift-ci commented Oct 18, 2016

Uh oh!

jrose-apple commented Oct 18, 2016

Uh oh!

xedin commented Oct 18, 2016

Uh oh!

Uh oh!

jrose-apple Oct 12, 2016 •

edited

Loading

xedin Oct 12, 2016 •

edited

Loading

xedin commented Oct 13, 2016 •

edited

Loading