[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T #1160

gregomni · 2016-02-02T03:12:16Z

Correctly determine callee closeness for func/ops that include generics as part of more complicated parameters, i.e. tuple or closure args containing generics as elements or args/results. Still only handling single archetypes.

Tests added, all tests pass. The one changed diagnostic in misc_diagnostics is sort of a regression, although both messages are of about equal quality, I think. This commit lets evaluateCloseness find a second partial match for ~= there, but the two partial matches involve different parameter archetypes, so filtering makes the callee argument failure info invalid as a result, thus going back to the generic message as opposed to a specific argument failure message. (In short, the older specific error was actually incorrectly over-specific.)

slavapestov · 2016-02-02T09:22:52Z

lib/Sema/CSDiag.cpp

+  if (paramType->getKind() != actualArgType->getKind())
+    return false;
+
+  // A little walker that just collects direct child types. Can't easily have a


Take a look at include/swift/AST/TypeMatcher.h, it might be what you need here

Aha! Thank you, that's exactly what I needed. It seemed like something like that must exist, but somehow I missed finding it.

slavapestov · 2016-02-02T09:44:16Z

I'm not very familiar with the diagnostics code so I'm still learning too. I noticed a few potential improvements. I won't insist you address all of them in this patch, but take a look and see if any look like low-hanging fruit. The code can always be improved later too.

gregomni · 2016-02-02T17:33:00Z

I'd like to try the TypeMatcher path and also fix the differing bound generic structs case, and then leave matching with type aliases and other sugared forms for the (hopefully near) future.

gregomni · 2016-02-03T18:10:44Z

Ok! Rewrote this patch to use TypeMatcher, which is a much cleaner way to do it.

Switched Generic -> Archetype transform to use ArchetypeBuilder::mapTypeIntoContext() as you suggested. So this now is hopefully future-proofed, and it also should work for dependent member types.
TypeMatcher had existing mismatch code for checking the base of BoundGenericType types, so that potential problem got fixed by adopting it.
Noticed that one of the improved diagnoses from one of my earlier patches here regressed with the TypeMatcher, and when I looked into it, it was because the previous code got a correct result accidentally by doing something wrong. The parameter was a SubstitutedType and the previous code was looking inside its children (its original type) and finding an archetype there that didn't match. That no longer happens with TypeMatcher because the paramType arg is canonicalized, which replaces the SubstitutedType with its replacement type.

The correct solution to do this in general and not by accident is to pre-populate the substitution vectors with any substitutions that have already been made, so this code also does that in findGenericSubstitutions() now, which leads to a few more corrected diagnoses (such as in Generics/function_defs.swift).

Still not looking for type aliases or sugared types, that's for the future.

All tests pass, new test added, changes to diagnoses for existing tests also cover the new features here. Went ahead and squashed right away because the original commit got entirely changed anyway, so there wasn't anything readable about the diff between the two commits.

slavapestov · 2016-02-03T21:52:33Z

This looks good! I'll merge it once it goes through CI.

slavapestov · 2016-02-03T21:52:39Z

@swift-ci Please test

gregomni · 2016-02-03T22:14:10Z

Oops. I see I was running just -t instead of -T and a couple of the validation tests failed. Will go back and look at those.

gregomni · 2016-02-03T23:00:14Z

@swift-ci Please test

gregomni · 2016-02-03T23:14:45Z

@slavapestov Looks like only people with commit access can request testing. Made a second commit to check for UnresolvedType in the paramType and changed to a better diagnosis in one of the validation tests, and everything now passes for me locally.

slavapestov · 2016-02-03T23:17:14Z

@swift-ci Please test

slavapestov · 2016-02-03T23:17:35Z

@gregomni Requesting tests doesn't always work for me either -- I think it might just be flaky. I'll follow up internally

gregomni · 2016-02-03T23:21:28Z

@slavapestov I went and found the announcement and it does explicitly say that only committers can kick it off: https://swift.org/blog/swift-ci/. I can understand that, since it's a fairly long-lived process you don't want random people kicking off zillions of tests.

gregomni · 2016-02-04T02:32:06Z

Huh. There is one failure in a test in IRGen, which I don't see how it could be related to these changes.

gregomni · 2016-02-04T04:42:02Z

For what it's worth, the next build on that jenkins instance https://ci.swift.org/job/swift-PR-osx/18/, failed with the exact same IRGen error, and it was just doing a build for stdlib changes. So I think that test must be broken by some previous pull.

slavapestov · 2016-02-04T05:51:07Z

I belive the broken test was a commit by @DougGregor that he has since fixed.

slavapestov · 2016-02-04T05:51:11Z

@swift-ci Please test

slavapestov · 2016-02-04T06:49:23Z

lib/Sema/CSDiag.cpp

@@ -1325,7 +1397,8 @@ void CalleeCandidateInfo::filterList(ArrayRef<CallArgParam> actualArgs) {
    // If this isn't a function or isn't valid at this uncurry level, treat it
    // as a general mismatch.
    if (!inputType) return { CC_GeneralMismatch, {}};
-    return evaluateCloseness(inputType, actualArgs);
+    Decl *decl = candidate.getDecl();
+    return evaluateCloseness(decl ? decl->getInnermostDeclContext() : nullptr, inputType, actualArgs);


Last nitpick before I merge this -- this line is too long, wrap it after the nullptr

…d) -> T Correctly determine callee closeness for func/ops that include generics as part of more complicated parameters, i.e. tuple or closure args containing generics as elements or args/results. Still only handling single archetypes. Also added code to check generic substitutions already made in the callee parameters, which further helps diagnosis.

gregomni · 2016-02-04T06:54:59Z

Sure thing. Wrapped that line, and squashed.

[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T

jtbandes · 2016-02-04T07:32:00Z

Cool!

This is a quick follow-up to <swiftlang#1160>, to replace the becoming deprecated getArchetype() with doing the same thing via calling through to ArchetypeBuilder with a declContext. Uses findGenericSubstitutions() to do so. So this site could take advantage of destructuring of more complex params containing generics. Right now, though, that never happens. For complex params the (unfortunately, worse) diagnosis happens in diagnoseFailureForExpr() on the argument expression before reaching here. I’d like to improve that in future work.

slavapestov reviewed Feb 2, 2016
View reviewed changes

gregomni changed the title ~~[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T~~ [WIP][Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T Feb 2, 2016

slavapestov self-assigned this Feb 3, 2016

gregomni force-pushed the generic-func-args branch 2 times, most recently from 4db95dd to 7cac541 Compare February 3, 2016 17:54

gregomni changed the title ~~[WIP][Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T~~ [Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T Feb 3, 2016

slavapestov reviewed Feb 4, 2016
View reviewed changes

gregomni force-pushed the generic-func-args branch from 8407b05 to 6b30695 Compare February 4, 2016 06:54

slavapestov added a commit that referenced this pull request Feb 4, 2016

Merge pull request #1160 from gregomni/generic-func-args

99b0e77

[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T

slavapestov merged commit 99b0e77 into swiftlang:master Feb 4, 2016

This was referenced Feb 4, 2016

Replace use of getArchetype() in diagnoseGenericParameterErrors() #1200

Merged

[Sema] Extend callee diagnoses to non-conforming complex args including generics. #1229

Merged

[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T #1160

[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T #1160

Conversation

gregomni commented Feb 2, 2016

Uh oh!

slavapestov Feb 2, 2016

Choose a reason for hiding this comment

Uh oh!

gregomni Feb 2, 2016

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Feb 2, 2016

Uh oh!

gregomni commented Feb 2, 2016

Uh oh!

gregomni commented Feb 3, 2016

Uh oh!

slavapestov commented Feb 3, 2016

Uh oh!

slavapestov commented Feb 3, 2016

Uh oh!

gregomni commented Feb 3, 2016

Uh oh!

gregomni commented Feb 3, 2016

Uh oh!

gregomni commented Feb 3, 2016

Uh oh!

slavapestov commented Feb 3, 2016

Uh oh!

slavapestov commented Feb 3, 2016

Uh oh!

gregomni commented Feb 3, 2016

Uh oh!

gregomni commented Feb 4, 2016

Uh oh!

gregomni commented Feb 4, 2016

Uh oh!

slavapestov commented Feb 4, 2016

Uh oh!

slavapestov commented Feb 4, 2016

Uh oh!

slavapestov Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

gregomni commented Feb 4, 2016

Uh oh!

jtbandes commented Feb 4, 2016

Uh oh!

Uh oh!