Provide a fix-it that inserts omitted generic parameters when they can't be inferred #4682

jrose-apple · 2016-09-09T03:14:14Z

This will help when migrating to Swift 3 and Objective-C classes with newly-added generic parameters.

Still to do:

More tests
See if there's a way to handle conflicting context

rdar://problem/27087345

jrose-apple · 2016-09-09T03:21:44Z

@nkcsgexi, @akyrtzi, what do you think about what's there so far? In particular, what do you think about the defaults?

No constraints, class X<T>: X<Any>(). Should this be X<<#T: Any##Any#>> instead?
Superclass constraint only, class X<T: UIView>: X<UIView>().
Class-bound only, class X<T: class>: X<AnyObject>(). Should this be X<<#T: class##AnyObject#>> instead?
Single ObjC protocol, class X<T: NSCoding>: X<NSCoding>().
Multiple protocols, or class+protocols: class X<T: UIViewController, UITableViewDataSource>: X<<#T: UIViewController & UITableViewDataSource#>>

Stepping back, is it okay to use placeholders at all here? (@DougGregor?) I see we have one other diagnostic with placeholders. Maybe we can change the text output format to special-case them (later).

jrose-apple · 2016-09-09T03:21:55Z

@swift-ci Please smoke test

slavapestov · 2016-09-09T06:03:56Z

lib/Sema/CSDiag.cpp

