-
Notifications
You must be signed in to change notification settings - Fork 570
Fix possible panic during internalization/externalization of time.Time/Date. #285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Test is currently failing with:
@neelance, since you're more familiar with Circle CI (I am more familiar with Travis), do you know how we can make it so the compiled Edit: This has been resolved in cf3fd3a. |
Looks like a good test for the issue. LGTM |
An empty .go file is needed because this package is empty aside from tests. Rename it to doc.go since that's a more common name and it becomes more readable at a glance (e.g., people can know it will not contain any code without even having to open/look in it).
…ime/Date. This is a test for issue #279. Proper testing for this issue prevents "testing" package from being imported. So, implement it as a low level single file .go test and invoke it from backend. In the time_inexternalization.go, you can use these statements for manual testing: var _ = time.Now() // Force time.Time not to be DCEed. var _ = time.Unix(0, 0) // Force time.Unix (and time.Time implicitly) not to be DCEed. Adding both of those statements (or just time.Unix one, since it also includes time.Time indirectly) would cause the test to pass. But a true fix will be in the compiler itself, in a way that will avoid the panics/incorrect behavior even when time.Time and/or time.Unix are DCEed, or time package is not imported at all.
This allows tests to invoke gopherjs binary when needed. According to https://circleci.com/docs/environment-variables, $PATH includes /home/ubuntu/bin and $HOME is /home/ubuntu.
…e/Date. Fixes #279 (initial report). The internalization/externalization code only checks if the time package is present and assumes time.Unix and time.Time are available if so. Previously, it was possible for time package to be present, but either time.Unix or time.Time were not (due to dead code elimination), causing possible panics. In time package itself, ensure time.Unix func and time.Time struct it returns are always included (despite DCE) to prevent that from happening.
d0cc457
to
6e0c1e9
Compare
This is ready to merge now and resolves the issue fully. LGTM. |
Fixes #279 (additional findings). When time package is not imported, it's still possible to internalize a Date object (into an interface{}). Due to fallthrough being default, this resulted in Function case running when internalizing Date with no time package. That makes no sense, and means that trying to externalize the Date object back into JavaScript world would fail to produce the original Date object (also, calling methods on it would obviously fail, since it's not a real Date object). For example, consider the following code that doesn't import time package, it can now run and produce expected results: date := js.Global.Get("Date").New().Interface() js.Global.Get("console").Call("log", date)
Apply feedback from #286 (comment).
6e0c1e9
to
8bffb58
Compare
(I've reworded the second last commit to have a slightly better commit message. It went from "Internalize Date when time package not imported in a reasonable way." to "Internalize Date in a reasonable way when time package not imported.") |
…tion Fix possible panic during internalization/externalization of time.Time/Date.
Begin by adding a failing test for issue #279.
Proper testing for this issue prevents "testing" package from being imported. So, implement it as a low level single file .go test and invoke it from backend. (See #284 for related context.)
In the
time_inexternalization.go
, you can use these statements formanual testing:
Adding both of those statements (or just
time.Unix
one, since it also includestime.Time
indirectly) would cause the test to pass. But a true fix will be in the compiler itself, in a way that will avoid the panics/incorrect behavior even whentime.Time
and/ortime.Unix
are DCEed, or time package is not imported at all.Edit: Implemented fix for the issue, so it now resolves #279.