Skip to content
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

Require elements in 1-to-n assignments to match targets exactly #11145

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Aug 26, 2021

Implements #2617 (comment).

a, b = [1, 2, 3] # Unhandled exception: Multiple assignment count mismatch
a, b = [1]       # Unhandled exception: Multiple assignment count mismatch

The first line expands to:

__temp_1 = [1, 2, 3]
if __temp_1.size != 2
  ::raise "Multiple assignment count mismatch"
end
a = __temp_1[0]
b = __temp_1[1]

Compile-time checks are applied for Tuple instances as well as unions consisting entirely of Tuple instances: (originally StaticArrays were supported too, but I dropped them because they cannot be splatted yet in certain contexts)

a, b, c = {1, 2} # Error: cannot assign Tuple(Int32, Int32) to 3 targets
a, b = {1, 2, 3} # Error: cannot assign Tuple(Int32, Int32, Int32) to 2 targets

# Error: cannot assign (Tuple(Int32, Int32) | Tuple(Int32, Int32, Int32, Int32)) to 3 targets
a, b, c = rand > 0.5 ? {1, 2} : {3, 4, 5, 6}

# no compile-time error is raised because assignment may succeed
a, b = rand > 1 ? {1, 2, 3} : {4, 5} # okay

# no compile-time error is raised because assignment "may" succeed
a, b = rand > 0.5 ? [1, 2, 3] : {3, 4, 5} # Unhandled exception: Multiple assignment count mismatch

Multiple assignments in the macro language also use the literal expander, so they raise too:

{% a, b = [1, 2, 3] %} # Error: Multiple assignment count mismatch

The affected multiple assignments in the standard library and the compiler are patched in separate commits.

Following the decision to opt in to breaking features with command-line flags, this feature is not enabled by default, and only available via -Dpreview_multi_assign (this is largely inspired by #7206 in particular, except we don't have a specialized Exception subclass here); it is expected that #10410 will leverage the same flag in some manner.

@HertzDevil HertzDevil changed the title Require element counts in 1-to-n assignments to match number of targets exactly Require elements in 1-to-n assignments to match targets exactly Aug 26, 2021
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

A codegen spec without the preview flag would be nice. I don't think we need to duplicate all specs without the flag, but we should make sure that non-conforming code still works as long as the flag is missing.

We'll also need to establish a means to keep track of such compiler flags so that we can activate them at the next major release. Maybe grepping might be sufficient for this, but I'd prefer a curated list somewhere.
Maybe for now just add a # TODO(2.0): comment?

This should probably become a warning at some time before being generally activated. It's probably better to skip that for now and just ship the pure preview functionality in the next release. But once we're certain it's going to stay, there should be a deprecation message in a later 1.x release.

src/compiler/crystal/codegen/primitives.cr Show resolved Hide resolved
src/compiler/crystal/semantic/cleanup_transformer.cr Outdated Show resolved Hide resolved
case type = temp_var.type
when UnionType
sizes = type.union_types.map { |union_type| constant_size(union_type) }
if sizes.none? &.in?(target_count, nil)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if partly matching types should really be accepted or if more strictness might be better.

Suggested change
if sizes.none? &.in?(target_count, nil)
unless sizes.all? &.in?(target_count, nil)

Copy link
Member

Choose a reason for hiding this comment

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

I'm leaning more on the side of leaving it as is. Right now tuples behave exactly the same as everything else, with the exception of the compile time error if there's really no way that the code will succeed.
And not to forget possible unions involving both tuples and arrays. As I understand, currently they are always allowed, which I also agree with.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Aug 27, 2021

We'll also need to establish a means to keep track of such compiler flags so that we can activate them at the next major release.

For simply enumerating the preview flags the makefile will do. Something like the following:

preview ?= ## Enable all preview compiler features
PREVIEW_FLAGS := -Dpreview_multi_assign
override FLAGS += $(if $(preview),$(PREVIEW_FLAGS) )

This allows CI to have both preview-enabled and disabled workflows, should that be necessary. But a public list of those flags should also be available in the language reference: crystal-lang/crystal-book#543

This should probably become a warning at some time before being generally activated.

What would they look like? For compile-time errors this is obvious, but do we print to STDERR for the runtime checks?

@straight-shoota
Copy link
Member

What would they look like?

For runtime errors this is definitely a challend. Probably shouldn't spit out random warnings during runtime.
But the compile time errors can easily become deprecation warnigns.

@Fryguy
Copy link
Contributor

Fryguy commented Aug 27, 2021

Is this meant to be merged in combination with #10410? As a user, I wouldn't like this change on its own without that other one in place. It's not clear in the OP here, but I kind of assumed it from your other comment here: #8436 (comment)

@oprypin
Copy link
Member

oprypin commented Oct 28, 2021

@HertzDevil You mention

it is expected that #10410 will leverage the same flag in some manner.

But it actually seems the best to not put them under the same flag.

Consider this comment: #10410 (comment)
By writing a, b, *_ = [1, 2, 3, 4, 5] one can make code compatible with both the old and the new way. However, if splats are available only in the "new way", that can't be done.

So actually the PR #10410 about supporting splats should go first (possibly in the upcoming release) and doesn't need to be behind a flag if it doesn't break anything.

Both this and #10410 are wanted features, let's just figure out the details how to get there.

@oprypin
Copy link
Member

oprypin commented Nov 12, 2021

@HertzDevil Now this pull request has a clear path forward :)

I don't think much will need to be changed, but please merge in the latest code, that'll be needed for sure.

@oprypin oprypin self-requested a review November 14, 2021 17:08
@oprypin oprypin self-requested a review November 21, 2021 13:44
@oprypin

This comment has been minimized.

Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

No outstanding comments from me, just need to rename the flag to strict_multi_assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants