stencilproject / Stencil

Stencil is a simple and powerful template language for Swift.
https://stencil.fuller.li
BSD 2-Clause "Simplified" License
2.33k stars 221 forks source link

Fixed nil mirrored values detection #269

Closed trupin closed 5 years ago

trupin commented 5 years ago

Hello,

I ran into this strange bug while I was working on Weaver.

One of the attributes set to nil is interpreted as none by Stencil. After debugging into Stencil, I found out that the problem came from the description of the attribute value which is in my particular case none rather than nil. When printing result, the debugger shows nil though.

I'm not sure what is the rule behind that, and I actually haven't been able to write a test that reproduces the issue, but I made a branch on Weaver's repo which reproduces the bug every-time. To test, just checkout that branch and open Weaver.xcodeproj, then run the target WeaverCommand with the debugger on, and you should get the same result as the following screenshot.

screen shot 2019-02-10 at 5 20 41 pm

I think that, even though I couldn't reproduce the bug in a unit test, I have a proof that String(describing: result) can take the value "none" and thus that it should be checked. But of course, if anyone has an explanation for this behavior, I'm open to better suggestions.

AliSoftware commented 5 years ago

I'm confused, if String(describing: value) returns none that means that value isn't nil (that case would have been caught by the if result == nil above anyway) but is actually "none". That could come for a lot of reasons unrelated to a value having to be interpreted as nil:

I'm not sure why we should compare to "none"then, I mean, didn't look at your branch (on my phone rn) but I'm wondering if the issue doesn't come from some conversion done ahead of this in Weaver, where you convert some of your nil values to the "none" string, so when you pass them to Stencil it's already converted to the "none" string anyway? And in that case it would make more sense to avoid that conversion in Weaver in the first place?

(That's just a guess from reading code from my phone so maybe I'm wrong and we should test, but worth checking that first)

trupin commented 5 years ago

Thanks for your answer, @AliSoftware.

I looked in my code for one of these 3 points you mentioned and I finally figured it out.

I was writing something like this in my main target:

extension Optional: ArgumentConvertible, CustomStringConvertible where Wrapped: ArgumentConvertible {

    public init(parser: ArgumentParser) throws {
        self = try? Wrapped(parser: parser)
    }

    public var description: String {
        return self?.description ?? "none"
    }
}

Turns out that the override of description apparently overrides the description of Optional<String>s served by reflexion in Stencil. What's extremely strange to me is that this extension isn't even located in the same target than the one using Stencil. I use Stencil in WeaverCodeGen and the extension is written in WeaverCommand. I'm not an expert with reflexion, but sounds to me this is a Swift bug.

Anyways, it was really tricky and I'm happy I found what was happening.

AliSoftware commented 5 years ago

Ah, glad you found it, great investigative work :+1: