Add generics to types #660

truizlop · 2018-08-08T14:44:37Z

As stated in issue #551, current type declaration only provides a boolean flag stating whether a type has a generic declaration or not. This PR parses the list of generic types associated to a type declaration as an Array<String>. They can be accessed during generation through the property type.genericTypes. Default value (when type is not generic) is an empty array.

SourceryBot · 2018-08-08T14:52:03Z

	1 Error
🚫	Any changes to library code need a summary in the Changelog.

Generated by 🚫 Danger

krzysztofzablocki · 2018-08-08T15:03:36Z

@truizlop looks good, could you add changelog info?

ilyapuchka · 2018-08-08T15:09:19Z

@truizlop @krzysztofzablocki I don't feel like this is an extensible solution though. How are we going to extend it to support generic constraints, i.e.? I think we should provide full information about generic parameters like we do for variable/parameters types. And I feel it will make sence to implement it so that we can parse generic parameters of both type and methods declarations, because the syntax is the same.

marcelofabri · 2018-08-08T15:32:09Z

BTW SourceKit in Swift 4.1 (or 4.2 not so sure) provides some of that information

ilyapuchka · 2018-08-08T15:36:26Z

@marcelofabri It's probably only for 4.2, I quickly checked on Swift 4.2 with SourceKitten 0.20 and there is nothing helpful

marcelofabri · 2018-08-08T15:41:49Z

Yeah, just double checked and it's for 4.2:

struct Foo<T: Equatable> {}

{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.length" : 27,
  "key.offset" : 0,
  "key.substructure" : [
    {
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.bodylength" : 0,
      "key.bodyoffset" : 26,
      "key.kind" : "source.lang.swift.decl.struct",
      "key.length" : 27,
      "key.name" : "Foo",
      "key.namelength" : 3,
      "key.nameoffset" : 7,
      "key.offset" : 0,
      "key.substructure" : [
        {
          "key.elements" : [
            {
              "key.kind" : "source.lang.swift.structure.elem.typeref",
              "key.length" : 9,
              "key.offset" : 14
            }
          ],
          "key.inheritedtypes" : [
            {
              "key.name" : "Equatable"
            }
          ],
          "key.kind" : "source.lang.swift.decl.generic_type_param",
          "key.length" : 12,
          "key.name" : "T",
          "key.namelength" : 1,
          "key.nameoffset" : 11,
          "key.offset" : 11
        }
      ]
    }
  ]
}

It doesn't provide any information about constraints added using where though.

krzysztofzablocki · 2018-08-08T15:46:59Z

@ilyapuchka fair point, @truizlop would you be able to add support for that?

truizlop · 2018-08-09T07:36:37Z

Yes, I can take care of that. Is there any class that you think I can reuse for this task, or should I create a new one? I am not 100% familiar with the project, so perhaps you can advise in this regard. Also, should we include where constraints as well? How should constraints be represented? They may be protocols or superclasses, but we may not have enough information to infer what they are.

SourceryBot · 2018-08-09T11:50:30Z

SourceryRuntime/Sources/Generic.swift

