Fix: Disallow setters with multiple arguments #6324

maxfierke · 2018-07-04T01:47:24Z

Fixes #5856 by throwing a syntax error when defining a setter method that accepts more than one argument, seeing as there is no way to call it.

Fixes crystal-lang#5856

straight-shoota · 2018-07-04T05:00:33Z

src/compiler/crystal/syntax/parser.cr

@@ -3344,6 +3347,10 @@ module Crystal
          index += 1
        end

+        if is_setter && args.size > 1


Can't is_setter be replaced by name.ends_with?('=')?

oh yeah! good catch

asterite · 2018-07-04T12:16:04Z

Also check named args, no splat, etc.

…amed args

maxfierke · 2018-07-04T15:20:42Z

@asterite added some checks for named args, splat, kwargs, and then also block args, since there's a similar issue calling those as well

asterite · 2018-07-04T15:30:05Z

src/compiler/crystal/syntax/parser.cr

@@ -3332,6 +3333,7 @@ module Crystal
          if block_arg = extras.block_arg
            compute_block_arg_yields block_arg
            check :")"
+            found_block = true


!block_arg.nil? == found_block

So the found_block var can be removed.

Ah, but I also see found_splat, etc., so it's fine to keep this variable 👍

straight-shoota · 2018-07-11T19:41:01Z

spec/compiler/parser/parser_spec.cr

@@ -174,6 +174,13 @@ module Crystal
    assert_syntax_error "def foo!=; end", "unexpected token: !="
    assert_syntax_error "def foo?=(x); end", "unexpected token: ?"

+    # 5856


straight-shoota · 2018-07-11T19:42:33Z

spec/compiler/parser/parser_spec.cr

@@ -174,6 +174,13 @@ module Crystal
    assert_syntax_error "def foo!=; end", "unexpected token: !="
    assert_syntax_error "def foo?=(x); end", "unexpected token: ?"

+    # 5856
+    assert_syntax_error "def foo=(a,b); end", "setter method 'foo=' cannot receive more than one argument"


s/receive/have/. "receive" is more associated to the call site. Same for the others. Just my opinion.

seems reasonable. have is a better hint at something static too.

straight-shoota · 2018-07-11T19:59:57Z

src/compiler/crystal/syntax/parser.cr

@@ -3344,6 +3346,14 @@ module Crystal
          index += 1
        end

+        if name.ends_with?('=') && !name.ends_with?("[]=")


The second test can be name == "[]=". There can't be anything before []= in a method name.

However, is there a particular reason why []= can be called like any method but all other setters can't?
If not, the same rules should apply (with args.size != 2).

With []= you can do var[1, 2] = 3 to implement a matrix type for example

Okay, that's a great use case. You can even do var[1, foo: 2] = 3 👀

But it still doesn't explain foo.[]=(foo) do |baz| end 😃

doh! I'll switch that to an equality check on name. @bew's totally right on with the []= with multi-arg case. There's at least one place in stdlib where []= is given splat args and kwargs too.

Good catch on the block case for []=. I'll address that.

crystal/src/compiler/crystal/syntax/parser.cr

Lines 649 to 653 in 8d28cbb

# If we have `f.x=(exp1).a.b.c`, consider it the same as `f.x = (exp1).a.b.c`

# and not as `(f.x = exp1).a.b.c` because a difference in space

# should not make a difference in semantic (#4399)

# The only exception is doing a splat, in which case this can only

# be expanded arguments for the call.

And because (exp1, exp2) is not a valid syntax, f.x=(exp1, exp2) is not a valid syntax either, so there's no way to pass multiple arguments there. It's the same as in Ruby (I copied it from Ruby, actually).

