It looks like the logic for externalize and internalize in prelude has some special handling of time.Time struct. It externalizes instances of time.Time package to new Date(...). It internalizes instances of Date into time.Time struct.

The current code has a guard that checks if time package is imported, but it then goes on to assume that time.Time struct must also be present if so. Due to dead code elimination, that may not be the case (if time.Time struct is not used in code anywhere, as in your example).

I see two possible approaches to fixing this, and after thinking about it pretty hard, I am still on the fence as to which is better.

Possible solution 1

One approach is to simply change the guard to check that both time package is imported and time.Time struct is available.

diff --git a/compiler/prelude/jsmapping.go b/compiler/prelude/jsmapping.go
index 38874da..5187f8b 100644
--- a/compiler/prelude/jsmapping.go
+++ b/compiler/prelude/jsmapping.go
@@ -89,7 +89,7 @@ var $externalize = function(v, t) {
     return s;
   case $kindStruct:
     var timePkg = $packages["time"];
-    if (timePkg && v.constructor === timePkg.Time.ptr) {
+    if (timePkg && timePkg.Time && v.constructor === timePkg.Time.ptr) {
       var milli = $div64(v.UnixNano(), new $Int64(0, 1000000));
       return new Date($flatten64(milli));
     }
@@ -271,7 +271,7 @@ var $internalize = function(v, t, recv) {
       return new $Bool(!!v);
     case Date:
       var timePkg = $packages["time"];
-      if (timePkg) {
+      if (timePkg && timePkg.Time) {
         return new timePkg.Time(timePkg.Unix(new $Int64(0, 0), new $Int64(0, v.getTime() * 1000000)));
       }
     case Function:

That will definitely fix the "Uncaught TypeError" error.

However, I find it hard to be extremely confident at this time that it's not going to cause trouble in some unforeseen edge case where time.Time is not available, yet Date is being internalized and fails to get internalized as time.Time, leading somehow to a change in behavior.

It seems like that should never happen because if you're doing any logic that depends on time.Time, then that struct would not be eliminated via dead code elimination, right? But then changes to more aggressive DCE or some other future changes may break this.

Possible solution 2

A more safe (to reason about), but possibly less efficient solution is to make it so that if time package is imported, then always make sure time.Time is not eliminated via DCE. This can be done by adding a reference to it in an augmented native time package:

diff --git a/compiler/natives/time/time.go b/compiler/natives/time/time.go
index fc79721..64169bf 100644
--- a/compiler/natives/time/time.go
+++ b/compiler/natives/time/time.go
@@ -9,6 +9,14 @@ import (
    "github.com/gopherjs/gopherjs/js"
 )

+// Make sure time.Time struct is always included with this package (despite DCE),
+// because it's needed for ex/internalization. See issue https://github.com/gopherjs/gopherjs/issues/279.
+func init() {
+   // avoid dead code elimination
+   t := Time{}
+   _ = t
+}
+
 type runtimeTimer struct {
    i       int32
    when    int64

Something similar is done in a few other places.

Doing this does end up bringing in (or rather, prevent from being eliminated) a significant part of time package, not just one small struct. See this diff in generated code.

@neelance, what are your thoughts?

Hmm, this is not easy. I think the first solution should work, since if you do o.Interface() to get a time.Time, you first have to do a type assertion to use it, e.g. o.Interface().(time.Time). This would be the reference to avoid DCE. You could also do a type assertion to some interface that has some of time.Time's methods, but I think this case is very very unlikely. We may also revisit this decision as soon as we have the more aggressive DCE.

That sounds good to me. It seems that solution 1 should not have issues. We can revisit this if it does.

Should I make a PR for it (with a test), or will you?

I'd appreciate a PR if you want to. :-)

Sure, I can do that. It'll be done within 16 hours (after sleep)!

Making a test for this is turning out to be a little problematic because importing "testing" package causes the bug to disappear, since that package imports "time" and uses time.Time internally. I can make it work, the test will end up having more bootstrapping code.

I've opened #284 with a proposal we change compiler tests to be done like Go compiler tests are done, rather than relying on a very high level mechanism of "testing" package.

If that's done, then in the future we can refactor this test to be shorter (a single .go file with what's needed to reproduce it).

On topic of this issue itself, while in the process of making a thorough test, I have discovered a flaw in my original fix and fixed it. It affects the internalize case only:

@@ -271,7 +271,7 @@ var $internalize = function(v, t, recv) {
       return new $Bool(!!v);
     case Date:
       var timePkg = $packages["time"];
-      if (timePkg) {
+      if (timePkg && timePkg.Time) {
         return new timePkg.Time(timePkg.Unix(new $Int64(0, 0), new $Int64(0, v.getTime() * 1000000)));
       }
     case Function:

Notice that the internalize case uses more things from "time" package than just timePkg.Time, it also uses timePkg.Unix. So I found a situation where if (timePkg && timePkg.Time) check was true, but it was still panicking because time.Unix was DCEed.

I will correct that fix to the following, which should fix that problem:

-if (timePkg && timePkg.Time) {
+if (timePkg && timePkg.Unix) { // timePkg.Time will be present implicitly because it's referenced by time.Unix.

If you prefer I write slightly less performant but more explicit check, then it can be:

-if (timePkg && timePkg.Time) {
+if (timePkg && timePkg.Unix && timePkg.Time) {

I've made another discovery. It is possible for the possible solution 1 approach to fail and produce incorrect behavior. I was able to come up with a concrete reproducible case for it.

In order to make my test very thorough, I wanted to ensure two things happen:

1. Exercise externalization/internalization and make sure it does not cause a panic.
2. Print out the results of externalization/internalization to make sure the values are correct.

I couldn't just print JavaScript Date object as is, because that would cause the output to be non-deterministic (default output depends on your local timezone, so tests would fail if timezone is incorrectly set).

So I went with the following code:

// Excercise internalization of JavaScript Date object (with its special handling of time.Time).
date := js.Global.Get("Date").New("2015-08-29T20:56:00.869Z").Interface()
js.Global.Set("myDate", date)
js.Global.Get("console").Call("log", js.Global.Get("myDate").Call("toUTCString"))

If I ensure time.Unix and time.Time are not DCEed (e.g., add var _ = time.Unix(0, 0) to above test program), then output is correct:

Sat, 29 Aug 2015 20:56:00 GMT

Otherwise, what ends up happening is the Date is internalized via the Function case, which doesn't make sense:

case Date:
  var timePkg = $packages["time"];
  if (timePkg && timePkg.Unix) { // timePkg.Time will be present implicitly because it's referenced by time.Unix.
    return new timePkg.Time(timePkg.Unix(new $Int64(0, 0), new $Int64(0, v.getTime() * 1000000)));
  }
  // No break here, so if above if condition is false, we'll fallthrough to case Function
  // and internalize it as that.
case Function:
  var funcType = $funcType([$sliceType($emptyInterface)], [$jsObjectPtr], true);
  return new funcType($internalize(v, funcType));

And the program panics. If I modify it to just js.Global.Get("console").Call("log", js.Global.Get("myDate")) without doing Call("toUTCString") on it, the output is not the original date:

[Function]

The above demonstrates that it's possible not to import "time" package at all in Go code, yet still try to internalize Date object and then externalize that object. In both cases, the if guard will be skipped, and some unexpected behavior will occur.

Conclusion is, fixing externalize case is easy. Fixing internalize so it works even if time package is not available is harder. Perhaps we need a custom internal type to use when time package isn't imported? Or reuse something like int since it's a rare case. Or force "time" package to always be imported, even if just for its time.Time and time.Unix funcs.

I will make a PR with just the failing test case, let's think and decide on how to best implement the fix.

I tried to keep going with possible solution 1, fixing the issues I discovered, but I'm finding it doesn't work well to handle all cases, so I will switch to using possible solution 2 (also modified to take into account and resolve the new findings) because it's simpler and solves all edge cases more nicely.

To elaborate on why possible solution isn't working well, it's the following reason:

Suppose your user program imports "time" package and uses only time.Time struct, but not time.Unix. So time.Time is available, but time.Unix gets DCEed. Now, imagine you do:

date := js.Global.Get("Date").New("2015-08-29T20:56:00.869Z").Interface().(time.Time)

This line panics, because the time.Time struct cannot be created without time.Unix being available (see #279 (comment) for details of why).

Dealing with that becomes problematic, as you have to consider the following situations:

• time package not imported at all
• time package imported, both time.Time and time.Unix DCEed
• time package imported, time.Time available, but time.Unix DCEed
• time package imported, both time.Time and time.Unix available

I've considered "vendoring" time.Unix func inside the internalization func, but that's not feasible because we also need to make sure that time.Local (type *time.Location) is available and not DCEed. So replacing the real time.Unix with a fake one is not feasible.

Instead of all that, I'll go with possible solution 2 which will reduce the number of cases to deal with to just two:

1. time package is not imported at all
2. time package imported, both time.Time and time.Unix available

That seems like the best solution. I'll make a PR (edit: #286) into PR #285 for a separate review. Comments or improvement suggestions, if any, are welcome.

Thanks for all the effort! :-)

No problem, glad to help make GopherJS a little better. :D

Thank you @govlas and @rusco for catching and reporting the issue.