Skip to content

AST: Transition to storing substitution maps in bound generic type nodes #31895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AnthonyLatsis
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis commented May 19, 2020

This change as it currently is will cause diagnostics regressions whenever a type appears within its own generic signature, because we now have to compute the generic signature just to form the declared interface type. Having discussed the latter with Slava, one idea we settled on was to define a StructuralBoundGenericType counterpart that stores an array of generic arguments -- just like we do today -- for use during the type-checking stage only. More importantly, this would ultimately enable us to unlock recursive generic signature validation altogether.

There are still a few tests failing, in particular, I haven't figured yet how to deal with bound generic types with unbound generic parents. Representing unbound generic parameters as T -> Type() in the substitution map doesn't work right off the bat.

@slavapestov I would greatly appreciate any feedback, especially on the performance-critical parts.

@AnthonyLatsis
Copy link
Collaborator Author

Moving out of draft status to be able to track conflicts.

@AnthonyLatsis AnthonyLatsis marked this pull request as ready for review May 19, 2020 20:14
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that getContextSubstitutionMap() now becomes completely trivial. It can get the SubstitutionMap out of the type, instead of re-deriving it.

args.push_back(param->getDeclaredInterfaceType());

return BoundGenericType::get(decl, ParentTy, args);
const auto sig = decl->getGenericSignature();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means calling getDeclaredInterfaceType() on a type will now compute it's generic signature. This may or may not be a problem. In any case, the above comment is out of date (it talks about "validation" which is no longer a thing).

assert(!resultType->hasTypeParameter());
return resultType;
const auto resultTy = decl->getDeclaredInterfaceType();
assert(!resultTy->hasTypeParameter());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion will fail if decl is generic

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what this is trying to accomplish by ignoring the arguments. This is the code path for Foo.Alias<...>, where Foo is an unbound generic parent. I will see if I can reproduce this assertion failure once I get the Standard Library to build once again.

@slavapestov
Copy link
Contributor

As for UnboundGenericType, I kind of wish it didn't exist at all. Instead, resolveType() should take some callback or other mechanism for filling in missing generic arguments. In expression context, we would move the logic from openUnboundGenericType() here; in type context, it would either take the arguments from context if you refer to a type inside its own body, or just diagnose.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented May 20, 2020

Note that getContextSubstitutionMap() now becomes completely trivial. It can get the SubstitutionMap out of the type, instead of re-deriving it.

getContextSubstitutionMap depends on the given declaration context, so I think we would still need to derive new maps for the protocol context case [Self -> baseTy] and when the declaration context is a constrained extension.

struct Foo<T, U> {}
extension Foo where T == Bool {}

For example, asking for the substitutions of Foo<Int, String> in the context of the extension will produce [T -> Bool, U -> String] AFAICT.

Edit: Forgot to mention the superclass context case.

@slavapestov
Copy link
Contributor

@AnthonyLatsis You're right about getContextSubstitutionMap(). However we should fast-path it in the case where the dc is the same as the nominal.

@AnthonyLatsis
Copy link
Collaborator Author

Trying to figure out what's causing the following assertion failure:

Looking for a function: $sSNsSxRzSZ6StrideRpzrlEyxSNsSxRzSZABRQrlE5IndexOyx_GcirSi_Tg5
Expected type: @yield_once @convention(method) (ClosedRange<Int>.Index, ClosedRange<Int>) -> @yields @in_guaranteed Int
Found    type: @yield_once @convention(method) (ClosedRange<Int>.Index, ClosedRange<Int>) -> @yields @in_guaranteed Int
Assertion failed: (ReInfo.getSpecializedType() == SpecializedF->getLoweredFunctionType() && "Previously specialized function does not match expected type.")

@slavapestov
Copy link
Contributor

It looks like you've got two BoundGenericTypes that print the same, but are not equal, because something is different with their substitution maps.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented May 21, 2020

Okay, here's what's going on.

createPrespecializations is being passed an apply site of the function

@convention(method) <τ_0_0 where τ_0_0 : Comparable> (@in_guaranteed ClosedRange<τ_0_0>) -> ()

with the semantic attribute @_semantics("prespecialize.X"), where X is the function

@yield_once @convention(method) <τ_0_0 where τ_0_0 : Strideable, τ_0_0.Stride : SignedInteger> (@in_guaranteed ClosedRange<τ_0_0>.Index, @in_guaranteed ClosedRange<τ_0_0>) -> @yields @in_guaranteed τ_0_0,

and attempts to specialize the latter using the substitutions from the apply site. Obviously, those substitutions are inadequate, because we need the conformance to Strideable, not just Comparable, for the specialized function to receive pattern substitutions without invalid conformances.

@slavapestov I am not familiar with the expectations about functions referenced in @_semantics. Should we map the substitutions accordingly, fail to specialize or is this specific attribute declaration simply flawed?

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented May 21, 2020

To make this less cryptic, here is the method that requires prespecializing subscript.read (the X mentioned earlier).

https://github.com/apple/swift/blob/417e1737bfb2e5402e45d953d7afff0e866561a6/stdlib/public/SwiftOnoneSupport/SwiftOnoneSupport.swift#L276-L285

subscript.read requires Bound: Strideable, Bound.Stride: SignedInteger, so it seems to me this method was put in the wrong extension. I am going to try moving it to the successive extension which has the right constraints.

@AnthonyLatsis AnthonyLatsis force-pushed the substitution-revolution branch from 412efb6 to b0d26cb Compare May 22, 2020 01:43
@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented May 22, 2020

Going to focus on cutting off UnboundGenericType usage next (this will not work properly as is anyway).

@slavapestov Let me know whether that SwiftOnoneSupport.swift tweak looks sane enough to cherry-pick.

@AnthonyLatsis AnthonyLatsis force-pushed the substitution-revolution branch from b0d26cb to 0d72b81 Compare July 3, 2020 16:08
@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@AnthonyLatsis AnthonyLatsis changed the base branch from master to main October 1, 2020 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants