compiler: fix variadic args not being nil when zero length. #807

myitcv · 2018-04-26T08:41:39Z

Fixes #806.

hajimehoshi · 2018-04-26T12:57:29Z

tests/compiler_test.go

+	if v := printVari(); v != nil {
+		t.Errorf("expected printVari() to be %v; got: %v", nil, v)
+	}
+}


Isn't tests/misc_test.go a proper place?

The tests package needs a bit of a cleanup at some point. The segmentation is, I agree, not great. I even be tempted to have subpackages to better segment things.

But that belongs in a later PR. Not critical either way in my opinion whether this goes in misc_test.go or a new file.

I was wondering why compiler_test.go didn't exist until this PR.

bzub · 2018-04-26T14:17:03Z

I tried this out and everything looks good on my end. Thank you.

dmitshur

Two comments about the test. Aside from that, not seeing issues.

dmitshur · 2018-04-26T16:55:18Z

tests/compiler_test.go

@@ -0,0 +1,17 @@
+// +build js


We use t.Skip rather than build constraints for skipping tests meant for GOARCH=js only, unless the build constraint is necessary for things to compile.

In this case, this test shouldn't be skipped on non-js; I want to see it run and pass using gc as well (just like all other tests in this package that which can pass with gc).

So, remove this line.

dmitshur · 2018-04-26T16:58:20Z

tests/compiler_test.go

+		return strs
+	}
+
+	if v := printVari(); v != nil {


It'd be nice to be more consistent and stick with the got, want test style:

if got := printVari(); got != nil { t.Errorf("printVari() returned: %#v, want: %#v", got, []string(nil)) }

I'm intentionally using untyped nil here; but I've renamed the got variable.

myitcv · 2018-04-26T17:10:35Z

Thanks @shurcooL. I've pushed up a commit that addresses your feedback.

dmitshur

LGTM. Thanks!

dmitshur · 2018-04-26T17:29:10Z

tests/compiler_test.go

+	}
+
+	if got := printVari(); got != nil {
+		t.Errorf("expected printVari() to be %v; got: %v", nil, got)


I want us to be more consistent with the style used in most Go tests, which is to print the "got" value first, the "want" value second. Mind changing it?

t.Errorf("printVari() returned: %#v, want: %#v", got, []string(nil))

Using %#v instead of %v has the advantage that it actually differentiates nil slice vs non-nil empty slice. %v prints both as [].

Done. Just pushed up a commit that does this.

Please can I ask in return that you start a wiki, that we can then build on, which documents the conventions you would like to follow?

Thanks. But why did you replace []string(nil) with just nil? It's not the same type. Not critical, just pointing out.

Please can I ask in return that you start a wiki, that we can then build on, which documents the conventions you would like to follow?

We already have exactly that. Please see https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#style.

(Actually, https://play.golang.org/p/L31k_UZViaQ is a better example of what I was pointing out.)

But why did you replace []string(nil) with just nil? It's not the same type. Not critical, just pointing out.

Yes, I know it's not the same type. But as I said in the comment above (that you 👍 -ed) it is an intentional use of nil vs []string(nil), precisely because the GopherJS compiler will see these as different types. We want to ensure that the comparison with untyped nil works (we could also add a test for typed nil but that doesn't seem particularly important).

We already have exactly that. Please see https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#style.

Great, thanks - I'd clearly missed this before.

Yes, I know it's not the same type. But as I said in the comment above (that you 👍 -ed) it is an intentional use of nil vs []string(nil), precisely because the GopherJS compiler will see these as different types.

I thought you were referring to using nil in the if statement condition. That's what my 👍 reaction was referring to. Using an untyped nil there makes sense, since if got != []byte(nil) is (sadly) not allowed in Go (i.e., it'd be a compilation error).

We want to ensure that the comparison with untyped nil works (we could also add a test for typed nil but that doesn't seem particularly important).

My comments aren't about the comparison (keeping it as is is fine), they're about the t.Errorf error message. I was hoping the https://play.golang.org/p/L31k_UZViaQ snippet would explain why my suggestion for changing the t.Errorf message would make it slightly more clear.

This is such a trivial thing, I don't mind the existing error message either (that's why this PR has my LGTM since earlier). I just wanted to clarify what I meant.