@@ -0,0 +1,25 @@
+import Foundation
+
+@objcMembers public final class Generic: NSObject, SourceryModel {


🚫 Undocumented symbol.

truizlop · 2018-08-09T11:54:21Z

@ilyapuchka @krzysztofzablocki @marcelofabri please take a look at the new changes I introduced. These changes are able to parse constraints on generics on types and methods, and also in where clauses. Before going any further, I'd like your opininon on it.

There is still one case that I am not sure how to handle. When there is a constraint on a protocol with an associated type, how should we model it?

ilyapuchka · 2018-08-13T09:13:09Z

Sourcery/Parsing/FileParser.swift

+            var openingAngles = 1
+            while openingAngles > 0 && end != source.endIndex {
+                end = source.index(after: end)
+                if source[end] == ">" {


is it really possible to have nested generic blocks?

Yes, if they appear on the constraints. Something like: class Foo<A, T : Bar<A>> is syntactically correct.

ilyapuchka · 2018-08-13T09:15:29Z

Sourcery/Parsing/FileParser.swift

+            declaration = String(declaration[declaration.startIndex ..< whereStart])
+        }
+
+        if let start = source.index(of: "<") {


this seems to work on class MyClass: SuperClass<Generic> when it shouldn't, so we should check for inheritance part and drop it first.

ilyapuchka · 2018-08-13T09:17:27Z

SourceryRuntime/Sources/Class.swift

@@ -24,7 +24,8 @@ import Foundation
                         typealiases: [Typealias] = [],
                         attributes: [String: Attribute] = [:],
                         annotations: [String: NSObject] = [:],
-                         isGeneric: Bool = false) {
+                         isGeneric: Bool = false,
+                         genericTypes: [Generic] = []) {


I think genericTypeParameters is a name that is more aligned with Swift terminology, same applies to type of elements, GenericTypeParameter

ilyapuchka · 2018-08-13T09:20:17Z

SourceryTests/Parsing/FileParserSpec.swift

+
+                    it("extracts generics correctly") {
+                        let result = parse("""
+class Foo<A : Equatable & Codable, B : Baz<A>,C> : Bar<B> where C : CustomStringConvertible, B : Comparable & Decodable, C : CustomDebugStringConvertible {


would be interesting to add some line breaks in where clause to make sure it's parsed correctly from multiple lines

ilyapuchka · 2018-08-13T09:21:55Z

SourceryRuntime/Sources/Generic.swift

+
+@objcMembers public final class Generic: NSObject, SourceryModel {
+    public let name: String
+    public var constraints: [TypeName]


we should also infer type of the constraint, when it is known, so this should be probably

class GenericTypeParameterConstraint { let typeName: TypeName let type: Type? }

I don't know how you are dealing with this in the rest of the project. Can you provide some hints on how to infer the type?

ilyapuchka · 2018-08-13T09:28:39Z

Sourcery/Parsing/FileParser.swift

+
+    fileprivate func parseGenerics(declaration: String, whereClause: String) -> [Generic] {
+        func extractGenerics(source: String) -> [Generic] {
+            return source.components(separatedBy: ",")


we should use commaSeparated method, it will ensure that separators are processed correctly, otherwise things like Foo<A: [(String, String)]> will be parsed incorrectly

ilyapuchka · 2018-08-13T09:34:21Z

Sourcery/Parsing/FileParser.swift

+            if let generic = filtered.first {
+                return generic
+            }
+            fatalError("Undeclared generic found: \(name)")


should be Log.error() instead

ilyapuchka · 2018-08-13T09:36:52Z

Sourcery/Parsing/FileParser.swift

+        var genericTypes: [Generic] = []
+        if let genericSource = extract(.name, from: source),
+            let parameterStartIndex = genericSource.index(of: "("),
+            let whereClauseSource = extract(.nameSuffixUpToBody, from: source) {


this might not work with protocols where methods don't have body. we should probably extract it along with extracting return type (above), which handles this case

SourceryBot · 2018-08-27T14:37:51Z

SourceryRuntime/Sources/GenericTypeParameterConstraint.swift

+
+/// Represents an inheritance/implementation constraint in a generic type parameter
+@objcMembers public final class GenericTypeParameterConstraint: NSObject, SourceryModel {
+    public let name: TypeName


🚫 Undocumented symbol.

SourceryBot · 2018-08-27T14:37:52Z

SourceryRuntime/Sources/GenericTypeParameterConstraint.swift

+/// Represents an inheritance/implementation constraint in a generic type parameter
+@objcMembers public final class GenericTypeParameterConstraint: NSObject, SourceryModel {
+    public let name: TypeName
+    public let type: Type?


🚫 Undocumented symbol.

truizlop · 2018-08-27T14:46:46Z

@ilyapuchka I have addressed all the comments in your review, except for the type inference, which right now I am not sure how to do. Any guidance would be appreciated.

truizlop · 2018-09-05T07:43:21Z

@ilyapuchka @krzysztofzablocki could anyone take a look at the most recent changes?

ilyapuchka · 2018-09-07T19:51:08Z

@truizlop I'll try to find time to look at that this weekend. For inference look at Composer.swift, it already has all the methods you need to use to resolve type references. You'll probably need to add new one, similar to one of resolve...Types

DenTelezhkin · 2018-11-07T12:59:37Z

Hello guys!

Hitting the same wall, writing templates that depend on the type of generic argument of type is currently what I want with my code generation, but cannot do because generic arguments are not exposed, only the fact that they are generic.

Is there something I can do to help move this PR forward?

truizlop · 2018-11-07T13:02:21Z

I'd love to move it forward as well, as this has been sitting here for a very long time. I was waiting for a review, and I know that we need to add support for the inference of types, which was not added yet. Perhaps you can take a look at that?

ilyapuchka · 2018-11-07T13:07:45Z

SourceryTests/Parsing/FileParserSpec.swift

+                    it("doesn't extract generics if they are concrete parameters to the supertype") {
+                        let result = parse("class Foo : Bar<Int> {}")
+                        let expected = Class(name: "Foo", isGeneric: false)
+                        expected.inheritedTypes = ["Bar<Int>"]


ideally the generic type information should be still available in this case. In the end if Boo is defined as Boo<Value>, then Foo.Value can be used in the Foo implementation. And it should have a concrete type information too.

ilyapuchka · 2018-11-07T13:08:38Z

SourceryTests/Parsing/FileParserSpec.swift

+    }
+}
+""")
+                        let expected = Class(name: "Foo", isGeneric: true, genericTypeParameters: [GenericTypeParameter(typeName: TypeName("A"), constraints: [GenericTypeParameterConstraint(name: TypeName("Equatable")), GenericTypeParameterConstraint(name: TypeName("Codable"))]), GenericTypeParameter(typeName: TypeName("B"), constraints: [GenericTypeParameterConstraint(name: TypeName("Baz<A>")), GenericTypeParameterConstraint(name: TypeName("Comparable")), GenericTypeParameterConstraint(name: TypeName("Decodable"))]), GenericTypeParameter(typeName: TypeName("C"), constraints: [GenericTypeParameterConstraint(name: TypeName("CustomStringConvertible")), GenericTypeParameterConstraint(name: TypeName("CustomDebugStringConvertible"))])])


some code formatting is probably needed here to make it more readable

DenTelezhkin · 2018-11-07T22:57:35Z

Ok, guys, so I've managed to make some progress on this task here in my fork - https://github.com/DenHeadless/Sourcery/tree/trl/add-generics-to-types

Stuff that was made:

Rebase on master to get project running with Xcode 10
Improve some text formatting in unit tests(Specifically FileParserSpec.swift)
Implement getting inheritedTypes property as [Type] instead of [String], so that generic information is retrievable(at least partially)

Unresolved issues

I was not able to regenerate AutoJSExport files for some reason - I always get this error - Problem with JSExport.ejs #438
Some unit tests fail(14 of them), 13 related to templates.
One test that is not related to templates, but I currently have hard time understanding why it's failing - https://github.com/DenHeadless/Sourcery/blob/trl/add-generics-to-types/SourceryTests/Parsing/FileParserSpec.swift#L632-L637
I'm very new to this project and could and probably did use codebase inefficiently, so this would require review and probably some changes.

@ilyapuchka @truizlop Hope for your help in this one, should I open another PR that subsumes this one, or lets somehow pull my changes into this PR?

truizlop · 2018-11-08T09:47:53Z

Given the discussion is already here, it would be nice if you can merge your branch with mine and we'll continue here. I can try to take care of the broken tests (probably there has been some change since last time I checked it out). After that, we can decide how to move it forward.

DenTelezhkin · 2018-11-08T10:42:00Z

@truizlop makes sense. I think the simplest way would be for you to checkout my branch and force push changes from it into your branch, replacing current commits with rebased and new ones.

_{Sent with GitHawk}

DenTelezhkin · 2018-11-13T17:50:35Z

Small update: I was able to resolve almost all issues I mentioned in previous post. I am continuing to add tests and improving generic parsing system to be recursive, and handle all cases from simple to more complex, for example A<B<C: D>. Things are looking good, but need more time to finish with all this stuff, as current workload on my day-job is pretty high at the moment.

Hope to get more updates soon.

DenTelezhkin · 2018-12-06T19:00:59Z

Hello guys, sorry it took so long, generic handling turned out to be much harder task than initially anticipated. I also diverged from this PR and current code base quite a bit, so I opened a new PR to clearly view and discuss all changes - #707.

Would love to hear your thoughts and feedback about it @truizlop @ilyapuchka.

truizlop · 2018-12-07T09:18:27Z

Thanks @DenHeadless, I'll take a look at your PR.

truizlop added 3 commits August 8, 2018 16:38

Extend Type to have generic types

0356ef5

Add parsing method to retrieve generic types of a declaration

d60a34c

Test parsing and generation of generic types

a6e7707

krzysztofzablocki force-pushed the master branch 2 times, most recently from 7b0a222 to fc7ebe8 Compare August 9, 2018 10:54

truizlop added 6 commits August 9, 2018 13:41

Add class to contain generic declaration with constraints

d43f181

Autogeneration of protocol implementations for Generic

29f9d24

Use new type to contain generic information in types and methods

5a6f521

Add utilities to work with String indices

87aabe9

Parse generic declarations

8d8d463

Test generic parsing and generation

098749a

SourceryBot reviewed Aug 9, 2018

View reviewed changes

Add forgotten data to test

88c8afe

ilyapuchka reviewed Aug 13, 2018

View reviewed changes

truizlop added 9 commits August 27, 2018 13:23

Add line breaks to test parsing robustness

9399814

Replace fatalError by Log.error()

e788c3c

Check corner case parsing generics only on the supertype

f359331

Check corner case parsing generics with tuples

f50e7ae

Check corner case for where clauses in protocol methods with no impl

f1f93d2

Introduce GenericTypeParameterConstraint class

2368336

Rename to use "genericTypeParameters"

b5bc12e

Use GenericTypeParameter class to describe generic parameters in types

f28503b

Add missing documentation

e514ef4

SourceryBot reviewed Aug 27, 2018

View reviewed changes

ilyapuchka reviewed Nov 7, 2018

View reviewed changes

DenTelezhkin mentioned this pull request Dec 6, 2018

[WIP] Introduce new generic parsing system #707

Closed

Liquidsoul force-pushed the master branch from c74746d to fb8ac81 Compare March 5, 2019 09:40

truizlop closed this Apr 8, 2020

truizlop deleted the trl/add-generics-to-types branch April 8, 2020 07:20

		@@ -0,0 +1,25 @@
		import Foundation

		@objcMembers public final class Generic: NSObject, SourceryModel {

Add generics to types #660

Add generics to types #660

Conversation

truizlop commented Aug 8, 2018

SourceryBot commented Aug 8, 2018

krzysztofzablocki commented Aug 8, 2018

ilyapuchka commented Aug 8, 2018

marcelofabri commented Aug 8, 2018

ilyapuchka commented Aug 8, 2018

marcelofabri commented Aug 8, 2018

krzysztofzablocki commented Aug 8, 2018

truizlop commented Aug 9, 2018

SourceryBot Aug 9, 2018 • edited

Choose a reason for hiding this comment

truizlop commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka Aug 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

truizlop commented Aug 27, 2018

truizlop commented Sep 5, 2018

ilyapuchka commented Sep 7, 2018 • edited

DenTelezhkin commented Nov 7, 2018

truizlop commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenTelezhkin commented Nov 7, 2018

truizlop commented Nov 8, 2018

DenTelezhkin commented Nov 8, 2018

DenTelezhkin commented Nov 13, 2018

DenTelezhkin commented Dec 6, 2018

truizlop commented Dec 7, 2018

SourceryBot Aug 9, 2018 •

edited

ilyapuchka Aug 13, 2018 •

edited

ilyapuchka commented Sep 7, 2018 •

edited