Skip to content
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

[SE] Allow where clauses on non-generic declarations in generic contexts #23489

Merged

Conversation

AnthonyLatsis
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis commented Mar 22, 2019

SE-0267

struct Foo<T> {
  func foo() where T: Equatable {}
}

I'm not happy with using pointer unions, but couldn't come up with a better idea that wouldn't require further unrelated changes of uncertain scale; there will be a lot of opportunities to get rid of this later through making where clauses less dependent on parameter lists.


  • Diagnose where on non-generic top-level declarations in Parse
  • Provide syntax highlighting for contextual where clauses
  • Lift the restriction on contextual where clauses
  • Make sure subscript accessors pick this up as well
  • SILGen: Test that declarations with contextual where clauses are mangled correctly
  • Test that we can demangle nested type declarations with contextual where clauses
  • Add execution tests
  • Test overrides involving contextual where clauses
  • Tweak checkConstrainedExtensionRequirements to look out for contextual where clauses as well

@AnthonyLatsis
Copy link
Collaborator Author

cc @slavapestov Mind having a look?

P.S. I am aware this most likely needs an evolution proposal

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.

I only skimmed this but it looks great.

The next step I think is to write some SILGen, IRGen and execution tests. And yeah, an evolution proposal would be nice. I don't expect much controversy since this is lifting an "obvious" restriction.

lib/Sema/TypeCheckGeneric.cpp Outdated Show resolved Hide resolved
@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Mar 27, 2019

Thanks for taking a look!
I haven't worked with raw SIL before, so the only thing that comes to mind is to see what happens to something in a constrained extension and CHECK for the same thing to happen without the extension. I will read through anything I can find in the docs folder about SIL before asking any questions, and get a proposal ready for discussion in the meantime.

@slavapestov
Copy link
Contributor

For SILGen tests, I think it’s enough to just have some CHECK lines to verify that the lowered generic signature looks correct.

@AnthonyLatsis
Copy link
Collaborator Author

For SILGen tests, I think it’s enough to just have some CHECK lines to verify that the lowered generic signature looks correct.

I will try. It should be identical to one in a constrained extension, correct?

@slavapestov
Copy link
Contributor

@AnthonyLatsis Yes, the generic signature should be identical to a constrained extension. The mangling should be slightly different though (and please test declarations that only differ in where clause; we should mangle them uniquely).

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from c35ea0f to 71521d5 Compare March 28, 2019 12:40
@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Mar 28, 2019

Added SILGen tests. I also noticed that a nil literal in a subscript getter is hitting a null type assert. It only happens if type checking succeeds elsewhere:
./bin/swiftc -typecheck /Users/polzovatel/Desktop/test.swift

protocol P { associatedtype Assoc }
class Class<T> {
  subscript(arg: Int) -> T.Assoc? where T: P {
    return nil
  }
}

expression has no type (nil_literal_expr type='<null>' initializer=**NULL**)

Almost sure this is specific to subscript accessors and the way they currently inherit the generic signature of the subscript. Once fixed, what is the preferred way to test against this: simply add the subscript to the silgen tests or create a separate "fixed crasher"?

@slavapestov
Copy link
Contributor

@AnthonyLatsis a SILGen test is probably your best bet for tests that only fail when no errors are emitted. We should fix the AST verifier one day to be smarter about this...

@AnthonyLatsis
Copy link
Collaborator Author

@slavapestov, regarding IRGen, can you give a rough idea of what to test for? Thanks.

@slavapestov
Copy link
Contributor

@AnthonyLatsis testing in IRGen probably won't get you much coverage because at that stage a generic type member with a where clause is not much different from a top level function with a 'flat' generic signature.

However, one interesting thing to test would be the runtime's mangled name to metadata recovery. Take a look at test/Runtime/demangleToMetadata.swift. The idea would be to define a non-generic type with a where clause nested inside a generic type and see if you can demangle it.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Apr 5, 2019

