[AST, Sema] Replace HoleType with PlaceholderType #35589

Jumhyn · 2021-01-25T20:47:00Z

This is a first pass at an implementation for placeholder types. Not yet ready for prime-time but much of the functionality is there!

CodaFi

A fantastic first go. I can only speak to the type AST and type resolution bits though.

CodaFi · 2021-02-02T07:11:12Z

lib/Sema/TypeChecker.cpp

+      }, /*placeholderHandler*/ [&]() {
+        // FIXME: Don't let placeholder types escape type resolution.
+        // For now, just return the placeholder type.
+        return Ctx.ThePlaceholderType;


Maybe not something to do here specifically, but I was really hoping we could use placeholder types to make UnboundGenericType obsolete. Wherever type resolution forms one, it could instead build a bound generic type full of placeholders. With that in mind, having them escape unopened is not so bad.

Yeah this was on my mind as well, especially since one of the goals of the proposal is to have UnboundGeneric be semantically equivalent to UnboundGeneric<_, ..., _>. Obsoleting UnboundGenericType would really help validate that approach.

It's been a bit since I worked on this but IIRC I started down that path, and then realized there was enough complexity (and bits that I didn't quite understand) in ConstraintSystem::openUnboundGenericType that it was going to turn into a bit of a rabbit hole, so I punted on that in favor of getting the basic functionality working.

Thank you for taking a look at this!

CodaFi · 2021-02-02T18:56:48Z

@Jumhyn @slavapestov and I are really excited to see this go in sooner rather than later. If you could separate the parsing code from everything else, we could get this merged immediately. Even if the proposal is not accepted, the re-model in here is extremely valuable.

Jumhyn · 2021-02-02T19:46:58Z

@CodaFi Thank you for the heads up. I'll work on separating those parts out. Could you provide just a bit more specificity on which aspects you'll want to merge right away? (E.g., do you want just the minimum to support the changes made in ParseType.cpp, or do you want basically everything here excluding the changes to TypeResolution::forContextual that actually generate the type variables for placeholder types?)

Just want to make sure we're on the same page!

CodaFi · 2021-02-02T19:49:17Z

Let's take the approach of: basically everything that doesn't touch the surface syntax can come along for the ride.

CodaFi · 2021-02-02T19:50:25Z

That includes all the type checker changes, type resolution changes, AST changes, etc. Just not the parser bits.

xedin · 2021-02-02T19:50:29Z

@Jumhyn Why not use HoleType as an explicit placeholder the same way we use it as implicit one for diagnostics? It's already has originating type variable so it could be used to suggest possible types.

CodaFi · 2021-02-02T19:55:55Z

Why not use HoleType as an explicit placeholder the same way we use it as implicit one for diagnostics?

Isn't HoleType a contextual type and PlaceholderType an interface type? You can't create a HoleType without a type variable for example. We could certainly open a PlaceholderType as a HoleType.

xedin · 2021-02-02T20:06:28Z

@CodaFi I would be great if we could unify three things here - HoleType, PlaceholderType and UnresolvedType - all three effectively aim to represent the same thing... I think we can rename HoleType into a PlaceholderType if that fits better and use it for unknown types in constraint system and AST - currently it's possible to use HoleType for type variable, declaration type and dependent member type.

Jumhyn · 2021-02-02T20:10:03Z

That includes all the type checker changes, type resolution changes, AST changes, etc. Just not the parser bits.

Ahh I had interpreted your suggestion to "separate the parsing code" the other way around. 🤦‍♂️ That makes way more sense, thank you. Will do!

I think we can rename HoleType into a PlaceholderType if that fits better and use it for unknown types in constraint system and AST.

So under this model we'd want to add PlaceholderTypeRepr as another potential Originator for a HoleType, and then type resolution in the constraint system context would turn HoleTypes into TypeVariableTypes, which would get turned back into HoleTypes if they could not be solved. Does that sound like the right approach?

