[Sema] Requestify default argument type checking #27756

hamishknight · 2019-10-17T16:36:52Z

This PR introduces two new requests, one to compute initializer contexts for default arguments, and another to type-check default arguments within this context.

This PR then splits getDefaultValue into 2 accessors:

getStructuralDefaultExpr which retrieves the potentially un-type-checked default argument expression.
getTypeCheckedDefaultExpr which retrieves a fully type-checked default argument expression.

And also adds hasDefaultExpr, which allows checking for the presence of a default expr without kicking off a request.

On its own, this isn't enough to resolve SR-11085, I'm going to tackle that in a follow-up to keep the size of this PR down.

hamishknight · 2019-10-17T16:37:17Z

@swift-ci please test

CodaFi · 2019-10-17T17:17:53Z

A high level comment: Why do we need to keep around a trichotomy of "parsed/typechecked/unchecked" if all of the callers before don't really care about the distinction between parsed and typechecked? I think you can get away with "definitely typechecked" and "potentially typechecked - no requests".

hamishknight · 2019-10-17T17:42:27Z

@CodaFi My understanding was that we were aiming to eventually get rid of that mutable state within the AST, so the result of getParsedDefaultExpr doesn't change if we happen to trigger the type-checking of the default.

CodaFi · 2019-10-17T17:50:51Z

I guess I'm wondering what's stopping us from doing it now?

hamishknight · 2019-10-17T18:17:32Z

@CodaFi From removing the mutable state? Things like the AST walker currently still need to set it (until it is changed to return a new AST), and as I understand it, it's currently more efficient to access the state from the AST than the evaluator's cache.

hamishknight · 2019-10-29T16:24:34Z

Ping @slavapestov

hamishknight · 2019-11-06T00:50:28Z

@swift-ci please test

swift-ci · 2019-11-06T03:29:10Z

Build failed
Swift Test OS X Platform
Git Sha - e3a53cebe5265999f7157c9757ca74c4e3764bbc

slavapestov · 2019-11-06T16:57:45Z

Sorry, I’ll review this today.

slavapestov · 2019-11-06T17:50:24Z

include/swift/AST/Decl.h

