art-divin
commented
5 months ago Thank you for the investigation and PR, @fabianmuecke 🤝
Closed fabianmuecke closed 5 months ago
art-divin
commented
5 months ago Thank you for the investigation and PR, @fabianmuecke 🤝
fabianmuecke
commented
5 months ago Just to keep you posted on this, @art-divin. I just identified another issue, which is probably related and does not get fixed by this PR. We're generating some mock stuff based on generic parameters and the typealias resolution / replacement, which already broke the thing described above also seems to be responsible for this:
We have a typealias named Value in some place of our codebase.
We have a function in a protocol like this:
func foo<Value>(bar: Foo<Value>)
typeName of parameters.first is Foo<Value> as expected, while typeName.generic is Foo<Element>, because our totally unrelated typealias of Value is pointed to Element. I could track this down to that forEach block after line 457 in ParserResultsComposed.swift, where the type parameter's typeName is replaced by the typeName.actualTypeName. Right now I'm at a loss how to fix this without breaking the typealias resolution you introduced in that PR linked above, though.
art-divin
commented
5 months ago Hey @fabianmuecke ;
thank you very much for your message.
I wonder, does it make sense to merge this PR because it is backed up by a test, and proceed with a fix for the second issue you found?
Because CI checks are green, I see a benefit of having an additional test.
Regarding the second issue, Would you be so kind creating an issue with a minimal reproducible with "given vs. expected" input/output?
That would be 50% of the bug fix, a good minimal reproducible.
Knowing you have already provided a glympse of it here, it is a bit of bureaucracy, yes.
fabianmuecke
commented
5 months ago Hey @fabianmuecke ;
thank you very much for your message.
I wonder, does it make sense to merge this PR because it is backed up by a test, and proceed with a fix for the second issue you found?
Because CI checks are green, I see a benefit of having an additional test.
Regarding the second issue, Would you be so kind creating an issue with a minimal reproducible with "given vs. expected" input/output?
That would be 50% of the bug fix, a good minimal reproducible.
Knowing you have already provided a glympse of it here, it is a bit of bureaucracy, yes.
Thanks for your reply. I guess you're right. I'll set it to RFR again. I'll create an issue for the second issue as soon as I managed to have a good minimal reproducible. 🙏
This PR fixes a regression introduced by https://github.com/krzysztofzablocki/Sourcery/pull/1288. If a type name contained the name of a typealias, part of the type name would be replaced with the resolved typealias, which ultimately led to the lookup of a wrong (in our case non-existent) type.
I simply added an early return, if the exact type name could be found and added a test case to make sure, that both cases work as expected now.
Please let me know, if there is anything I can do, to speed up getting this merged, as this regression has been keeping us from upgrading to the latest Sourcery version for a while (didn't get to having a closer look sooner).