+  if (FoundGenericTypeBase && !isa<GenericIdentTypeRepr>(FoundGenericTypeBase)){
+    assert(FoundDecl);
+
+    SmallString<64> genericParamBuf;


Is this in any way similar to diagnoseUnboundGenericType() in TypeCheckType.cpp?

Oh, good catch. I'll see if I can factor out common logic here. (I like my implementation better.)

nkcsgexi · 2016-09-09T22:31:06Z

I like the idea of using placeholders in fixits. We have a precedent of that in the fixits for auto-filling protocol conformances skeletons.

DougGregor · 2016-09-12T23:17:11Z

I like the idea of using placeholders. It works very well in Xcode, and is fairly easy to handle in other tools that might want to present Fix-Its to the user.

jrose-apple · 2016-09-13T00:22:08Z

Implementation mostly done, but I still need to write tests.

@swift-ci Please test

jrose-apple · 2016-09-13T00:46:47Z

Apple, Linux

jrose-apple · 2016-09-20T23:21:48Z

@nkcsgexi, would you mind reviewing this formally for the Swift 3.0 branch? And would you like me to make sure these fix-its are all enabled in migration mode?

@swift-ci Please test

jrose-apple · 2016-09-20T23:22:30Z

Whoops, a test change snuck in there. Gotta fix that.

swift-ci · 2016-09-20T23:22:58Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 85b793e37a974dcbba3f9d78dcaf232c4eef59cc
Test requested by - @jrose-apple

swift-ci · 2016-09-20T23:23:09Z

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

jrose-apple · 2016-09-20T23:24:09Z

And it looks like master's messed up at the moment. I'll wait.

nkcsgexi · 2016-09-20T23:24:54Z

Will review. This is a fixit associated with error, right? Isn't that automatically picked up by migrator?

jrose-apple · 2016-09-20T23:26:02Z

I guess also:

@slavapestov, is this an okay use of GenericTypeParamDecl, or should I be using the generic signature and environment?
@DougGregor or @rudkx, is this a reasonable way to fish for resolved type parameters?

jrose-apple · 2016-09-20T23:28:53Z

@nkcsgexi The new fix-it is on a note because usually you want a more specific type than the default…but in migration, you might not have a type because in Swift 2.3 it wasn't generic.

jrose-apple · 2016-09-21T02:56:10Z

@swift-ci Please test

jrose-apple · 2016-09-21T02:56:24Z

@swift-ci Please test

swift-ci · 2016-09-21T03:14:57Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 634055e298806183286699b229646be08c200860
Test requested by - @jrose-apple

jrose-apple · 2016-09-21T15:50:47Z

@swift-ci Please test Linux platform

jrose-apple · 2016-09-21T15:51:01Z

macOS full tests passed earlier.

@swift-ci Please smoke test macOS platform

swift-ci · 2016-09-21T16:19:28Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 634055e298806183286699b229646be08c200860
Test requested by - @jrose-apple

jrose-apple · 2016-09-21T16:20:34Z

^ @aciidb0mb3r, have you seen this SwiftPM failure before? (unrelated to this patch)

aciidgh · 2016-09-21T16:24:41Z

@jrose-apple unfortunately yes, I disabled a similar test failure here
swiftlang/swift-package-manager@57c5be1

will disable all related tests.

aciidgh · 2016-09-21T16:29:04Z

Disabled here: swiftlang/swift-package-manager@76714d2

jrose-apple · 2016-09-21T16:58:54Z

Thanks, Ankit.

@swift-ci Please test Linux platform

nkcsgexi · 2016-09-21T18:21:36Z

lib/Sema/CSDiag.cpp

+        return Type();
+
+      TypeVariableType *unused;
+      Type maybeFixedType = cs.getFixedTypeRecursive(preferred, unused,


I'm a little confused here. If we can get generic argument type, why do we invoke CS to fix it? to remove unsubstituted type variables?

This is for cases where we have some but not all of the generic arguments. Suggesting <Int, Any> is usually better than <Any, Any>.

oh, I see. Mind adding some inline comments?

nkcsgexi · 2016-09-21T18:34:06Z

lib/Sema/MiscDiagnostics.cpp

+
+    if (protocols.empty()) {
+      genericParamText << "Any";
+      return;


For future proof, can we retrieve "Any" from ASTContext.getAnyDecl()->getName().

Otherwise, LGTM!

There's no "Any" decl anymore, but I'll switch it to Context.ID_Any so it's at least more searchable.

jrose-apple · 2016-09-21T21:52:30Z

Updated for Xi's review comments.

@swift-ci Please smoke test

nkcsgexi · 2016-09-21T21:53:52Z

Thank you!

@objc

For example, if someone tries to use the newly-generic type Cache, from Foundation: var cache = Cache() they'll now get a fix-it to substitute the default generic parameters: var cache = Cache<AnyObject, AnyObject>() The rules for choosing this placeholder type are based on constraints and won't be right 100% of the time, but they should be reasonable. (In particular, constraints on associated types are ignored.) In cases where there's no one concrete type that will work, an Xcode- style placeholder is inserted instead. - An unconstrained generic parameter defaults to 'Any'. - A superclass-constrained parameter defaults to that class, e.g. 'UIView'. - A parameter constrained to a single @objc protocol (or to AnyObject) defaults to that protocol, e.g. 'NSCoding'. - Anything else gets a placeholder using the generic parameter's name and protocol composition syntax. rdar://problem/27087345

Slava pointed out that we have an existing version of the code from the previous commit that's only used when checking types. Replace it with the new code, which handles more cases.

Last bit of rdar://problem/27087345.

jrose-apple · 2016-09-22T00:04:43Z

@swift-ci Please smoke test

jrose-apple · 2016-09-22T00:04:51Z

@swift-ci Please smoke test

jrose-apple · 2016-09-22T00:19:28Z

Connectivity issue.

@swift-ci Please smoke test OS X platform

jrose-apple · 2016-09-22T00:43:06Z

@swift-ci Please smoke test OS X platform

shahmishal · 2016-09-22T00:44:23Z

@swift-ci Please smoke test OS X platform

Provide a fix-it that inserts omitted generic parameters when they can't be inferred. rdar://problem/27087345

jrose-apple · 2016-09-22T03:01:00Z

Still waiting for feedback from @DougGregor or @rudkx on accessing the constraint system, and @slavapestov on the use of GenericTypeParamDecl.

rudkx · 2016-09-22T04:03:23Z

@jrose-apple I suspect this is safe, but I could not guarantee it.

slavapestov reviewed Sep 9, 2016
View reviewed changes

jrose-apple force-pushed the generic-param-fix-it branch from 2c749af to 63fcb91 Compare September 13, 2016 00:21

jrose-apple force-pushed the generic-param-fix-it branch from 63fcb91 to 85b793e Compare September 20, 2016 23:20

jrose-apple force-pushed the generic-param-fix-it branch from 85b793e to 634055e Compare September 20, 2016 23:23

jrose-apple changed the title ~~[WIP] Provide a fix-it that inserts omitted generic parameters when they can't be inferred~~ Provide a fix-it that inserts omitted generic parameters when they can't be inferred Sep 20, 2016

jrose-apple force-pushed the generic-param-fix-it branch from 634055e to 66fd330 Compare September 21, 2016 15:50

nkcsgexi reviewed Sep 21, 2016

View reviewed changes

jrose-apple force-pushed the generic-param-fix-it branch from 66fd330 to ebb639d Compare September 21, 2016 21:52

jrose-apple added 3 commits September 21, 2016 18:04

Better generic fix-its for types as well as construction and casts.

1d7ad9e

Slava pointed out that we have an existing version of the code from the previous commit that's only used when checking types. Replace it with the new code, which handles more cases.

Auto-apply the new generic parameter fix-it during migration.

fea7878

Last bit of rdar://problem/27087345.

jrose-apple force-pushed the generic-param-fix-it branch from ebb639d to fea7878 Compare September 22, 2016 00:04

jrose-apple merged commit 00d0b55 into swiftlang:master Sep 22, 2016

jrose-apple deleted the generic-param-fix-it branch September 22, 2016 01:28

jrose-apple added a commit to jrose-apple/swift that referenced this pull request Sep 22, 2016

Merge pull request swiftlang#4682 from jrose-apple/generic-param-fix-it

d8a3757

Provide a fix-it that inserts omitted generic parameters when they can't be inferred. rdar://problem/27087345

Provide a fix-it that inserts omitted generic parameters when they can't be inferred #4682

Provide a fix-it that inserts omitted generic parameters when they can't be inferred #4682

Uh oh!

Conversation

jrose-apple commented Sep 9, 2016

Uh oh!

jrose-apple commented Sep 9, 2016

Uh oh!

jrose-apple commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcsgexi commented Sep 9, 2016

Uh oh!

DougGregor commented Sep 12, 2016

Uh oh!

jrose-apple commented Sep 13, 2016

Uh oh!

jrose-apple commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

swift-ci commented Sep 20, 2016

Uh oh!

swift-ci commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

nkcsgexi commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 20, 2016

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

aciidgh commented Sep 21, 2016

Uh oh!

aciidgh commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Sep 21, 2016

Uh oh!

nkcsgexi commented Sep 21, 2016

Uh oh!

jrose-apple commented Sep 22, 2016

Uh oh!

jrose-apple commented Sep 22, 2016

Uh oh!

jrose-apple commented Sep 22, 2016

jrose-apple commented Sep 13, 2016 •

edited

Loading