@@ -5208,7 +5211,8 @@ class ParamDecl : public VarDecl {
  TypeRepr *TyRepr = nullptr;

  struct StoredDefaultArgument {
-    PointerUnion<Expr *, VarDecl *> DefaultArg;
+    PointerUnion<Expr *, VarDecl *> ParsedExprOrVar;
+    Optional<Expr *> TypeCheckedExpr;


I'm not sure it makes sense to split these up currently. Type checking an expression mutates it, destroying the original value.

@slavapestov Ah right, I guess in that case we should just expose something like getStructuralDefaultExpr and getTypeCheckedDefaultExpr where the former returns either an un-type-checked or a type-checked expr?

slavapestov · 2019-11-06T17:51:28Z

include/swift/AST/TypeCheckRequests.h

@@ -1744,6 +1744,47 @@ class ValueWitnessRequest
  void cacheResult(Witness value) const;
 };

+/// Computes initializer contexts for each of the parameters with default
+/// arguments within a parameter list.
+class DefaultArgumentInitContextsRequest


Is it really worth making this a request? It looks like a request that's run for its side effects, which is something we would like to avoid having. Maybe it should at least be per-ParamDecl and not per-ParameterList?

@slavapestov The reasoning for putting at the level of ParameterList is to allow us to access the parameter index when creating the contexts. We could make it per-ParamDecl, but then we'd need to iterate over the parameter list for each param to find its own index. Either that or start storing the index on ParamDecl. What do you think?

@slavapestov On second thought, we can totally make this per-ParamDecl, we can just cache the other parameter's init contexts while we iterate. Does the updated code look good?

hamishknight · 2019-11-11T00:13:57Z

@swift-ci please test

CodaFi · 2019-11-11T00:26:29Z

Sorry in advance @hamishknight, #28179 is gonna cause a bunch of merge conflicts here.

hamishknight · 2019-11-11T00:34:47Z

@CodaFi No worries, it's worth it :)

This commit adds a request that computes the initializer context for a parameter with a default expr or stored property default. This avoids having to compute them for synthesized decls and is a step towards requestifying default argument parsing.

This commit introduces a request to type-check a default argument expression and splits `getDefaultValue` into 2 accessors: - `getStructuralDefaultExpr` which retrieves the potentially un-type-checked default argument expression. - `getTypeCheckedDefaultExpr` which retrieves a fully type-checked default argument expression. In addition, this commit adds `hasDefaultExpr`, which allows checking for a default expr without kicking off a request.

hamishknight · 2019-11-11T21:54:43Z

@swift-ci please smoke test

hamishknight · 2019-11-11T23:58:23Z

@swift-ci please smoke test Linux platform

slavapestov · 2019-11-14T20:26:50Z

lib/AST/Decl.cpp

@@ -6046,15 +6046,79 @@ AnyFunctionType::Param ParamDecl::toFunctionParam(Type type) const {
  return AnyFunctionType::Param(type, label, flags);
 }

-void ParamDecl::setDefaultValue(Expr *E) {
+Initializer *ParamDecl::getDefaultArgumentInitContextCached() const {


This could return an Optional<> I think. Also typically we name these methods "getCached...()"

Rename getDefaultArgumentInitContextCached, and have it return an Optional<Initializer *>.

hamishknight · 2019-11-14T20:53:18Z

@swift-ci please smoke test

hamishknight requested a review from slavapestov October 17, 2019 16:36

hamishknight force-pushed the carpe-default branch 2 times, most recently from 262d08a to c5a65eb Compare October 22, 2019 22:31

hamishknight force-pushed the carpe-default branch from c5a65eb to 73d2419 Compare October 28, 2019 17:35

hamishknight force-pushed the carpe-default branch 3 times, most recently from 6bc4abd to e3a53ce Compare November 6, 2019 00:48

hamishknight force-pushed the carpe-default branch 2 times, most recently from d68d9c6 to c28bbd7 Compare November 6, 2019 15:59

slavapestov reviewed Nov 6, 2019

View reviewed changes

hamishknight force-pushed the carpe-default branch from fd2e71c to e399e3c Compare November 11, 2019 00:11

hamishknight added 3 commits November 11, 2019 13:49

[AST] Add swift::getParameterList

4d216c7

hamishknight force-pushed the carpe-default branch from e399e3c to 01d5c00 Compare November 11, 2019 21:54

slavapestov approved these changes Nov 14, 2019

View reviewed changes

Address review feedback

3c05aca

Rename getDefaultArgumentInitContextCached, and have it return an Optional<Initializer *>.

hamishknight merged commit 1be22c5 into swiftlang:master Nov 14, 2019

hamishknight deleted the carpe-default branch November 14, 2019 22:09

drodriguez mentioned this pull request Nov 15, 2019

[msvc] std::move the pointer for the method that requires an rvalue. #28280

Merged

[Sema] Requestify default argument type checking #27756

[Sema] Requestify default argument type checking #27756

Uh oh!

Conversation

hamishknight commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishknight commented Oct 17, 2019

Uh oh!

CodaFi commented Oct 17, 2019

Uh oh!

hamishknight commented Oct 17, 2019

Uh oh!

CodaFi commented Oct 17, 2019

Uh oh!

hamishknight commented Oct 17, 2019

Uh oh!

hamishknight commented Oct 29, 2019

Uh oh!

hamishknight commented Nov 6, 2019

Uh oh!

swift-ci commented Nov 6, 2019

Uh oh!

slavapestov commented Nov 6, 2019

Uh oh!

slavapestov Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Nov 11, 2019

Uh oh!

CodaFi commented Nov 11, 2019

Uh oh!

hamishknight commented Nov 11, 2019

Uh oh!

hamishknight commented Nov 11, 2019

Uh oh!

hamishknight commented Nov 11, 2019

Uh oh!

slavapestov Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Nov 14, 2019

Uh oh!

Uh oh!

hamishknight commented Oct 17, 2019 •

edited

Loading