Great, thanks - I'd clearly missed this before.

No problem.

I think the Go and Upspin projects serve as really good references for when there's uncertainty about how to best do something. The way I try to think about it is this: if it were a CL sent to the Go project and someone like Brad Fitzpatrick or Ian Lance Taylor saw it, would they be happy to approve it? It's a really good practice to look over the CLs to the Go project and learn from the code and reviews there.

hajimehoshi

I'm still not sure the test rule, but I'm fine as @shurcooL is fine

hajimehoshi · 2018-04-29T19:52:01Z

tests/compiler_test.go

+	if v := printVari(); v != nil {
+		t.Errorf("expected printVari() to be %v; got: %v", nil, v)
+	}
+}


I was wondering why compiler_test.go didn't exist until this PR.

myitcv · 2018-04-29T19:54:47Z

@hajimehoshi indeed, compiler_test.go didn't exist before; I took the opportunity of this PR to start the process of splitting things up a bit. Could have waited until a later PR, but I didn't think it was too big a deal to start sooner rather than later!

hajimehoshi · 2018-04-29T20:05:43Z

Yeah, I don't mean we should not have compiiler_test.go now, but rather I wanted to know the current direction. I'm also fine with splitting this :-)

dmitshur · 2018-04-29T21:32:45Z

I wonder if we handle the case where an empty but non-nil slice is passed to a variadic function via .... I.e., the 4th case in this snippet: https://play.golang.org/p/357N56KuktI.

dmitshur · 2018-04-29T21:40:28Z

I tested it, and that case is handled correctly. But, I think it'd be great to add a test case for it (as far as I can see, we don't have one). Mind adding one @myitcv? (Otherwise, I can add it before merging.)

myitcv · 2018-04-30T06:32:21Z

Mind adding one @myitcv?

No problem @shurcooL I've pushed up a couple of extra tests.

paralin · 2018-08-03T23:57:22Z

Merge?

nevkontakte · 2021-10-23T17:45:22Z

@myitcv I'm finally getting through the backlog of issues and pull requests. Could you rebase this PR on top of the latest master so that we can re-run the current test suite? Sorry it took this long...

flimzy · 2021-12-27T14:39:44Z

I've rebased this here: https://github.com/gopherjs/gopherjs/tree/fix_806 and opened a new PR #1096 to replace this one.

compiler: fix variadic args not being nil when zero length.

2cad84c

Fixes gopherjs#806.

myitcv mentioned this pull request Apr 26, 2018

Omitted variadic function parameter should be nil #806

Closed

hajimehoshi reviewed Apr 26, 2018

View reviewed changes

dmitshur requested changes Apr 26, 2018

View reviewed changes

Address feedback

2084791

dmitshur approved these changes Apr 26, 2018

View reviewed changes

Address more feedback

00b561f

hajimehoshi approved these changes Apr 29, 2018

View reviewed changes

Add a couple more tests for varidic nil/non-nil checks

cafa873

myitcv mentioned this pull request Oct 10, 2018

Support for vgo builds outside GOPATH #835

Closed

nevkontakte self-assigned this Oct 23, 2021

nevkontakte added the WaitingForInfo label Oct 23, 2021

flimzy mentioned this pull request Dec 28, 2021

compiler: fix variadic args not being nil when zero length. #1096

Merged

flimzy closed this Dec 28, 2021

compiler: fix variadic args not being nil when zero length. #807

compiler: fix variadic args not being nil when zero length. #807

Uh oh!

Conversation

myitcv commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzub commented Apr 26, 2018

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myitcv commented Apr 26, 2018

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajimehoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myitcv commented Apr 29, 2018

Uh oh!

hajimehoshi commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Apr 29, 2018

Uh oh!

myitcv commented Apr 30, 2018

Uh oh!

paralin commented Aug 3, 2018

Uh oh!

nevkontakte commented Oct 23, 2021

Uh oh!

flimzy commented Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dmitshur Apr 26, 2018 •

edited

Loading

dmitshur Apr 26, 2018 •

edited

Loading

dmitshur Apr 26, 2018 •

edited

Loading

hajimehoshi commented Apr 29, 2018 •

edited

Loading

dmitshur commented Apr 29, 2018 •

edited

Loading

flimzy commented Dec 27, 2021 •

edited

Loading