[Function builders] Infer function builder from a protocol requirement. #32005

DougGregor · 2020-05-25T06:21:42Z

Allow a protocol requirement for a function or property to declare
a function builder. A witness to such a protocol requirement will
infer that function builder when all of the following are two:

The witness does not explicitly adopt a function builder
The witness is declared within the same context that conforms to the
protocol requirement (e.g., same extension or primary type declaration)
The witness's body does not contain a "return" statement (since those
disable the function builder transform).

Allow a protocol requirement for a function or property to declare a function builder. A witness to such a protocol requirement will infer that function builder when all of the following are two: * The witness does not explicitly adopt a function builder * The witness is declared within the same context that conforms to the protocol requirement (e.g., same extension or primary type declaration) * The witness's body does not contain a "return" statement (since those disable the function builder transform).

DougGregor · 2020-05-25T06:55:33Z

@swift-ci please smoke test

DougGregor · 2020-05-25T06:55:39Z

@swift-ci please test source compatibility

Now that this request will be triggered more often, due to inference of function builders, cache the result.

Eliminates both circular references and unwanted dependencies.

DougGregor · 2020-05-25T20:39:49Z

@swift-ci please smoke test

DougGregor · 2020-05-25T20:39:56Z

@swift-ci please test source compatibility

DougGregor · 2020-05-25T20:44:08Z

@swift-ci smoke test macOS

DougGregor · 2020-05-26T01:08:05Z

Debug source compatibility failure in ACHNBrowserUI is unrelated. It passed in Release, so I think we're fine.

DougGregor · 2020-05-26T01:11:05Z

What you are thoughts on the situation where closure body is a function body but it has a return just as a force of habit?

I think it's reasonable to have that as a diagnostic path---if there's are explicit returns and we have some sort of type conflict in the returns, in a closure that was matched up against a parameter that has a function builder, try dropping the returns. For this PR, we'd want to recompute the inference path while ignoring the returns to fit in with such a diagnostic.

xedin · 2020-05-26T01:19:35Z

To make it easier for diagnostics would it be possible to make return of this function reacher e.g. either type or a failure kind - return in the body, used on getter etc.? applyFunctionBuilder should probably do the same thing...

DougGregor · 2020-05-26T02:14:07Z

To make it easier for diagnostics would it be possible to make return of this function reacher e.g. either type or a failure kind - return in the body, used on getter etc.? applyFunctionBuilder should probably do the same thing

When we do that diagnostic, we'll want to go through inferFunctionBuilderType again with an "ignore explicit returns" bit on it. Checking for explicit returns early avoids a bunch of name lookups and conformance resolutions that we don't want all the time (because they add incremental dependencies).

xedin · 2020-05-26T02:37:01Z

I asking because instead of assuming that ‘return`s could be a problem we can get an indication from this method what exactly went from and act on that when trying to call the method again for diagnostics...

DougGregor · 2020-05-26T04:29:33Z

I asking because instead of assuming that ‘return`s could be a problem we can get an indication from this method what exactly went from and act on that when trying to call the method again for diagnostics...

If we need more than what I'm suggesting, we can extend the return type. We shouldn't do it proactively because (1) we want to minimize dependencies when not in recovery mode, and (2) we might not need the extra information until we have that diagnostic.

xedin · 2020-05-26T04:46:00Z

I see that there is a new diagnostic about adding explicit return added by this PR, why not add a reverse case as well and see what needs to be done to support that?

DougGregor · 2020-05-26T05:36:01Z

I see that there is a new diagnostic about adding explicit return added by this PR, why not add a reverse case as well and see what needs to be done to support that?

Happy to look at a general improvement for function builders diagnostics to try to address this failure case (an explicit return unexpectedly disabling the function builder transform), but that should be separate. It's a good improvement independent of this PR, and doesn't get significantly harder with this PR.

xedin · 2020-05-26T05:51:47Z

Sure! It would be great to address that sooner than later though because not having a good diagnostic there is very confusing.

…rements

DougGregor · 2020-05-26T06:43:10Z

@swift-ci please smoke test

DougGregor · 2020-05-26T06:44:27Z

@swift-ci please smoke test

DougGregor · 2020-05-26T14:41:52Z

@swift-ci smoke test Linux

rjmccall · 2020-05-26T17:34:31Z

lib/AST/Decl.cpp

-  // Fast path: most declarations (especially parameters, which is where
-  // this is hottest) do not have any custom attributes at all.
-  if (!getAttrs().hasAttribute<CustomAttr>()) return nullptr;
-


Losing these hot paths seems abstractly unfortunate. I never profiled them, of course, so who knows, but this does get called for every parameter of every direct call, at least when the argument is a closure. Have you measured compile time/memory impact?

You're only inferring function builders for method bodies, I think, which should let us more selectively weaken the fast-path check.

The request-evaluator needs to be fast enough so that we don't need to scatter hot paths like this everywhere, because they suppress incremental dependency generation.

I feel like the statement "[it just] needs to be fast enough" is doing a lot of work for something that inherently looks up and permanently allocates a hash-table entry.

rjmccall · 2020-05-26T17:35:15Z

lib/Sema/TypeCheckProtocol.cpp

@@ -5038,10 +5038,11 @@ diagnoseMissingAppendInterpolationMethod(NominalTypeDecl *typeDecl) {
  }
 }

-SmallVector<ProtocolConformance *, 2>
+std::vector<ProtocolConformance *>


Should this use the typedef if you're relying on equivalence?

Yeah, it should. Good call.

rjmccall · 2020-05-26T17:49:18Z

lib/Sema/TypeCheckRequestFunctions.cpp

+        diag::function_builder_infer_ambig, lookupDecl->getName(),
+        functionBuilderType, otherFunctionBuilderType);
+    decl->diagnose(diag::function_builder_infer_add_return)
+      .fixItInsert(funcDecl->getBodySourceRange().End, "return <#expr#>\n");


I guess this can't have official source-compatibility impact given that we haven't standardized the feature.

It seems unfortunate that this gets diagnosed here eagerly. I would not expect calling getFunctionBuilderType() to be an authoritative statement by the caller that it wants to try to use the function-builder transform on a method body, and I don't think it's reasonable to diagnose in other cases.

You would prefer that we return the (ambiguous) set of possible function builders rather than diagnose here?

That might be a more reasonable design, yeah. Or if you really need this behavior, consider renaming the function in a way that makes it clearer that it's not "logically pure" on otherwise-valid code — it triggers query-specific diagnostics if there's no unambiguous function builder type.

DougGregor · 2020-05-26T17:59:35Z

I'll do some cleanup after this PR for stuff that still needs it.

DougGregor added 3 commits May 25, 2020 13:30

Cache LookupAllConformancesInContextRequest.

c5a9575

Now that this request will be triggered more often, due to inference of function builders, cache the result.

[Function builders] Short-circuit function builder inference earlier.

54c1267

Eliminates both circular references and unwanted dependencies.

[Tests] Add newly-introduced dependencies to the incremental verifier.

5d9d4f9

[Function builders] Add tests for function builders on protocol requi…

8cec6b5

…rements

rjmccall reviewed May 26, 2020

View reviewed changes

DougGregor merged commit 8e0ec60 into swiftlang:master May 26, 2020

DougGregor deleted the function-builder-infer branch May 26, 2020 17:59

[Function builders] Infer function builder from a protocol requirement. #32005

[Function builders] Infer function builder from a protocol requirement. #32005

Uh oh!

Conversation

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 25, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

xedin commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

xedin commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

xedin commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

xedin commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

rjmccall May 26, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor May 26, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall May 26, 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!

rjmccall May 26, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor May 26, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall May 26, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor May 26, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall May 26, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor commented May 26, 2020

Uh oh!

Uh oh!

rjmccall May 26, 2020 •

edited

Loading