Bug in "type.implements"?

[bolismauro]

I'm trying the feature implemented in #42 (as requested in #41)

This is my code

struct T1: Equatable {
  static func == (l: T1, r: T1) -> Bool {
    return true
  }
}

struct T: AutoEquatable {
  let a: Int
}

struct T2: AutoEquatable {
  let t: T
  let t1: T1
}

with this condition, the T1 struct is ignored

{% if variable.type.implements.AutoEquatable or variable.type.implements.Equatable %}

while with this one, it works:

{% if variable.type.implements.AutoEquatable or variable.type.inheritedTypes.Equatable %}

If I print the types, inheritedTypes values have the expected content (so AutoEquatable for T and T2, and Equatable for T1). It seems a bug in the implements "function".

If this is the desired behaviour, I think it is confusing (right now, maybe there is a good reason I don't see :) )

[bolismauro]

By playing a little bit with the Sourcery code I discovered that the T1 definition doesn't contain the Equatable protocol in the implements dictionary

Struct: isExtension = false, kind = struct, accessLevel = internal, name = T1, isGeneric = false, localName = T1, variables = [], annotations = [:], staticVariables = [], computedVariables = [], storedVariables = [], inheritedTypes = ["Equatable"], containedTypes = [], parentName = nil, implements = [:], 

(see last variable)

Maybe Equatable is not a known protocol (meaning that the declaration is not in the project but it is in the Apple framework) and therefore is not added?

EDIT: after some other investigation I discovered that updateTypeRelationship is never invoked for T1. Also, If I remove the AutoEquatable declaration, also the implements.AutoEquatable check stops to work.
So I guess that, like in #40, it doesn't work because Equatable is not known.

The weird thing though is that the behaviour is different in implements and inheritedTypes. I guess it should be unified somehow.

[ilyapuchka]

Here is how these collections work:

• type.based.ClassOrProtocol - contains names of all known and unknown types, flattening hierarchy of known types. If you need to deal both with stdlib types and your own types - use this to make temaplate easier to read. This is analogous to types.based.ClassOrProtocol
• type.inherits.Class - contains names of all known classes which given class inherits from, flattening hierarchy of known classes. Enums and structs can not inherit in Swift, so for them that collection will be always empty. This is a convenience accessor that should be used only when dealing with known classes. This is analogous to types.inheriting.Class
• types.implements.Protocol - contains names of all known protocols which given type implements, flattening hierarchy of known protocols. This is a convenience accessor that should be used only when you are dealing with your own protocols. This is analogous to types.implementing.Protocol
• type.inheritedTypes - this is a utility collection of types names that given class derives from directly, you are not able to use it in templates for anything useful (it should be removed from docs, I guess). Use based instead.

@krzysztofzablocki to be honest I'm starting to think that my original proposal to distinguish inherited/implementing types was not that good and creates confusion, looks like having just one based collection would be enough

[bolismauro]

If I can give my opinion, it is very very hard to understand from the user point of view (*). This division seems more related to Sourcery internals than how people (but here I have just one example, myself) would think about it.

I think that it would be amazing to just have implements.Protocol and inherits.Class and then flatten everything. In this way you don't have to think about the two cases (know/unknown) every time. You just use them.

If a protocol/class is not known (that is, you can't know which types are carried by that protocol/class) then of course you won't have some information, but I guess here there is very little Sourcery can do without adding Apple frameworks metadata.

(*) Maybe it is just because the lack of examples/documentation. I'm thinking how to improve it. if I find a good idea, I will send a PR

[krzysztofzablocki]

there is one bug when it doesn't properly propagate protocols when one protocol implements another, but I've already fixed it, will push it when I get better network reception (on train atm).

@ilyapuchka I had inherited, implementing already and symmetry here is in order so we shouldn't change it. I'm removing inheritedTypes from docs as that's a good call

@bolismauro if you don't care whether is known or not, then you can use based for both and it flattens them already, inherits and implements are there to allow people to use only fully known types to make it easier. I hope that makes sense?

[krzysztofzablocki]

Actually since structs and enums can't inherit we can infer that the inheritedTypes are in fact protocols and we could create their definitions internally, what do you guys think about this? @bolismauro ?

@ilyapuchka do you think if I added that it would made sense? or would it break the idea of 'Known types'

[ilyapuchka]

I also started to think about creating Type for unknown types, but then it will only work for value types, protocols and extensions, but not for classes. Given just this class MyClass: Equatable {} we can not know either Equatable is a class we are inheriting from or we are creating a root class that conforms to protocols. It's even worse for variables that can be of unknown class/struct/enum/protocol. How do we know there? So I gave up on idea of doing that solely pretty quickly 😀 We can of course try to check if any value type,protocol or extension is based on that unknown type, but it sounds like too much for a profit (if there is actually any, except consistency with language itself) of distinguishing protocols from classes. And it might not even help in the end.

At this point I still would say that the best way to avoid confusion here is to keep only one collection of types, name it based or what ever. Then we don't need to care about the kind of that unknown type and then we can easily create a dummy Type for it (maybe returning empty string in a kind property...). Maybe also adding isKnown property.
Or we keep it like it is explaining the difference in the docs with examples.

[krzysztofzablocki]

Yeah that's my thought it's going to cause more confusion, I'd leave the implementation as it is, and add examples to docs, in this task using based would make it work right? in that case we should close it, I've one bug that I fixed that will land today

[ilyapuchka]

Yes, using based or consistently using AutoEquatable instead of Equatable should work here.

[krzysztofzablocki]

ok closing then, hope our reasoning makes sense to you @bolismauro, but let us know if anything is unclear, I should have a fix for protocol implementing other protocols in ~2h when I get home

[bolismauro]

if you don't care whether is known or not, then you can use based for both and it flattens them already, inherits and implements are there to allow people to use only fully known types to make it easier. I hope that makes sense?
Yes it does. As I said, maybe is just a matter of documentation/understand why it is done in this way.

At this point I still would say that the best way to avoid confusion here is to keep only one collection of types, name it based or what ever. Then we don't need to care about the kind of that unknown type and then we can easily create a dummy Type for it (maybe returning empty string in a kind property...). Maybe also adding isKnown property.

I was thinking something similar. Something like

variable.type.based.MyProtocol.isKnown
variable.type.based.MyProtocol.name // empty for unknown types

But I trust you if you think that it will cause more confusion. I have tried just 2/3 use cases and maybe the changes I'm suggesting make sense only in very few cases :)

Thanks again!

[krzysztofzablocki]

Let's keep it simple for now and see if we find more use-cases, if we saw a class Foo :Equatable then if variable.type.based.Foo.based.Equatable should already work

[ilyapuchka]

if variable.type.based.Foo.based.Equatable 😉