xedin · 2021-02-02T20:15:55Z

@Jumhyn Currently HoleType is a replacement for a type variable since we need all type variables to be bound to some type before valid solution could be formed. My suggestion would be:

If placeholder arrives from a declaration - turn it into a type variable and record in the locator that type variable has originated from a placeholder (so that finalization could replace type variables back into originating placeholders if they weren't bound);
if type variable couldn't be determined bind it to a placeholder to form a valid solution (place holder becomes a replacements for HoleType) in diagnostic mode;
If solution has a PlaceholderType in it - use it in the AST instead of using UnresolvedType, currently HoleType are turned into UnresolvedTypes.

Jumhyn · 2021-02-05T19:11:08Z

@CodaFi @xedin Sorry for the delay—this turned out to be a bit of a busy week for me. Hoping to get to this over the weekend!

xedin · 2021-02-05T19:51:06Z

No worries!

Jumhyn · 2021-02-08T14:28:03Z

Hey @xedin @CodaFi @hborla, please take a look at the latest and let me know your thoughts. I've merged HoleType and PlaceholderType under the PlaceholderType name, basically implementing the first two items from your last comment, @xedin. If this seems like the right direction I can move forward with merging UnresolvedType and PlaceholderType as well, either in this PR or a follow-up.

xedin

Looks good to me so far!

xedin · 2021-02-08T19:29:07Z

include/swift/AST/Types.h


    /// This type contains a DependentMemberType.
-    HasDependentMember   = 0x200,


Just a note - making unrelated whitespace changes like this sometimes complicates the review since it's easy to miss value changes...

Ah, that must have been clang-format, I didn't make any intentional whitespace changes here. I'll revert!

xedin · 2021-02-08T19:35:18Z

include/swift/Sema/CSBindings.h

-  /// regardless of where hole type originated.
-  bool isHole() const {
-    if (isDirectHole())
+  /// if there is only one binding and it's a placeholder type, consider


@Jumhyn @hborla What do you think about keeping "hole" terminology in the constraint system? My personal preference would be to keep calling "untyped" places in constraint system holes and call them placeholders in AST.

I agree - I like the term "hole" for the constraint system because "placeholder" implies that it can be replaced, but if we bind to a placeholder in the constraint system, that means it solver could not replace it with a real type.

Sounds good to me, I don't have super strong feelings either way! So we'd keep the type name as PlaceholderType, but all the constraint system methods (such as isDirectHole()) stay as is?

Yes, that’s the idea.

Effectively we are just utilizing a placeholder in a solution and AST to represent places which couldn’t be inferred.

@hborla @xedin Thoughts on TVO_CanBindToPlaceholder vs. TVO_CanBindToHole? This is the only one in the constraint system that seems to me like the use of "hole" is perhaps more confusing than "placeholder", since it most literally represents whether we will bind the type var to a PlaceholderType or not. But I'm good with whatever y'all think is best!

I think we should leave it as hole since it would be consistent with the rest.

Jumhyn · 2021-02-09T13:42:58Z

@xedin @hborla Have updated to restore the "hole" terminology throughout the constraint system except where we are actually referring to the PlaceholderType type directly. Let me know if y'all have further thoughts!

Jumhyn · 2021-02-16T21:22:23Z

Hey @xedin @hborla @CodaFi, just wanted to check in on the status here—is there further work this needs to be merge-able? Should we fold the work of combining UnresolvedType and PlaceholderType as part of this same PR?

CodaFi

Holly and Pavel will have more to say. But LGTM

xedin

LGTM!

xedin · 2021-02-17T00:09:49Z

include/swift/Sema/ConstraintSystem.h

@@ -5024,6 +5024,24 @@ class OpenUnboundGenericType {
  }
 };

+class HandlePlaceholderType {
+  ConstraintSystem &cs;
+  const ConstraintLocatorBuilder &locator;


I'd suggest we allocate a locator here instead of referencing a builder since we'd have to build a locator per placeholder anyway.

Gotcha, will update!

For now, don't do anything useful in clients. Specifying a placeholder type is still an error

Rename `openUnboundGenericTypes` to `convertInferableTypes`. In addition to unbound generics, this method also converts placeholder types to fresh type variables.

Move markAcceptableDiscardExprs into PreCheckExpr so that we can perform this analysis before we convert expressions like `_? = <value>` into `_ = <value>` since `_?` is now an expression with meaning, and we only want to perform this transformation when `_?` is on the LHS of an assignment

HoleType basically served the same purpose as PlaceholderType. This commit unifies the two.

Now that we're checking DiscardAssignmentExprs in PreCheckExpr, we have to be careful to make sure we don't diagnose anything until after SequenceExprs have been folded, since the relevant AssignExprs won't even have a "right hand side" and "left hand side" until after folding.

Jumhyn · 2021-02-17T16:26:48Z

@xedin Updated to allocate a ConstraintLocator upon construction of HandlePlaceholderType! Mind giving this a test run for me?

hborla · 2021-02-17T16:32:24Z

@swift-ci please smoke test

Jumhyn · 2021-02-17T16:37:57Z

Thank you @hborla!

hborla · 2021-02-17T16:50:53Z

@Jumhyn No problem! If you'd like, I think it's time for you to request commit access 🙂

xedin · 2021-02-17T18:53:39Z

@swift-ci please smoke test Windows platform

xedin · 2021-02-17T18:53:53Z

@swift-ci please smoke test macOS platform

xedin · 2021-02-17T18:54:04Z

@swift-ci please test source compatibility

Jumhyn · 2021-02-19T17:06:14Z

@xedin Do these failures in the release source compat run seem related to you? What's the best way for me to investigate those failures locally?

xedin · 2021-02-19T18:02:56Z

I don't think these errors are related to your changes because they also happen in main branch - https://ci.swift.org/view/Source%20Compatibility/job/swift-main-source-compat-suite/

In general to investigate source compatibility issues you'd have to clone swift-source-compat-suite repository and use runner.py script with projects.json file to trigger build of a project you are interested in. That would clone the project locally, checkout revision specified in projects.json and run xcodebuild or swiftpm build (depending on project configuration). runner.py also allows you to pass additional swift/clang flags and use custom compiler path as well.

xedin · 2021-02-19T18:03:50Z

Let's wait for @hborla to take a look and I think if there is nothing major this should be good to go.

hborla

LGTM! I just have one tiny opinion about a function name

hborla · 2021-02-19T18:55:02Z

include/swift/Sema/ConstraintSystem.h

-  /// \returns The opened type.
-  Type openUnboundGenericTypes(Type type, ConstraintLocatorBuilder locator);
+  /// \returns The converted type.
+  Type convertInferableTypes(Type type, ConstraintLocatorBuilder locator);


Subjective nit: I don't love the term "convert/conversion" here, because a type conversion means something different. I would rather use something like replaceInferableTypes or substituteInferableTypes. We could also go for the long form and tack on WithTypeVar[iable]s at the end depending on how verbose we want to be 🙂

That makes sense to me re: "convert"! I'll swap this out with one of your suggestions.

Jumhyn · 2021-02-19T19:35:33Z

@hborla Done!

xedin · 2021-02-19T19:39:12Z

@swift-ci please smoke test

xedin · 2021-02-20T09:21:31Z

@swift-ci please test

Jumhyn · 2021-02-22T15:22:01Z

@hborla or @xedin looks like we are good to go, mind merging? 🙂

hborla · 2021-02-22T16:36:04Z

@Jumhyn done, thanks again for your work on this!

Jumhyn · 2021-02-22T17:08:52Z

Thank you both (+ @CodaFi) for your feedback and guidance!

hborla self-requested a review January 25, 2021 21:34

CodaFi reviewed Feb 2, 2021

View reviewed changes

Jumhyn force-pushed the placeholder-types branch from a088b5c to df9d2f2 Compare February 8, 2021 14:24

Jumhyn force-pushed the placeholder-types branch from df9d2f2 to 1c10526 Compare February 8, 2021 17:11

Jumhyn changed the title ~~[DNM] WIP: placeholder types implementation~~ [AST, Sema] Replace HoleType with PlaceholderType Feb 8, 2021

Jumhyn marked this pull request as ready for review February 8, 2021 17:12

Jumhyn requested a review from CodaFi February 8, 2021 17:12

xedin reviewed Feb 8, 2021

View reviewed changes

Jumhyn force-pushed the placeholder-types branch from 1c10526 to 9536e17 Compare February 9, 2021 13:41

CodaFi approved these changes Feb 17, 2021

View reviewed changes

xedin approved these changes Feb 17, 2021

View reviewed changes

Jumhyn added 5 commits February 16, 2021 22:59

[AST] Add PlaceholderTypeRepr

0f7ec99

[Parse] Enable parsing of PlaceholderTypeRepr

580fd40

[AST] Introduce PlaceholderType and ThePlaceholderType singleton

d78d95e

[Sema] Introduce placeholder handler to TypeResolver

e508fb3

For now, don't do anything useful in clients. Specifying a placeholder type is still an error

[Sema] Handle placeholder types in during expression type resolution

65521b5

Jumhyn added 8 commits February 16, 2021 22:59

[Sema] Convert placeholder types to type vars wherever we open generics

fbb78cd

Rename `openUnboundGenericTypes` to `convertInferableTypes`. In addition to unbound generics, this method also converts placeholder types to fresh type variables.

[Sema] Convert placeholder types in expression conversion types

a79e7b5

[AST] Assume that placeholder types conform to all protocols

a5d0153

Rename HoleType to PlaceholderType

e4ea167

HoleType basically served the same purpose as PlaceholderType. This commit unifies the two.

Disable PlaceholderType parsing

1819d33

[Sema] Allocate locator for HandlePlaceholderType

ddd4842

Jumhyn force-pushed the placeholder-types branch from 9536e17 to ddd4842 Compare February 17, 2021 16:24

hborla approved these changes Feb 19, 2021

View reviewed changes

[Sema] Rename convertInferableTypes to replaceInferableTypesWithTypeVars

8539782

hborla merged commit 1102835 into swiftlang:main Feb 22, 2021


		/// This type contains a DependentMemberType.
		HasDependentMember = 0x200,

[AST, Sema] Replace HoleType with PlaceholderType #35589

[AST, Sema] Replace HoleType with PlaceholderType #35589

Uh oh!

Conversation

Jumhyn commented Jan 25, 2021

Uh oh!

CodaFi left a comment

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!

CodaFi commented Feb 2, 2021

Uh oh!

Jumhyn commented Feb 2, 2021

Uh oh!

CodaFi commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Feb 2, 2021

Uh oh!

xedin commented Feb 2, 2021

Uh oh!

CodaFi commented Feb 2, 2021

Uh oh!

xedin commented Feb 2, 2021

Uh oh!

Jumhyn commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 2, 2021

Uh oh!

Jumhyn commented Feb 5, 2021

Uh oh!

xedin commented Feb 5, 2021

Uh oh!

Jumhyn commented Feb 8, 2021

Uh oh!

xedin left a comment

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!

Jumhyn commented Feb 9, 2021

Uh oh!

Jumhyn commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

xedin left a comment

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!

Jumhyn commented Feb 17, 2021

Uh oh!

hborla commented Feb 17, 2021

Uh oh!

Jumhyn commented Feb 17, 2021

CodaFi commented Feb 2, 2021 •

edited

Loading

Jumhyn commented Feb 2, 2021 •

edited

Loading

Jumhyn commented Feb 16, 2021 •

edited

Loading

xedin commented Feb 19, 2021 •

edited

Loading