And the exception for splat mentioned there maybe should be removed (it doesn't work in Ruby, so...)

straight-shoota · 2018-07-11T20:02:29Z

yield should also be a syntax error in a setter method. It's not part of the method signature, so it must be implemented differently, and doesn't need to be in this PR.

straight-shoota · 2018-07-12T07:27:59Z

@maxfierke []= can currently receive a block argument. This should only be disallowed in the method signature if it doesn't work at call site. We could change that, but it isn't yet. And maybe it shouldn't.

straight-shoota · 2018-07-12T12:15:40Z

To clarify: Currently, []= can be called with a block (var.[]= {}). I just think we shouldn't disallow the declaration without disallowing the call. Either we allow both or disallow both. I'm not sure about it... and it probably doesn't make a any difference whatsoever.

sdogruyol

Thank you @maxfierke 👍

bcardiff

I don't like the special treatment of []= regarding blocks.

Where are setters with blocks used in the stdlib? Maybe it is better to disallow blocks in setters.

So far the only difference of []= with respect other setters is that []= allows multiple arguments because of multiple indices. m[i, j] = v

maxfierke · 2018-08-08T14:49:26Z

@bcardiff Are you referring to the changes in src/object.cr?

blocks are disallowed for all setters with this PR. The special case here for []= is to avoid generating block-supporting method forwards for []=.

bcardiff · 2018-08-08T14:54:58Z

Oh, I missed that it was inside a method.id.ends_with?('='). So that's fine.

But I am still missing why there is an if name == "[]=" in the changeset for parser.cr. Yet the specs clearly says that foo=(&block) is disallowed 🤔

maxfierke · 2018-08-08T15:10:53Z

@bcardiff ahhh! I assume you're referring to here. That is to disallow calls to []= with a block at the call site in response to this feedback: #6324 (comment). This way, users get a compiler error instead of a silent failure if for some reason they're calling []= with a block somewhere and expecting it to work (e.g. doing so caught the invalid generation via macro of the aforementioned block-supporting forward for []= in src/object.cr)

bcardiff · 2018-08-08T15:15:52Z

Ahhh! The parsing snippet is because []= is parsed as [, args, ]. =, etc. So the stressed path is not the same as method=

Fix: Disallow setters with multiple arguments

4e018ac

Fixes crystal-lang#5856

straight-shoota reviewed Jul 4, 2018

View reviewed changes

maxfierke added 2 commits July 4, 2018 09:57

parser: prevent setter methods with splat args, kwargs, or multiple n…

724d20e

…amed args

parser: prohibit setter method from receiving a block

5d44d5d

asterite reviewed Jul 4, 2018

View reviewed changes

straight-shoota reviewed Jul 11, 2018

View reviewed changes

parser: prevent use of block with []= assignment method

6a8c7b3

RX14 approved these changes Jul 12, 2018

View reviewed changes

maxfierke added 2 commits July 15, 2018 18:57

Merge branch 'master' into mf-disallow_mutli_arg_setters

f1645b0

parser: disallow calling setters with a block

9697515

RX14 approved these changes Jul 16, 2018

View reviewed changes

RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Jul 16, 2018

straight-shoota mentioned this pull request Jul 20, 2018

Replace Indexable#at with #fetch #6296

Merged

sdogruyol approved these changes Jul 28, 2018

View reviewed changes

RX14 requested a review from bcardiff July 30, 2018 16:10

bcardiff reviewed Aug 8, 2018

View reviewed changes

bcardiff approved these changes Aug 8, 2018

View reviewed changes

bcardiff merged commit 0ff17f1 into crystal-lang:master Aug 8, 2018

bcardiff added this to the 0.26.0 milestone Aug 8, 2018

bcardiff mentioned this pull request Aug 9, 2018

Upcoming breaking change in crystal 0.26.0 jwaldrip/shell-table.cr#2

Closed

straight-shoota mentioned this pull request Feb 15, 2021

>= and <= considered as setters, allow only one arguments, unlike > and < #10397

Closed

HertzDevil mentioned this pull request Jan 2, 2024

Allow calling #[]= with a block using method syntax #14161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Disallow setters with multiple arguments #6324

Fix: Disallow setters with multiple arguments #6324

maxfierke commented Jul 4, 2018

straight-shoota Jul 4, 2018

maxfierke Jul 4, 2018

asterite commented Jul 4, 2018

maxfierke commented Jul 4, 2018

asterite Jul 4, 2018

asterite Jul 4, 2018

straight-shoota Jul 11, 2018

straight-shoota Jul 11, 2018

maxfierke Jul 11, 2018

straight-shoota Jul 11, 2018

bew Jul 11, 2018

straight-shoota Jul 11, 2018 •

edited

Loading

maxfierke Jul 11, 2018 •

edited

Loading

asterite Jul 11, 2018

asterite Jul 11, 2018

asterite Jul 11, 2018

straight-shoota commented Jul 11, 2018

straight-shoota commented Jul 12, 2018

straight-shoota commented Jul 12, 2018

sdogruyol left a comment

bcardiff left a comment •

edited

Loading

maxfierke commented Aug 8, 2018

bcardiff commented Aug 8, 2018

maxfierke commented Aug 8, 2018

bcardiff commented Aug 8, 2018

	# If we have `f.x=(exp1).a.b.c`, consider it the same as `f.x = (exp1).a.b.c`
	# and not as `(f.x = exp1).a.b.c` because a difference in space
	# should not make a difference in semantic (#4399)
	# The only exception is doing a splat, in which case this can only
	# be expanded arguments for the call.

Fix: Disallow setters with multiple arguments #6324

Fix: Disallow setters with multiple arguments #6324

Conversation

maxfierke commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jul 4, 2018

maxfierke commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

maxfierke Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Jul 11, 2018

straight-shoota commented Jul 12, 2018

straight-shoota commented Jul 12, 2018

sdogruyol left a comment

Choose a reason for hiding this comment

bcardiff left a comment • edited Loading

Choose a reason for hiding this comment

maxfierke commented Aug 8, 2018

bcardiff commented Aug 8, 2018

maxfierke commented Aug 8, 2018

bcardiff commented Aug 8, 2018

straight-shoota Jul 11, 2018 •

edited

Loading

maxfierke Jul 11, 2018 •

edited

Loading

bcardiff left a comment •

edited

Loading