Skip to content

compiler/prelude: Keep nil slice nil when slicing. #852

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

Merged
merged 2 commits into from
Aug 20, 2018
Merged

Conversation

dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 20, 2018

According to the Go specification:

If the sliced operand of a valid slice expression is a nil slice,
the result is a nil slice.

This behavior was not implemented and not caught previously. Slicing a nil slice would incorrectly make it an empty but non-nil slice. This change fixes that.

The minified prelude was regenerated with:

go generate ./compiler/prelude

Fixes #851.

According to the Go specification:

> If the sliced operand of a valid slice expression is a nil slice,
> the result is a nil slice.

This behavior was not implemented and not caught previously.
This change fixes that.

The minified prelude was regenerated with:

	go generate ./compiler/prelude

Fixes #851.
@dmitshur dmitshur mentioned this pull request Aug 20, 2018
var s []int
s = s[:]
s = s[0:0]
s = s[0:0:0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the same if s != nil above this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting this in case one of the assigments makes s non-nil, but the following line makes it nil again, and we don't catch it?

Copy link
Member

@hajimehoshi hajimehoshi Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As s is expected to be nil, I thought you don't have to reassign nil, but probably reassiging would be better. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I reassigned just to be sure that the error messages would be accurate if printed.

s = s[0:0]
s = s[0:0:0]
if s != nil {
t.Errorf("nil slice became non-nil after slicing: %#v", s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the expectation, but would it be better to reword this including expectation?

var s = []int{}
s = s[:]
if s == nil {
t.Errorf("non-nil slice became nil after slicing: %#v", s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Use the more common "got <x>, want <y>" style.
@dmitshur
Copy link
Member Author

Done, PTAL.

Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dmitshur dmitshur merged commit 89baedc into master Aug 20, 2018
@dmitshur dmitshur deleted the fix-slicing-nil branch August 20, 2018 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nil slice becomes non-nil after slicing.
2 participants