@slavapestov I am having a hard time guessing the mangled names. By picking a specific example and comparing the output from -emit-silgen with the respective test in demangleToMetadata.swift, I can see that for the most part the mangled names match, but there is still a prefix and suffix that I can't predict. What is the proper way to obtain the name I need for a demangle test?

EDIT
Never mind.

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from e623829 to b21244f Compare April 7, 2019 02:46
@AnthonyLatsis AnthonyLatsis changed the title [Parse][Sema] Allow where clauses on nongeneric declarations in generic contexts [Parse, Sema] Allow where clauses on nongeneric declarations in generic contexts Apr 7, 2019
@AnthonyLatsis
Copy link
Collaborator Author

@slavapestov I think this is ready for a smoke test.

P.S. What do you think of the error messages?

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor

@swift-ci Please test source compatibility

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from 60e9b6b to d4c9e02 Compare April 9, 2019 22:23
@AnthonyLatsis
Copy link
Collaborator Author

Let's try again?

@slavapestov
Copy link
Contributor

@swift-ci Please test source compatibility

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor

@AnthonyLatsis Can you try to prepare a new PR to merge as much of this as possible without the language change/proposal? In particular I'm interested in the TypeCheckGeneric.cpp cleanups to unify the function/subscript code paths. We're about to start reworking that code for the request evaluator and I'd have to either lose the cleanup or make you redo your work.

@slavapestov slavapestov self-assigned this Apr 18, 2019
@AnthonyLatsis
Copy link
Collaborator Author

Sure. I'll cherry-pick the cleanups onto a new branch and open a PR. Let's hope we can land them without merge conflicts.

@slavapestov
Copy link
Contributor

@AnthonyLatsis Thanks! Also of course I hope the overall change is accepted too! But I know it can take a while to go through the process.

@AnthonyLatsis AnthonyLatsis changed the title [Parse, Sema] Allow where clauses on nongeneric declarations in generic contexts [Parse, Sema, SE] Allow where clauses on non-generic declarations in generic contexts Apr 21, 2019
@AnthonyLatsis AnthonyLatsis changed the title [Parse, Sema, SE] Allow where clauses on non-generic declarations in generic contexts [SE] Allow where clauses on non-generic declarations in generic contexts Sep 22, 2019
@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from d4c9e02 to 0f29427 Compare September 22, 2019 22:57
@AnthonyLatsis
Copy link
Collaborator Author

@slavapestov Wrapped this up. Let me know what you think.

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from 0df2ea0 to 9f42953 Compare March 5, 2020 04:47
@AnthonyLatsis
Copy link
Collaborator Author

Let's land this as soon as the merge conflict is fixed though (and tests pass).

Sure, it should be ready now. All the comments I have addressed so far are marked as resolved (folded).

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from 9f42953 to 5bfb0a9 Compare March 5, 2020 05:04
@theblixguy
Copy link
Collaborator

@swift-ci please test

@theblixguy
Copy link
Collaborator

@swift-ci please test source compatibility

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@AnthonyLatsis AnthonyLatsis force-pushed the where-clause-nongeneric-decl branch from 5bfb0a9 to f762644 Compare March 5, 2020 17:30
@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Mar 5, 2020

Ouch, the crasher from #27987. That is a separate circularity issue I thought I had fixed here, but forgot to remove.

@AnthonyLatsis
Copy link
Collaborator Author

@theblixguy Could you trigger the tests for me once more please?

@theblixguy
Copy link
Collaborator

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@slavapestov
Copy link
Contributor

Let's merge this. Can you take a look at my other comments that you didn't address yet though?

  • Investigate moving diagnosing the where clause
  • Add SILGen tests to test/SILGen/vtable_thunks_reabstraction.swift
  • Add type reconstruction tests to test/TypeDecoder/

@slavapestov
Copy link
Contributor

Alright, let's merge this!

@slavapestov slavapestov merged commit adbf8da into swiftlang:master Mar 6, 2020
@AnthonyLatsis
Copy link
Collaborator Author

Can you take a look at my other comments that you didn't address yet though?

Sure thing, they are on my mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants