Refactor NavigationView

[ezraberch]

This PR refactors NavigationView in the TokamakStaticHTML renderer. While this should have no functional effect, it fixes the crash in #445. This is likely due to an underlying bug in swiftwasm.

[MaxDesiatov]

Just a quick note: if the workaround in #367 (comment) also makes the crash go away (I didn't have time to verify that myself yet), this isn't a bug in SwiftWasm, but the fact that Tokamak consumes a lot of stack memory, which causes a stack overflow.

[jhoughjr]

I was able to get around that early on by passing some stack size arg in but don’t recall what it was. I noted that doubling it again when it happened did not work at all.

…

On Sep 10, 2021, at 9:59 AM, Max Desiatov ***@***.***> wrote: Just a quick note: if the workaround in #367 (comment) <#367 (comment)> also makes the crash go away (I didn't have time to verify that myself yet), this isn't a bug in SwiftWasm, but the fact that Tokamak consumes a lot of stack memory, which causes a stack overflow. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAL5QCIFRBEIHKLG3SWQSFTUBIMLNANCNFSM5DZPAMSQ>.

[ezraberch]

Just a quick note: if the workaround in #367 (comment) also makes the crash go away (I didn't have time to verify that myself yet), this isn't a bug in SwiftWasm, but the fact that Tokamak consumes a lot of stack memory, which causes a stack overflow.

That does make it work, but here's why I think there's still an underlying wasm issue.

If the problematic code is moved to an unreachable code path...

              if toolbarContent.isEmpty {
                HTML("div", ["class": "_tokamak-toolbar-content _tokamak-toolbar-leading"]) {
                  title.font(.headline)
                }
              } else if true {
                HTML("div", ["class": "_tokamak-toolbar-content _tokamak-toolbar-leading"]) {
                  title.font(.headline)
                }
              } else {
                HTML("div", ["class": "_tokamak-toolbar-content _tokamak-toolbar-leading"]) {
                  items(from: toolbarContent, at: .navigationBarLeading)
                  items(from: toolbarContent, at: .navigation)
                  title
                    .font(.headline)
                  items(from: toolbarContent, at: .navigationBarTrailing)
                  items(from: toolbarContent, at: .automatic, .primaryAction)
                  items(from: toolbarContent, at: .destructiveAction)
                    .foregroundColor(.red)
                }
                HTML("div", ["class": "_tokamak-toolbar-content _tokamak-toolbar-center"]) {
                  items(from: toolbarContent, at: .principal, .status)
                }
                HTML("div", ["class": "_tokamak-toolbar-content _tokamak-toolbar-trailing"]) {
                  items(from: toolbarContent, at: .cancellationAction)
                  items(from: toolbarContent, at: .confirmationAction)
                    .foregroundColor(.accentColor)
                }
              }

NavigationView.swift:39:17: warning: will never be executed

...then the stack overflow remains. However, commenting out the code in the unreachable block will eliminate the overflow.

[MaxDesiatov]

My hunch that's how result builders work. if statement there is transformed into an expression, which allocates all of the view values in both branches to yield a value of _ConditionalContent. In that sense there's no truly "unreachable" blocks with result builders, the only way to prevent them from being eagerly evaluated and blowing up the stack is to hide them in a separate View type, or to comment them out completely.

Creating a new View type makes that view "lazy", meaning that its body content will only evaluate when that view is supposed to be rendered.

I may be wrong here. Just looking at the proposal, it doesn't say anything anywhere about if branches being eager or lazy. We may be able to come up with a separate test case that proves this though. Something like

if true {
  Foo()
} else {
  Bar()
}

and then both Foo and Bar initializers should have a print statement in them to show which of the branches are evaluated. Sorry, I'm a bit limited with time this week to try it myself. Just spitballing some ideas here.

[ezraberch]

To test your theory, I used the following function:

  func printAndReturnText(_ text: String) -> Text {
    print(text)
    return Text(text)
  }

Then I added a call in each of the three code paths. With TokamakDemo, I get just the second code path. With the minimal test case, I get no print statements at all.

[MaxDesiatov]

Hm, fair enough. @kateinoigakukun do you have any ideas? Could this be something related to how function builders work, or is there something else going on that isn't a simple stack overflow?

[MaxDesiatov]

On a second thought, merging this as a temporary solution is definitely worth it 👍

[kateinoigakukun]

Sorry for slow response. I'll take a look later but it's ok to merge for now.

[revolter]

it fixes the crash in #445

For me, this PR didn't actually fix the crash in #445.

[revolter]

For me, this PR didn't actually fix #445.